Bluetooth: Host: Add advertising state to bt_le_ext_adv_info #90185

Thalley · 2025-05-19T21:40:28Z

The bt_le_ext_adv_info struct has been extended to also contain the advertising and periodic advertising states.

Additionally, the function verifies the input to avoid NULL pointer access, and more importantly does not return 0 with addr set to a valid that is not
used for advertising.

Thalley · 2025-05-19T21:54:13Z

There will be a use of these new fields in #85642:

We need to start advertising if not already (or reject a function call if it's not, TBD)
We need to reject a function call if periodic advertising is not configured

Thalley · 2025-05-21T09:05:17Z

Did also consider to also add the options as field to the info, but I don't have a use case for it at this moment

Copilot

Pull Request Overview

Adds tracking and reporting of extended and periodic advertising states to Bluetooth LE advertising info and shell output, with input validation to prevent NULL dereferences.

Extend bt_le_ext_adv_info with ext_adv_state and per_adv_state enums
Validate arguments and populate new state fields in bt_le_ext_adv_get_info
Print the new states in the shell command cmd_adv_info

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
subsys/bluetooth/host/shell/bt.c	Print extended/periodic advertising states in shell output
subsys/bluetooth/host/adv.c	Validate inputs and set advertising/periodic state in info struct
include/zephyr/bluetooth/bluetooth.h	Add enums for advertising states and update struct documentation

Comments suppressed due to low confidence (1)

subsys/bluetooth/host/shell/bt.c:2573

[nitpick] Rename the label 'Per Adv state' to 'Periodic Adv state' to more clearly reflect its meaning.

shell_print(sh, "Per Adv state: %d", info.per_adv_state);

subsys/bluetooth/host/adv.c

include/zephyr/bluetooth/bluetooth.h

subsys/bluetooth/host/shell/bt.c

Copilot

Pull Request Overview

This PR extends the advertising information structure to include extended and periodic advertising states, and improves input validation to prevent NULL pointer accesses.

Adds printing of advertising and periodic advertising states in the shell command output.
Introduces new pointer validations and state assignments in bt_le_ext_adv_get_info.
Updates header file with new state enums and refined documentation.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
subsys/bluetooth/host/shell/bt.c	Prints new extended and periodic advertising state information.
subsys/bluetooth/host/adv.c	Adds NULL checks, state assignments, and initializes info struct.
include/zephyr/bluetooth/bluetooth.h	Introduces new advertising state enums and updates documentation.

include/zephyr/bluetooth/bluetooth.h

Copilot

Pull Request Overview

Adds explicit tracking and display of extended and periodic advertising states in the Bluetooth LE advertising APIs and shell.

Extend bt_le_ext_adv_info with ext_adv_state and per_adv_state enums.
Add input validation and state population in bt_le_ext_adv_get_info.
Update shell command to print the new advertising state fields.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
subsys/bluetooth/host/shell/bt.c	Print extended and periodic advertising states and conditionally show address only when advertising is started.
subsys/bluetooth/host/adv.c	Validate `adv` and `info` pointers, memset info, and set up new state fields based on flags.
include/zephyr/bluetooth/bluetooth.h	Define enums for extended and periodic advertising states; update struct and function return documentation.

Comments suppressed due to low confidence (1)

subsys/bluetooth/host/shell/bt.c:2573

[nitpick] The abbreviation "Per Adv state" may not be immediately clear; consider using "Periodic Adv state" for consistency and clarity.

shell_print(sh, "Per Adv state: %d", info.per_adv_state);

subsys/bluetooth/host/shell/bt.c

alwa-nordic · 2025-05-27T13:16:15Z

include/zephyr/bluetooth/bluetooth.h

+	/** No state */
+	BT_LE_EXT_ADV_STATE_NONE,


Is this "no state" as in an invalid value?

It's actually unused, so perhaps the starting value of the enum should be BT_LE_EXT_ADV_STATE_CREATED

Continuing my question from #90185 (comment)

Now there is only two values, 0 or 1, please use a boolean instead.

alwa-nordic · 2025-05-27T13:18:06Z

include/zephyr/bluetooth/bluetooth.h

+	/** The advertising set is temporarily paused */
+	BT_LE_EXT_ADV_STATE_PAUSED,


I think the paused state may be a confusing addition to the API. How is the paused state any different to the started state in the eyes of the API user?

Yeah, struggled with this one as well.

In the PAUSED state, we are updating the RPA, which means that printing or copying the address in this state, may not be the same address in 200 milliseconds from now.

But it all depends on whether privacy is enabled and the options used for the advertising set, so I agree that it's pretty vaguely defined, but that's because the actual state, and what it represents, is pretty complex to describe.

I'm OK with removing it and treating the PAUSED state as the ENABLED state, as that is what it is in the eyes of caller

I'm OK with removing it and treating the PAUSED state as the ENABLED state, as that is what it is in the eyes of caller

I'm for that. The paused state is supposed to be very temporary. It will be easier for application developers if they don't have to care. It's basically a workaround for HCI and just an internal detail in the "Host and Controller in one"-box.

alwa-nordic · 2025-05-27T13:19:51Z

include/zephyr/bluetooth/bluetooth.h

+	/** The advertising set has been created but not started */
+	BT_LE_EXT_ADV_STATE_CREATED,


The advertiser will go into this state after the user stops it, right? I think "stopped" may be a better name for this state rather than "created". What do you think?

Tangent thoughts: I realize that our APIs are "start" and "stop", so we should probably be consistent with that. But especially now that we do not re-enable the advertiser automatically after it creates a connection, we could use "enable" and "disable" terminology like the spec and other resources use.

Sure, that makes sense. Could rename these _ENABLED and _DISABLED

To be clear: I did not mean to demand "enabled". I'm on the fence about myself, since as I said, we also need to be consistent with our API, which is "start" and "stop".

So, "stopped" is also a viable alternative. Which do you prefer?

Prefer ENABLED and DISABLED. STOPPED sounds like more like it has actively been stopped, which isn't always the case.

alwa-nordic · 2025-05-27T13:32:08Z

include/zephyr/bluetooth/bluetooth.h

+	/** No state */
+	BT_LE_PER_ADV_STATE_NONE,


This "no state" seems different to BT_LE_EXT_ADV_STATE_NONE, since looks to be a valid state? I don't know our periodic advertiser implementation, so I'm not sure. Do you know?

Correct.

An advertising set can go from being just extended advertising (BT_LE_PER_ADV_STATE_NONE), to be configured for periodic advertising BT_LE_PER_ADV_STATE_CONFIGURED and finally have periodic advertising enabled, which is a state that isn't tied to the extended advertising state.

Copilot

Pull Request Overview

Adds explicit advertising and periodic advertising state reporting to the extended advertising API, hardens input validation, and surfaces the new states in the shell.

Extend bt_le_ext_adv_info with ext_adv_state and per_adv_state enums and fields
Harden bt_le_ext_adv_get_info() with null checks, creation checks, and populate new state fields
Update the shell’s cmd_adv_info to show the new state fields and improve the error message

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
subsys/bluetooth/host/shell/bt.c	Print extended and periodic advertising states; refine error text
subsys/bluetooth/host/adv.c	Add NULL/creation checks; set `ext_adv_state` and `per_adv_state` fields
include/zephyr/bluetooth/bluetooth.h	Define new enums and struct fields; expand Doxygen return/value docs

Comments suppressed due to low confidence (1)

subsys/bluetooth/host/adv.c:1614

Add unit tests covering the new state assignments in bt_le_ext_adv_get_info, including NULL inputs, not-created cases, enabled/disabled, and periodic states.

if (atomic_test_bit(adv->flags, BT_ADV_ENABLED)) {

subsys/bluetooth/host/adv.c

subsys/bluetooth/host/shell/bt.c

include/zephyr/bluetooth/bluetooth.h

cvinayak · 2025-06-04T09:41:17Z

include/zephyr/bluetooth/bluetooth.h

+enum bt_le_ext_adv_state {
+	/** The advertising set has been created but not enabled. */
+	BT_LE_EXT_ADV_STATE_DISABLED,
+
+	/** The advertising set is enabled. */
+	BT_LE_EXT_ADV_STATE_ENABLED,
+};


Why no BT_LE_EXT_ADV_STATE_NONE ?

#90185 (comment)

I assume you are asking why there is a none state in one but not the other enum.

The none state in the periodic advertiser case means that no configuration has been set for periodic advertising for this set. We could remove the need for the none state in both of the enums if the Host sets a default configuration for periodic advertising. But this is extra code in the Host, so we should weight up the benefits.

#90185 (comment)

An enabled boolean member in the info structure instead is better for application to check.

The enum is far more extendable if we ever want to add back a state like PAUSED if that becomes a necessity, and it's also more consistent with the per_adv_state which cannot be a boolean

Agreed. No action required from my side.

cvinayak · 2025-06-04T09:42:48Z

include/zephyr/bluetooth/bluetooth.h

+	/** Periodic advertising state. */
+	enum bt_le_per_adv_state per_adv_state;


Should this be guarded by the periodic advertising feature support? according to implementation later in this PR, the referencing to conditionally guarded to always return BT_LE_PER_ADV_STATE_NONE, instead this member is not required and application code shall fail to compile when periodic advertising is not supported.

I did consider this, and was also discussed when I added something similar w.r.t. ISO in #86092, but it was, IIRC, decided that a small increase in the struct for something that is short-lived, would be OK.

Similar to the other info structs, it's only stack allocated so it wouldn't be taking up much memory, but might run into stack overflows in some cases I guess.

Ok, as the info struct in typical use is a auto variable, this is acceptable.

subsys/bluetooth/host/adv.c

cvinayak · 2025-06-06T01:28:02Z

include/zephyr/bluetooth/bluetooth.h

+enum bt_le_ext_adv_state {
+	/** The advertising set has been created but not enabled. */
+	BT_LE_EXT_ADV_STATE_DISABLED,
+
+	/** The advertising set is enabled. */
+	BT_LE_EXT_ADV_STATE_ENABLED,
+};


#90185 (comment)

An enabled boolean member in the info structure instead is better for application to check.

cvinayak · 2025-06-06T01:32:30Z

include/zephyr/bluetooth/bluetooth.h

+	/** Periodic advertising state. */
+	enum bt_le_per_adv_state per_adv_state;


Similar to the other info structs, it's only stack allocated so it wouldn't be taking up much memory, but might run into stack overflows in some cases I guess.

Ok, as the info struct in typical use is a auto variable, this is acceptable.

My comments are resolved.

The bt_le_ext_adv_info struct has been extended to also contain the advertising and periodic advertising states. Additionally, the function verifies the input to avoid NULL pointer access, and the addr field is more properly documented. Signed-off-by: Emil Gydesen <emil.gydesen@nordicsemi.no>

sonarqubecloud · 2025-06-16T11:46:45Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Thalley force-pushed the adv_info_state branch from a91135e to c40e77e Compare May 19, 2025 22:07

Thalley marked this pull request as ready for review May 19, 2025 23:40

github-actions bot added area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth labels May 19, 2025

github-actions bot requested review from alwa-nordic, cvinayak, hermabe, jhedberg, rugeGerritsen, sjanc and theob-pro May 19, 2025 23:41

github-actions bot assigned jhedberg and alwa-nordic May 19, 2025

Thalley force-pushed the adv_info_state branch from c40e77e to e4f71e1 Compare May 25, 2025 14:02

Thalley requested a review from Copilot May 25, 2025 14:43

Copilot AI reviewed May 25, 2025

View reviewed changes

subsys/bluetooth/host/adv.c Outdated Show resolved Hide resolved

include/zephyr/bluetooth/bluetooth.h Outdated Show resolved Hide resolved

subsys/bluetooth/host/shell/bt.c Show resolved Hide resolved

Thalley force-pushed the adv_info_state branch from e4f71e1 to 6242764 Compare May 25, 2025 14:55

Thalley requested a review from Copilot May 25, 2025 14:55

Copilot AI reviewed May 25, 2025

View reviewed changes

include/zephyr/bluetooth/bluetooth.h Outdated Show resolved Hide resolved

Thalley force-pushed the adv_info_state branch from 6242764 to 828b281 Compare May 25, 2025 15:22

Thalley requested a review from Copilot May 25, 2025 15:23

Copilot AI reviewed May 25, 2025

View reviewed changes

subsys/bluetooth/host/shell/bt.c Outdated Show resolved Hide resolved

alwa-nordic reviewed May 27, 2025

View reviewed changes

Thalley requested a review from alwa-nordic May 27, 2025 13:41

Thalley force-pushed the adv_info_state branch 4 times, most recently from 8a394cb to 250a4dd Compare May 29, 2025 08:51

Thalley requested a review from Copilot May 29, 2025 08:51

Copilot AI reviewed May 29, 2025

View reviewed changes

subsys/bluetooth/host/adv.c Outdated Show resolved Hide resolved

subsys/bluetooth/host/shell/bt.c Show resolved Hide resolved

include/zephyr/bluetooth/bluetooth.h Show resolved Hide resolved

alwa-nordic previously approved these changes Jun 2, 2025

View reviewed changes

Thalley mentioned this pull request Jun 2, 2025

Modified bt_le_ext_adv_stop() to fix an issue in which BT Controller #90757

Open

cvinayak requested changes Jun 4, 2025

View reviewed changes

Thalley dismissed alwa-nordic’s stale review via f2e2174 June 4, 2025 18:53

Thalley force-pushed the adv_info_state branch from 250a4dd to f2e2174 Compare June 4, 2025 18:53

Thalley requested review from cvinayak and alwa-nordic June 4, 2025 18:53

cvinayak previously requested changes Jun 6, 2025

View reviewed changes

Thalley force-pushed the adv_info_state branch from f2e2174 to 146e000 Compare June 6, 2025 06:53

Thalley requested a review from cvinayak June 6, 2025 06:56

Thalley force-pushed the adv_info_state branch from 146e000 to 6dcd7c0 Compare June 16, 2025 11:40

jhedberg approved these changes Jun 16, 2025

View reviewed changes

alwa-nordic approved these changes Jun 17, 2025

View reviewed changes

henrikbrixandersen merged commit 6885a36 into zephyrproject-rtos:main Jun 17, 2025
29 checks passed

Thalley deleted the adv_info_state branch June 18, 2025 06:45

		/** The advertising set is temporarily paused */
		BT_LE_EXT_ADV_STATE_PAUSED,

		/** The advertising set has been created but not started */
		BT_LE_EXT_ADV_STATE_CREATED,

		/** Periodic advertising state. */
		enum bt_le_per_adv_state per_adv_state;

Bluetooth: Host: Add advertising state to bt_le_ext_adv_info #90185

Bluetooth: Host: Add advertising state to bt_le_ext_adv_info #90185

Uh oh!

Conversation

Thalley commented May 19, 2025

Uh oh!

Thalley commented May 19, 2025

Uh oh!

Thalley commented May 21, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alwa-nordic Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alwa-nordic Jun 4, 2025 •

edited

Loading