Bluetooth: tester: Add support for BTP_GAP_SET_POWERED command

[zhaynxp]

Fill BTP_GAP_SET_POWERED command to support power on/off BLE controller independently

Fixes: #67346

Signed-off-by: Ying Zhang [email protected]

[github-actions]

Hello @zhaynxp, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[zhaynxp]

The issue this PR solves is #67346 , please help to review, thanks!

[Thalley]

@zhaynxp you can link a PR to an issue using one of the GH keywords in your PR description: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue

[zhaynxp]

@Thalley , thanks for your comments, have linked issue to this PR.
BTW, there is compile error in this PR workflow checking, said undeclared function 'bt_conn_cb_unregister', actually this function was created by my nxp colleague in PR #64599 , the feature developed in this PR was based on fixes in #64599, otherwise BLE connection can't be established after bt_disable --> bt_enable ,
so need below PR checked in first, could you please help to review both PR, and let us know if any action needed?
#64599
The bt_disable --> bt_enable --> bt_disable repeat test is mandatory to us no matter with bt-shell or bt-tester, and currently block our testing, so please help to review and merge these PRs ASAP. Thanks advance!

[Thalley]

@Thalley , thanks for your comments, have linked issue to this PR.

Hi @zhaynxp - It is not linked correctly. You have to add something like closes https://github.com/zephyrproject-rtos/zephyr/issues/67346 or fixes https://github.com/zephyrproject-rtos/zephyr/issues/67346 to properly link it

[sjanc]

Hi,

I don't think zephyr should implement this command, it is meant for systems that can disable controller only (ie. Linux) without shutting down whole host/apps stack. And you also disable already registered services on power off.

If you need to disable bluetooth in zephyr I think correct way would be to implement service unregister functions and than BTP user can unregister services (including GAP which could disable bluetooth subsystem). Just note that unregister functions would have to properly cleanup things as current implementation only have stubs for services unregister (as we don't really use it in autopts for now)

[zhaynxp]

@Thalley , thanks for your comments, have linked issue to this PR.

Hi @zhaynxp - It is not linked correctly. You have to add something like closes https://github.com/zephyrproject-rtos/zephyr/issues/67346 or fixes https://github.com/zephyrproject-rtos/zephyr/issues/67346 to properly link it

Hi @Thalley , thanks! I've submitted new commit message to link issue, please help check and review. Thanks!

[zhaynxp]

Hi,

I don't think zephyr should implement this command, it is meant for systems that can disable controller only (ie. Linux) without shutting down whole host/apps stack. And you also disable already registered services on power off.

If you need to disable bluetooth in zephyr I think correct way would be to implement service unregister functions and than BTP user can unregister services (including GAP which could disable bluetooth subsystem). Just note that unregister functions would have to properly cleanup things as current implementation only have stubs for services unregister (as we don't really use it in autopts for now)

Hi @sjanc , thanks a lot for your suggestion, we'd deprecated the BTP_GAP_SET_POWERED command, and use Unregister GAP Service command(00 04 ff 01 00 01) instead to shutdown whole Bluetooth Subsystem, it will act as main switch to power off BT controller and clean up app+host stack layer resources, and will call unregister_all_services_except_gap to unregister all other services such as GATT, L2CAP.. as you know, user/customer may registered lots of services during use, when they want to shutdown Bluetooth, it's hard/impossible to unregister the services one by one manually, so a main switch is convenient and important we think, and we made it all cleaned with one Unregister GAP Service command. Next time, user need to enter Register GAP Service to power up/enable Bluetooth again to use. Please help to review the new solution and commit, thank you very much!

[zhaynxp]

Dear Reviewers,

Since this PR is on top of PR #64599 which completed bsim test recently and ready to merge soon if no accident, want to check what's your opinion on this PR? Is it ok to merge after PR #64599 merged and all checks passed? We have a tight schedule to upstream these PRs, so please kindly share your views and let us know any pending items on our side, thanks a lot!

[Thalley]

Some comments, but ideally @sjanc is the one that really needs to look at this

[zhaynxp]

Dear Reviewers,

Would you please help to review solution in this PR? Since currently no unregister services will be called by autopts or other vendor, also obviously bt_tester lacks such an interface to call bt_disable , so guess this should be fine. Kindly help to review it again, many thanks!

[Thalley]

Dear Reviewers,

Would you please help to review solution in this PR? Since currently no unregister services will be called by autopts or other vendor, also obviously bt_tester lacks such an interface to call bt_disable , so guess this should be fine. Kindly help to review it again, many thanks!

Please be patient :) I don't have any significant change requests, but @sjanc is effectively the maintainer of the BT tester, so I'd like to wait for him to provide a review.

[sjanc]

Hi,

sorry for late response, BTP protocol doesn't allow for implicit unregistering of services

Since Zephyr's host doesn't really allow for shutdown, but rather full re-init, my suggestion would be to implement this per service. As GAP profile is kinda special it may for example return error as long as any other service is stil registered. And than that specifics can be handled in test system that is using tester application.

Other option could be extending BTP to allow for this behavior (eg a command in core service) but I don't have clear idea on how such command should look like (probably should allow for some customization to accommodate for various stacks)

[zhaynxp]

Hi,

sorry for late response, BTP protocol doesn't allow for implicit unregistering of services

Since Zephyr's host doesn't really allow for shutdown, but rather full re-init, my suggestion would be to implement this per service. As GAP profile is kinda special it may for example return error as long as any other service is stil registered. And than that specifics can be handled in test system that is using tester application.

Other option could be extending BTP to allow for this behavior (eg a command in core service) but I don't have clear idea on how such command should look like (probably should allow for some customization to accommodate for various stacks)

Hi @sjanc , thanks for your reply. so you mean should not unregister other services inside unregister GAP service? and it should be the test system or user who use bt_tester themselves to unregister the registered services one by one, right?
Also is it ok to call bt_disable inside unregister GAP service? as introduced in issue #67346 , we need an interface to call the bt_disable to shutdown Bluetooth subsystem, similar as the bt disable cmd in bt_shell app.
Could you please help to double confirm? Thanks a lot!

[zhaynxp]

Hi @sjanc ,

As introduced in #67346 , our demand is to have an interface to call bt_disable in bt_tester so as to power off BLE controller during COEX test, and can work normally after power on the controller again. The reason why we clear app resource and unregister services in this PR was because there exists unable to connect issue with bt_tester after power on controller again.
As you know, this topic is pending for a long time, we reviewed this case and have few possible proposals as below, could you please help to review and share your opinion ASAP? Thanks a lot！
a. use below BTP_GAP_RESET command to call bt_disable + bt_enable to achieve restart BLE controller feature

advantages of this way is : reset controller with only 1 cmd + no unregister services and clear app resource needed, the already registered services and command handlers can keep running
b. use below BTP_GAP_SET_POWERED command to power on/off controller, if the cmd parameter Powered is 1 then call bt_enable to power on controller, else call bt_disable to power off controller

advantages of this way is : no unregister services and clear app resource needed+ can disable/enable controller separately
c. as submitted in this PR, or the comment last week, use unregister GAP service to call bt_disable
d. no way to call bt_disable, won't support this, and share the reason
Could you please let us know your preference explicitly so we can proceed this ticket accordingly? Thanks again!

[sjanc]

Hi,

Okay, so I thought about this, double checked APIs and discussed this with with Emil.
Conclusion was that it should be possible to implement 'Set powered' command by simply mapping this to bt_disable() and bt_enable() API.

It is not ideal but best we can get regarding BTP and what Zephyr offers. Just make sure to properly toggle BTP_GAP_SETTINGS_POWERED BTP setting when doing so (via BTP_GAP_EV_NEW_SETTINGS event)

[Thalley]

Hi,

Okay, so I thought about this, double checked APIs and discussed this with with Emil. Conclusion was that it should be possible to implement 'Set powered' command by simply mapping this to bt_disable() and bt_enable() API.

It is not ideal but best we can get regarding BTP and what Zephyr offers. Just make sure to properly toggle BTP_GAP_SETTINGS_POWERED BTP setting when doing so (via BTP_GAP_EV_NEW_SETTINGS event)

It is, however, very important to note that bt_disable() followed by a bt_enable() does not mimic a reboot. At best it mimics this in the controller, but there are data in the host that is not cleared (most notably registered services, callbacks, etc. (anything that can be changed before calling bt_enable())).

[zhaynxp]

Hi @sjanc @Thalley ,

Thank you very much for the confirmation, agree that currently the set powered command seems to be the most appropriate for the power on/off controller scenario. Well understand it's not system/BT reboot, but can help controller reboot without touch app layer resources. Have submitted the PR for review, kindly pls help to review and approve, let us know any comments.
Many thanks!

[sjanc]

enable set powered command bit in supported_commands(), otherwise looks OK to me

[zhaynxp]

enable set powered command bit in supported_commands(), otherwise looks OK to me

thanks sjanc for the reminder ! Have corrected this

[github-actions]

Hi @zhaynxp!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁