Skip to content

Conversation

@cvinayak
Copy link
Contributor

@cvinayak cvinayak commented Aug 10, 2025

Update broadcaster_multiple sample to start multiple advertising sets of type legacy and extended advertising.

How they appear on-air (BabbleSim import), for 4 advertising sets:
image

@cvinayak cvinayak force-pushed the github_broadcast_multiple_with_legacy branch 9 times, most recently from 771caba to 7afe802 Compare August 11, 2025 04:29
@cvinayak cvinayak marked this pull request as ready for review August 11, 2025 05:10
@cvinayak cvinayak force-pushed the github_broadcast_multiple_with_legacy branch from 4819544 to 394c10b Compare October 11, 2025 14:35
@cvinayak cvinayak requested review from Thalley and Copilot October 11, 2025 14:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +86 to +90
const struct adv_param_config {
uint32_t options;
const struct bt_data *ad;
size_t ad_size;
} param_config[] = {
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The struct adv_param_config is defined inside the function scope and used locally. Consider moving this struct definition outside the function or to a header file to improve code organization and potential reusability.

Copilot uses AI. Check for mistakes.
* configured, and 1 advertising set is calculating the aux_offset, 2 other advertising sets and 2
* scanning instances can expire changing the ticks_current value while we are querying for
* aux_offset value.
*/
Copy link

Copilot AI Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number 2U in the macro definition lacks explanation. Consider adding a comment to explain what this number represents (e.g., '2 scanning instances' as mentioned in the comment above).

Suggested change
*/
*/
/* 2U represents the maximum number of scanning instances that can expire and change ticks_current. */

Copilot uses AI. Check for mistakes.
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Oct 11, 2025
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove overlay from name, overlay terminology is for DT (I know there are other files named that way, but it's wrong).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is followed across the net, drivers and bluetooth subsystem... and KConfig settings too refer to applications can have SoC overlay configuration that is applied to it or phrase like overlay configuration files here: https://docs.zephyrproject.org/latest/build/kconfig/setting.html

Hence, outside the scope of this PR. You may request a github issue to update rest of the repository and Kconfig settings documentation with a suitable proposal.

@cvinayak cvinayak requested a review from Thalley October 14, 2025 14:35
@cvinayak cvinayak force-pushed the github_broadcast_multiple_with_legacy branch 2 times, most recently from 427feb4 to f815c12 Compare October 14, 2025 17:16
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this file to bt_ll_sw_split.conf

cc @nordicjm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this file to bt_ll_sw_split.conf

cc @nordicjm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this file to bt_ll_sw_split-all.conf

cc @nordicjm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this file to bt_ll_sw_split-all.conf

cc @nordicjm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this file to bt_ll_sw_split.conf

cc @nordicjm

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

/* Use advertising set instance index as SID */
adv_param.sid = index;

/* Advertising set options, AD and AD array size */
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using modulo operation to cycle through configurations could be confusing when CONFIG_BT_EXT_ADV_MAX_ADV_SET is not a multiple of param_config array size. Consider adding a comment explaining this behavior or bounds checking.

Suggested change
/* Advertising set options, AD and AD array size */
/* Advertising set options, AD and AD array size */
/*
* Cycle through param_config using modulo operation.
* If CONFIG_BT_EXT_ADV_MAX_ADV_SET is greater than ARRAY_SIZE(param_config),
* configurations will repeat for additional advertising sets.
* This is intentional; update param_config or CONFIG_BT_EXT_ADV_MAX_ADV_SET
* if a one-to-one mapping is required.
*/

Copilot uses AI. Check for mistakes.
Comment on lines 3337 to 3339
LL_ASSERT_MSG((ticks_elapsed <= ticks_to_start), "%s overhead = %u (%u) us.",
__func__, HAL_TICKER_TICKS_TO_US(ticks_elapsed),
HAL_TICKER_TICKS_TO_US(ticks_to_start));
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion message format string expects 3 arguments but only provides 2 after %s. This will cause undefined behavior in the assertion message formatting.

Copilot uses AI. Check for mistakes.
@cvinayak cvinayak removed this from the v4.3.0 milestone Oct 17, 2025
@cvinayak cvinayak added this to the v4.4.0 milestone Oct 27, 2025
@cvinayak cvinayak force-pushed the github_broadcast_multiple_with_legacy branch from f815c12 to 56d6b00 Compare October 27, 2025 19:21
Enable similar Controller Kconfig features between nRF52 and
nRF5340 BabbleSIM LE Audio testing.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Tune the aux offset calculation overhead assertion to use
EVENT_OVERHEAD_START_US; this is the correct maximum
overhead causing ISR latency for start of a radio event that
occurs thereafter.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Update broadcaster_multiple sample to start multiple
advertising sets of type legacy and extended advertising.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Move Zephyr Controller Kconfigs to overlay file.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Move harness and tags to common section for observer and
broadcaster_multiple samples.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Do not use the BT_ prefix for definitions not provided by
the Bluetooth subsystem.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@cvinayak cvinayak force-pushed the github_broadcast_multiple_with_legacy branch from 56d6b00 to 1209301 Compare October 28, 2025 12:29
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Audio area: Bluetooth Controller area: Bluetooth HCI Bluetooth HCI Driver area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth area: Samples Samples area: Tests Issues related to a particular existing or missing test Enhancement Changes/Updates/Additions to existing features platform: nRF BSIM Nordic Semiconductors, nRF BabbleSim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants