bluetooth: conn: move bt_conn_lookup_index to public header

[JordanYates]

Move the bt_conn_lookup_index declaration to a public header. This function is essentially the inverse of bt_conn_index, which is already public.

[rugeGerritsen]

Does this API have some concerns with regards to thread safety?

Let's say that the user at some point stores index 1. And then later calls bt_conn_lookup_index(1). I assume it cannot know that the returned connection is the same connection as used last time.

Nevertheless, I'm sure there are good use cases for this API. But we should make this clear to the user.

[JordanYates]

Does this API have some concerns with regards to thread safety?

I can't see why the concerns would be any different to bt_conn_index.

[rugeGerritsen]

I was just wondering: An index cannot be used as input to any of the Bluetooth APIs, so the user cannot do anything "wrong/unexpected". When the user gets a connection reference, this is no longer true

[Thalley]

I think it's a fair point from @rugeGerritsen

Our APIs use a conn pointer instead of an index for a reason, and there may be unknown side effects from the higher layers relying on the index instead of the point.

That being said, I don't think this is actually any different than our APIs for

int bt_hci_get_conn_handle(const struct bt_conn *conn, uint16_t *conn_handle);
struct bt_conn *bt_hci_conn_lookup_handle(uint16_t handle);

So any issues/challenges that may or not be with this suggestion function, already exists.

[alwa-nordic]

I think we should document the following clearly: There are two distinct use cases for bt_conn_lookup_index, as follows:

• The first use case is for looping over all connections. In this case, the user must not care about finding a specific connection, and there are no guarantees for that.
• The second use case is to round-trip a reference trough bt_conn_index and back again. In this case, the index is a reified reference, similar to a bt_conn pointer. For the purpose of tracking references, we say bt_conn_index moved the original reference into the index it returned. If the caller does not have a valid reference, bt_conn_lookup_index is not guaranteed to return the same connection object (or return one at all).

[alwa-nordic]

@jhedberg That's reasonable. But bt_conn_lookup_index plays no role in this use case. Is this an argument to close this PR?

As long as the "protocol data" contains information that the connection "is alive" the reverse lookup can still make sense and be useful.

I think we agree then. So long as the caller of bt_conn_lookup_index can prove the object it got that index from is still alive, then that's proof bt_conn_lookup_index returns that object. Tracking references is one type of proof, but I agree it's not the only one.

[alwa-nordic]

To steer this thread back on track: Does anyone have a good suggestion for API documentation for the average user to make it easy to use the new API correctly?

[Thalley]

Should/could we align this with bt_conn_foreach?

bt_conn_foreach uses bt_conn_ref to determine if the conn object has a reference, i.e. it only iterates on those that have a non-0 reference counter. Should bt_conn_lookup_index return NULL if the reference counter is 0?

Do we want the possible return values of bt_conn_lookup_index and bt_conn_foreach to differ?
If not, then I guess we could/should align the documentation of those 2 functions, as they effectively provide the same feature.

That also reminded me that bt_conn_lookup_index should take the type into account as well.
A ISO bt_conn object and an ACL bt_conn object may have the same index, so with the current suggested API we may have

uint8_t index = bt_conn_index(iso_conn); /* returns 0 */
struct bt_conn *conn = bt_conn_lookup_index(index); /* returns ACL at index 0 */

This hasn't been a problem when used internally, as that has always been used in an ACL context, but that cannot be guaranteed in a API context

[jhedberg]

Should bt_conn_lookup_index return NULL if the reference counter is 0?

Doesn't it already do that? Internally it calls bt_conn_ref() which returns NULL if the refcount is 0.

[Thalley]

Should bt_conn_lookup_index return NULL if the reference counter is 0?

Doesn't it already do that? Internally it calls bt_conn_ref() which returns NULL if the refcount is 0.

Ah, you are right. Then we can align with the documentation of bt_conn_foreach as they effectively do the same. When we add the type paramter to bt_conn_lookup_index, we can even just call bt_conn_lookup_index from bt_conn_foreach to ensure that their behavior is similar.

[Thalley]

I'd like to see the suggest missing documentation from #85546 (comment) being added.

A conn object returned by this function my not be connected nor contain valid values, which should be made clear to the user

[JordanYates]

Who knew this internal function was so problematic :)
I'm still not clear on what is actually desired from a documentation perspective.

[jhedberg]

A conn object returned by this function my not be connected nor contain valid values

It may indeed not be connected, but it should contain valid values since the API only returns objects with a pre-existing refcount > 0

[Thalley]

Who knew this internal function was so problematic :) I'm still not clear on what is actually desired from a documentation perspective.

My perspective:

1. It need to also include the type of connection we are looking up. We have several arrays of connections and the index is only unique per type.
2. The documentation of bt_conn_lookup_index should match the documentation of bt_conn_foreach; they are effectively doing the same, except that bt_conn_foreach will run on all possible IDs.
3. To ensure that bt_conn_foreach and bt_conn_lookup_index can share similar documentation, their behavior should be similar. I suggest to achieve this by modifying bt_conn_foreach to call bt_conn_lookup_index for each index.

If any additional documentation is required, that should also be added to bt_conn_foreach

[JordanYates]

Who knew this internal function was so problematic :) I'm still not clear on what is actually desired from a documentation perspective.

My perspective:

1. It need to also include the type of connection we are looking up. We have several arrays of connections and the index is only unique per type.

2. The documentation of `bt_conn_lookup_index` should match the documentation of `bt_conn_foreach`; they are effectively doing the same, except that `bt_conn_foreach` will run on all possible IDs.

3. To ensure that `bt_conn_foreach` and `bt_conn_lookup_index` can share similar documentation, their behavior should be similar. I suggest to achieve this by modifying `bt_conn_foreach` to call  `bt_conn_lookup_index` for each index.

If any additional documentation is required, that should also be added to bt_conn_foreach

That's all fair enough but is a pretty big explosion of scope from just moving a function from an internal to a public header.
Quite frankly this is a low value PR for me and I'm not going to get any value from the time spent pushing the changes.

[Thalley]

Who knew this internal function was so problematic :) I'm still not clear on what is actually desired from a documentation perspective.

My perspective:

1. It need to also include the type of connection we are looking up. We have several arrays of connections and the index is only unique per type.

2. The documentation of `bt_conn_lookup_index` should match the documentation of `bt_conn_foreach`; they are effectively doing the same, except that `bt_conn_foreach` will run on all possible IDs.

3. To ensure that `bt_conn_foreach` and `bt_conn_lookup_index` can share similar documentation, their behavior should be similar. I suggest to achieve this by modifying `bt_conn_foreach` to call  `bt_conn_lookup_index` for each index.

If any additional documentation is required, that should also be added to bt_conn_foreach

That's all fair enough but is a pretty big explosion of scope from just moving a function from an internal to a public header. Quite frankly this is a low value PR for me and I'm not going to get any value from the time spent pushing the changes.

That's fair :) Feel free to create a GH issue requesting this to become public, and then we can be aware of the use case and request and then use that to (hopefully) implement this in the future

[JordanYates]

That's fair :) Feel free to create a GH issue requesting this to become public, and then we can be aware of the use case and request and then use that to (hopefully) implement this in the future.

My interpretation of the blockers here was that the existing documentation (and implementation?) of the (already public) bt_conn_foreach is insufficient. If that was not the case, then copying the docs from bt_conn_foreach as requested would be simple.

[Thalley]

My interpretation of the blockers here was that the existing documentation (and implementation?) of the (already public) bt_conn_foreach is insufficient.

You can indeed use bt_conn_foreach and bt_conn_index to do the same, although in a less optimal way as you'd always iterate on all connections