Bluetooth: conn: Expose simplified connection state in get_info #42540

Thalley · 2022-02-07T13:43:13Z

Add a state field in struct bt_conn_info that is a simplified
version of the internal state value (bt_conn_state_t).

This should provide an application to better determine the state
of the connection whne calling bt_conn_get_info, in case the
application does not keep track of the state itself.

Signed-off-by: Emil Gydesen [email protected]

fixes #28009

asbjornsabo · 2022-02-08T12:51:18Z

include/bluetooth/conn.h

+	/** Channel disconnected */
+	BT_CONN_STATE_DISCONNECTED,
+	/** Channel in connecting state */
+	BT_CONN_STATE_CONNECT,


I would prefer to add "ING" -> BT_CONN_STATE_CONNECTING

That is what the documentation says

It ensures that the name is not a subset of the next name (CONNECTED).

Ditto for DISCONNECT -> DISCONNECTING

IIRC the names were originally chosen to reflect those used by Linux: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/net/bluetooth/bluetooth.h#n222

However, I can see that we're anyway not following these 1:1, so I'm not opposed to changing them.

I am not opposed to changing them either. If we decide to change them, so that change also be applied to the L2CAP, ISO and internal BT CONN states?

Yes, it'd make sense to be consistent with the naming across the various layers and protocols

Will add additional commit to fix that

Was that additional commit ever added?
I would like to have it. (But if we decided not to do that, I will approve as is.)

It has not been added yet, will do that soon™

Added it now. Will do the L2CAP renaming in a different PR as that requires an API change.

Add a state field in struct bt_conn_info that is a simplified version of the internal state value (bt_conn_state_t). This should provide an application to better determine the state of the connection whne calling bt_conn_get_info, in case the application does not keep track of the state itself. Signed-off-by: Emil Gydesen <[email protected]>

Add `ing` to the `BT_CONN_CONNECT` and `BT_CONN_DISCONNECT` states, so that the name better matches the actual state. Signed-off-by: Emil Gydesen <[email protected]>

Add `ing` to the `BT_ISO_CONNECT` and `BT_ISO_DISCONNECT` states, so that the name better matches the actual state. Signed-off-by: Emil Gydesen <[email protected]>

Change from BT_ISO_<state> to BT_ISO_STATE_<state> to make the value more descriptive. Signed-off-by: Emil Gydesen <[email protected]>

Add an enum for the state field, to provide more semantics to it. Signed-off-by: Emil Gydesen <[email protected]>

Thalley · 2022-02-22T14:34:26Z

@jhedberg @asbjornsabo Changing it from CONNECT to CONNECTING isn't hard, but it will require an API change to L2CAP if we want it to be the same.

I've also updated ISO to use the same naming scheme as introduced by bt_conn_state

jhedberg · 2022-02-25T11:19:35Z

include/bluetooth/iso.h

@@ -114,7 +114,7 @@ struct bt_iso_chan {
 	struct bt_iso_chan_ops		*ops;
 	/** Channel QoS reference */
 	struct bt_iso_chan_qos		*qos;
-	uint8_t				state;
+	enum bt_iso_state		state;


Is it intentional that you’re increasing the struct member size here. We’ve used packed enums in some places where it matters.

IIRC the size of the enum should stay at 8-bit (given the possible values), right?

In any case, this value is for the host only, and the size of it isn't used for any size-specific fields.

I can __packed it, but I thought we generally wanted to avoid that in public APIs?

AFAIK it’s always promoted to word size if you don’t use packed.

Anyway, not an issue if it’s not critical to compact the struct here

AFAIK it’s always promoted to word size if you don’t use packed.

From what I can tell, that's implementation (compiler) specific, and the only requirement is that an enum shall be able hold value up to values representable by an int.

In any case, this a minor case and I suggest to leave as is. I like the idea of using named enums as fields in the public API, as it gives a direct reference to values that may be used in the field

Thalley · 2022-03-25T10:07:47Z

@jhedberg Will you please re-review?

Thalley requested review from alwa-nordic, jhedberg and Vudentz as code owners February 7, 2022 13:43

github-actions bot added area: API Changes to public APIs area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Feb 7, 2022

Thalley requested review from asbjornsabo and cvinayak February 7, 2022 13:43

Thalley force-pushed the conn_info_state branch from bb516d6 to bb06dd6 Compare February 7, 2022 13:50

zephyrbot assigned jhedberg Feb 8, 2022

asbjornsabo reviewed Feb 8, 2022

View reviewed changes

Thalley force-pushed the conn_info_state branch from bb06dd6 to ccbd0db Compare February 22, 2022 08:20

Thalley added 2 commits February 22, 2022 15:13

Bluetooth: Host: Add ing to the bt_conn (dis)connect-ing state

d9e7824

Add `ing` to the `BT_CONN_CONNECT` and `BT_CONN_DISCONNECT` states, so that the name better matches the actual state. Signed-off-by: Emil Gydesen <[email protected]>

Thalley force-pushed the conn_info_state branch from ccbd0db to d9e7824 Compare February 22, 2022 14:22

Thalley requested a review from asbjornsabo February 22, 2022 14:23

asbjornsabo previously approved these changes Feb 22, 2022

View reviewed changes

Thalley added 3 commits February 22, 2022 15:28

Bluetooth: ISO: Add ing to the bt_iso (dis)connect-ing state

d1abee6

Add `ing` to the `BT_ISO_CONNECT` and `BT_ISO_DISCONNECT` states, so that the name better matches the actual state. Signed-off-by: Emil Gydesen <[email protected]>

Bluetooth: ISO: Add STATE infix for ISO state enum

7d4578c

Change from BT_ISO_<state> to BT_ISO_STATE_<state> to make the value more descriptive. Signed-off-by: Emil Gydesen <[email protected]>

Bluetooth: ISO: Add named enum for bt_iso_state

66e8a14

Add an enum for the state field, to provide more semantics to it. Signed-off-by: Emil Gydesen <[email protected]>

Thalley dismissed asbjornsabo’s stale review via 66e8a14 February 22, 2022 14:34

github-actions bot added area: Bluetooth Audio area: Samples Samples labels Feb 22, 2022

Thalley mentioned this pull request Feb 24, 2022

Bluetooth: L2CAP: Add ing to the bt_l2cap (dis)connect-ing state #43165

Merged

jhedberg reviewed Feb 25, 2022

View reviewed changes

Thalley requested review from asbjornsabo and jhedberg February 25, 2022 11:36

alwa-nordic approved these changes Feb 25, 2022

View reviewed changes

asbjornsabo approved these changes Mar 23, 2022

View reviewed changes

jhedberg approved these changes Mar 25, 2022

View reviewed changes

carlescufi merged commit fe91c1d into zephyrproject-rtos:main Mar 25, 2022

Thalley deleted the conn_info_state branch July 8, 2022 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: conn: Expose simplified connection state in get_info #42540

Bluetooth: conn: Expose simplified connection state in get_info #42540

Thalley commented Feb 7, 2022

asbjornsabo Feb 8, 2022

jhedberg Feb 8, 2022

Thalley Feb 8, 2022

jhedberg Feb 8, 2022

Thalley Feb 22, 2022

asbjornsabo Feb 22, 2022

Thalley Feb 22, 2022

Thalley Feb 22, 2022

Thalley Feb 25, 2022

Thalley commented Feb 22, 2022

jhedberg Feb 25, 2022

Thalley Feb 25, 2022

jhedberg Feb 25, 2022

jhedberg Feb 25, 2022

Thalley Feb 25, 2022 •

edited

Loading

Thalley commented Mar 25, 2022

Bluetooth: conn: Expose simplified connection state in get_info #42540

Bluetooth: conn: Expose simplified connection state in get_info #42540

Conversation

Thalley commented Feb 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley commented Feb 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Thalley commented Mar 25, 2022

Thalley Feb 25, 2022 •

edited

Loading