zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.48k stars 6.41k forks source link

Samples: iso_broadcast can not work properly unless some extra configuration flags #41635

Closed kaiser-ren closed 2 years ago

kaiser-ren commented 2 years ago

Describe the bug If user build this sample for nrf52840, the sample can not work properly.

After adding more configuration flags in prj.conf, the sample can broadcast BIGinfo and BIS.

Need to add these flag in separate Board folder or prj.conf

The flags CONF_BT_CTLR_ADV_EXT CONF_BT_CTLR_ADV_PERIODIC CONF_BT_CTLR_ADV_ISO

Thalley commented 2 years ago

This isn't really a bug, as those are "just" board specific configurations. Not sure what the correct label is though.

@cvinayak 1) Do we have a list of nrf boards that support extended advertising? 2) Do we have a list of nrf boards that support broadcast ISO?

cvinayak commented 2 years ago

All nRF52 and nRF53 Series SoCs support Extended Advertising, Broadcast ISO and Synchronized Receiver. The Kconfig are not implicitly/default enabled until the implementation reach acceptable maturity (I see bug during my development/testing and most implementation is still work in progress). Users are open to explicitly enable the Kconfig(s). That said, the Kconfigs are accordingly enabled in tests only for CI and regression coverage.

Thalley commented 2 years ago

So you would argue that we perhaps shouldn't add the suggested Kconfigs to the specfic board files for samples @cvinayak ? Or did I misunderstand your comment?

carlescufi commented 2 years ago

This isn't really a bug, as those are "just" board specific configurations. Not sure what the correct label is though.

I agree, but given that we anyway have plans to enable those features in the built-in controller, and users tend to see Bluetooth as whole, we might as well leave it as a low-prio bug that we close in the near future.

Thalley commented 2 years ago

This isn't really a bug, as those are "just" board specific configurations. Not sure what the correct label is though.

I agree, but given that we anyway have plans to enable those features in the built-in controller, and users tend to see Bluetooth as whole, we might as well leave it as a low-prio bug that we close in the near future.

What about marking it as a enhancement instead?

cvinayak commented 2 years ago

So you would argue that we perhaps shouldn't add the suggested Kconfigs to the specfic board files for samples @cvinayak ? Or did I misunderstand your comment?

We shouldn't add the suggested Kconfigs to specific boards, not yet.

Thalley commented 2 years ago

So you would argue that we perhaps shouldn't add the suggested Kconfigs to the specfic board files for samples @cvinayak ? Or did I misunderstand your comment?

We shouldn't add the suggested Kconfigs to specific boards, not yet.

What would be the criteria for when then? If it waits until they are accepted maturity, wouldn't we just enable the common config (e.g. CONFIG_BT_EXT_ADV)?

cvinayak commented 2 years ago

Passing conformance tests is the criteria to remove EXPERIMENTAL tag and to enable as default. Host and Controller are separate components and yes, host CONFIG_BT_EXT_ADV could implicitly enable the CONFIG_BT_CTLR_ADV_EXT when the Controller component is conformant.

Until the implementation is conformant, users should explicitly be aware of using the feature, hence let users enable it in there application conf file.

(question of the ADV_EXT name will arise, Advertising Extensions Feature in Controller is considered the parent of Extended Advertising (when enabling BT_BROADCASTER) and Extended Scanning (when enabling BT_OBSERVER).

Thalley commented 2 years ago

Passing conformance tests is the criteria to remove EXPERIMENTAL tag and to enable as default. I thought EXPERIMENTAL referred to the API stability, and not so much the conformance test :o (but that's outside the discussion of this issue).

So you suggest that we close this issue, as we won't add the experimental controller configs to the specific board config files?

kaiser-ren commented 2 years ago

If you decide to close this issue, I respect the decision. However, this is sample code, not internal test code, it means that everybody can set up and run it. If the current configuration can't make the board run properly, please leave something in readme of this sample to let the user know that how to meet the criteria to run this sample manually, otherwise, they will be frustrated.

Thalley commented 2 years ago

If you decide to close this issue, I respect the decision. However, this is sample code, not internal test code, it means that everybody can set up and run it. If the current configuration can't make the board run properly, please leave something in readme of this sample to let the user know that how to meet the criteria to run this sample manually, otherwise, they will be frustrated.

I think updating the README might be the better choice here. Even if we add the suggested changed to specific board files, that will only "solve" the issue for those specific boards.

kaiser-ren commented 2 years ago

A reminder, how to handle this?

Thalley commented 2 years ago

I think the best course of action is to update the README file in the affected samples.

kaiser-ren commented 2 years ago

I think the best course of action is to update the README file in the affected samples.

Great. What are your opinions? @cvinayak , @carlescufi