Samples: Bluetooth: Vendor Specific: support for HCI Scan Request #68659

giansta · 2024-02-06T15:39:07Z

Example of usage of HCI vendor specific Set Scan Request Reports command, Scan Request Received Event and corresponding callback to get application events.

The sample application shows usage as an alternative to extended advertisement saving precious RAM.

With sample application the RAM saving as an alternative to extended advertisement is about 1.5 kB RAM. Building more complex applications gives larger RAM usage saving, around 5 kB.

This PR depends on previous #66529

cvinayak · 2024-03-05T09:43:14Z

Tested the changes here. Controller changes already reviewed here:
#66529

Tested the sample in this PR:

Tested the sample when not using Vendor Specific feature (Using Advertising Extensions):

cvinayak · 2024-03-05T09:47:55Z

samples/bluetooth/hci_vs_scan_req/prj.conf

+CONFIG_BT_CTLR_VS_SCAN_REQ_RX=y
+CONFIG_BT_CTLR_ADVANCED_FEATURES=y
+CONFIG_BT_CTLR_TX_PWR_DYNAMIC_CONTROL=y


These should be placed in a separate overlay file, say overlay-bt_ll_sw_split.conf and have a sample.yaml file that be by CI to test building this sample (take a look at other samples that do something similar).

I pushed you change proposal. The sample app builds on my machine. I don't know why it's getting this error in CI:
FileNotFoundError: [Errno 2] No such file or directory: '_test_plan_partial.json'
When I try running on my console:
scripts/twister -c -T tests/bluetooth/init -T samples/bluetooth/hci_vs_scan_req --all --save-tests _test_plan_partial.json
I see no errors, yet not sure if it's done correctly.

cvinayak · 2024-03-05T10:43:13Z

Handle vendor specific Scan Request Received Event and provide a callback to get application events.

A sample application shows usage as an alternative to extended advertisements saving precious 5.5 kB RAM.

This PR depends on previous #66529

@giansta With the sample in this PR, with CONFIG_BT_PERIPHERAL=y and Dynamic Pwr Control disabled:

I do not see your upto ~5.5 KB saving in RAM... is your saving in your product application?

Thalley · 2024-03-12T08:16:46Z

Generally I would not be in favor of this feature.
The memory savings are not as great as claimed where my tests show less than 1 kB for enabling EXT_ADV with minimum settings, and with the additional memory usage of this, then the savings are probably less than 900B of RAM.
I think the (small) advantage of this implementation is not worth polluting the host with this vendor specific functionality.
I will not block it from being merged, but it won't get my approval unless a better argument than a small RAM saving is provided.
If it indeed should get merged, then the comments from this review should be taken into account.

Thanks, @Thalley for all your inputs.

About memory saving, in my previous reply to @jori-nordic, I gave some details about the memory savings when building for my nRF52832 target: slightly more 1.5 kB for the sample app can be considered not much, I agree; but in our application we currently save still more than 5 kB (they were more than 5.5 kB when using Zephyr 3.2, v3.6 had quite good optimizations).

In our nRF52832 based board saving 5 of 64 kB isn't negligible. We were using already great part of them, we did some good optimizations, and this is my contribution in having a new feature with almost no additional RAM to help in field activities during installation and to wake up these devices. Using VS commands looks really suitable for our use case, as @jori-nordic suggested since beginning.

Moreover, thanks to his latest suggestions, the changes in the host disappeared and everything is now only in the sample app (and in our application).

Thanks for your reply :)

Specific savings will always depend on your configuration, and for the nRF52840 that I did my tests on, the savings would be less than 1 kB using the newest main for the samples, but of course this depends on the Kconfig values as well as the application (Which I why I wanted to get any specific numbers be removed).

With everything in the sample now (thank you!), I'm no longer against this :) Given that this is a sample, and that samples should, IMO, be as simple as possible (and not defined to be used for performance experimentation), I would still suggest to remove the EXT_ADV stuff from it, so that it be much clearer what it does, and be a lot easier to read. If you do want to keep the EXT_ADV stuff, then I would suggest to modify the code so that everything related to one is in a single block (or even in a single file), and ditto for the other, although I'm still in favor of just removing everything related to EXT_ADV as this VS command is specifically for devices that does not want to use that, right?

samples/bluetooth/hci_vs_scan_req/README.rst

samples/bluetooth/hci_vs_scan_req/src/main.c

giansta · 2024-03-18T22:26:07Z

Generally I would not be in favor of this feature.
The memory savings are not as great as claimed where my tests show less than 1 kB for enabling EXT_ADV with minimum settings, and with the additional memory usage of this, then the savings are probably less than 900B of RAM.
I think the (small) advantage of this implementation is not worth polluting the host with this vendor specific functionality.
I will not block it from being merged, but it won't get my approval unless a better argument than a small RAM saving is provided.
If it indeed should get merged, then the comments from this review should be taken into account.

Thanks, @Thalley for all your inputs.
About memory saving, in my previous reply to @jori-nordic, I gave some details about the memory savings when building for my nRF52832 target: slightly more 1.5 kB for the sample app can be considered not much, I agree; but in our application we currently save still more than 5 kB (they were more than 5.5 kB when using Zephyr 3.2, v3.6 had quite good optimizations).
In our nRF52832 based board saving 5 of 64 kB isn't negligible. We were using already great part of them, we did some good optimizations, and this is my contribution in having a new feature with almost no additional RAM to help in field activities during installation and to wake up these devices. Using VS commands looks really suitable for our use case, as @jori-nordic suggested since beginning.
Moreover, thanks to his latest suggestions, the changes in the host disappeared and everything is now only in the sample app (and in our application).

Thanks for your reply :)

Specific savings will always depend on your configuration, and for the nRF52840 that I did my tests on, the savings would be less than 1 kB using the newest main for the samples, but of course this depends on the Kconfig values as well as the application (Which I why I wanted to get any specific numbers be removed).

With everything in the sample now (thank you!), I'm no longer against this :) Given that this is a sample, and that samples should, IMO, be as simple as possible (and not defined to be used for performance experimentation), I would still suggest to remove the EXT_ADV stuff from it, so that it be much clearer what it does, and be a lot easier to read. If you do want to keep the EXT_ADV stuff, then I would suggest to modify the code so that everything related to one is in a single block (or even in a single file), and ditto for the other, although I'm still in favor of just removing everything related to EXT_ADV as this VS command is specifically for devices that does not want to use that, right?

I've both removed specific values of memory saving and all code using extended adv.

giansta · 2024-03-18T22:47:00Z

The BabbleSim Tests seem to have errors somewhere else, right?

jori-nordic

No further comments. The rest of the PR is ok for me

jori-nordic · 2024-03-20T10:10:23Z

samples/bluetooth/hci_vs_scan_req/src/main.c

+/* Allow different TX power per role/connection. It requires CONFIG_BT_CTLR_TX_PWR_DYNAMIC_CONTROL.
+ * same as in samples/bluetooth/hci_pwr_ctrl/src
+ */
+static void set_tx_power(uint8_t handle_type, uint16_t handle, int8_t tx_pwr_lvl)


Does this sample really need to set the TX power?
I think you should remove it if not absolutely necessary, as to make the sample as focused and streamlined as possible.

It's necessary in our app module, not in this sample: removed.

Thalley

A few more minor requests

samples/bluetooth/hci_vs_scan_req/README.rst

samples/bluetooth/hci_vs_scan_req/overlay-bt_ll_sw_split.conf

samples/bluetooth/hci_vs_scan_req/src/main.c

jori-nordic · 2024-03-26T06:12:30Z

samples/bluetooth/hci_vs_scan_req/README.rst

+RAM usage than using extended advertising.
+This is quite important in applications in which the broadcaster role is added
+to the central role, where the RAM saving can be bigger.
+This sample implements only the broadcaster role; the peripheral role with


A last comment and then I'll approve:
The sample can be slimmed down even more, by removing the CONFIG_BT_PERIPHERAL support.

I don't see it adding anything related to this VS event in the code. Users can easily copy code from peripheral samples if they need it.

I agree with this. Since the purpose of this sample is to show the VS command, then it should do just that.

jori-nordic · 2024-03-26T06:12:38Z

samples/bluetooth/hci_vs_scan_req/prj.conf

+CONFIG_LOG=y
+CONFIG_BT_BROADCASTER=y
+CONFIG_BT_DEVICE_NAME="VS Scan Notify"
+#CONFIG_BT_PERIPHERAL=y


Thalley

A few final thoughts, but otherwise LGTM.

Thanks for your quick updates and patience :)

Thalley · 2024-03-26T07:29:00Z

samples/bluetooth/hci_vs_scan_req/overlay-bt_ll_sw_split.conf

@@ -0,0 +1 @@
+CONFIG_BT_CTLR_VS_SCAN_REQ_RX=y


Should this be moved to the prj.conf file?

Since the main purpose of the sample is to use this, it shouldn't be in an overlay IMO

It was asked above to move some configs into this overlay file. Config moved and overlay removed, if it doesn't break anything in CI.

I see. I guess you are referring to #68659 (comment)

@cvinayak Should CONFIG_BT_CTLR_VS_SCAN_REQ_RX really be in an overlay if the sample's primary purpose is to test this feature?

I was only concern if the sample was to be used with other Controllers that may decide to add the feature in which case supplying overlay files seemed fair.

I have no objections as-is to have in prj.conf, and changes can be done in the future if required.

samples/bluetooth/hci_vs_scan_req/src/main.c

jori-nordic · 2024-03-27T07:00:29Z

@cvinayak could you re-review?

giansta · 2024-04-22T10:51:13Z

No way to merge this PR?

Thalley · 2024-04-22T10:54:27Z

No way to merge this PR?

@giansta You need to rebase the PR as there is a merge conflict

Example of usage of HCI vendor specific Set Scan Request Reports command, Scan Request Received Event and corresponding callback to get application events. The sample application shows usage as an alternative to extended advertisent saving precious RAM. Signed-off-by: Giancarlo Stasi <[email protected]>

giansta · 2024-04-22T19:35:54Z

No way to merge this PR?

@giansta You need to rebase the PR as there is a merge conflict

Sorry, I was focusing on the above request for review and missed the below conflict. Rebased now.

Thalley · 2024-04-22T20:37:53Z

@cvinayak please dismiss your review or re-review this

No objections to changes in this PR.

giansta · 2024-04-26T13:32:23Z

Can I ask if something is preventing this PR to be merged? Any further action required?

zephyrbot added area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Samples Samples area: Bluetooth area: Bluetooth Controller labels Feb 6, 2024

zephyrbot requested review from alwa-nordic, asbjornsabo, carlescufi, cvinayak, hermabe, jhedberg, jori-nordic, kartben, kruithofa, nashif, sjanc, Thalley, theob-pro, thoh-ot and Vudentz February 6, 2024 15:39

zephyrbot assigned jhedberg Feb 6, 2024

giansta mentioned this pull request Feb 6, 2024

Bluetooth: Controller: Vendor Specific: Scan Request Reports for Legacy advertising #66529

Merged

giansta force-pushed the fix/scan-cb-legacy-adv-host branch 2 times, most recently from 9b32b5e to 27d559f Compare February 6, 2024 16:59

kruithofa removed their request for review February 27, 2024 10:42

Thalley removed their request for review March 1, 2024 13:48

cvinayak previously requested changes Mar 5, 2024

View reviewed changes

cvinayak self-assigned this Mar 5, 2024

giansta force-pushed the fix/scan-cb-legacy-adv-host branch from 27d559f to 0c72bb5 Compare March 5, 2024 17:59

Thalley requested changes Mar 12, 2024

View reviewed changes

giansta force-pushed the fix/scan-cb-legacy-adv-host branch 3 times, most recently from 46c9294 to 653cf1f Compare March 18, 2024 21:18

jori-nordic requested changes Mar 20, 2024

View reviewed changes

Thalley requested changes Mar 25, 2024

View reviewed changes

giansta force-pushed the fix/scan-cb-legacy-adv-host branch from 653cf1f to 869aed1 Compare March 25, 2024 21:29

jori-nordic requested changes Mar 26, 2024

View reviewed changes

Thalley requested changes Mar 26, 2024

View reviewed changes

giansta force-pushed the fix/scan-cb-legacy-adv-host branch from 869aed1 to 320b2bf Compare March 26, 2024 14:27

Thalley previously approved these changes Mar 26, 2024

View reviewed changes

jori-nordic previously approved these changes Mar 27, 2024

View reviewed changes

giansta dismissed stale reviews from jori-nordic and Thalley via f807980 April 22, 2024 19:14

giansta force-pushed the fix/scan-cb-legacy-adv-host branch from 320b2bf to f807980 Compare April 22, 2024 19:14

Thalley approved these changes Apr 22, 2024

View reviewed changes

jori-nordic approved these changes Apr 23, 2024

View reviewed changes

jhedberg approved these changes Apr 26, 2024

View reviewed changes

aescolar merged commit f3fc534 into zephyrproject-rtos:main Apr 26, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Samples: Bluetooth: Vendor Specific: support for HCI Scan Request #68659

Samples: Bluetooth: Vendor Specific: support for HCI Scan Request #68659

giansta commented Feb 6, 2024 •

edited

Loading

cvinayak commented Mar 5, 2024 •

edited

Loading

cvinayak Mar 5, 2024

giansta Mar 5, 2024

cvinayak commented Mar 5, 2024

Thalley commented Mar 12, 2024

giansta commented Mar 18, 2024

giansta commented Mar 18, 2024

jori-nordic left a comment

jori-nordic Mar 20, 2024

giansta Mar 25, 2024

Thalley left a comment

jori-nordic Mar 26, 2024

Thalley Mar 26, 2024

giansta Mar 26, 2024

jori-nordic Mar 26, 2024

giansta Mar 26, 2024

Thalley left a comment

Thalley Mar 26, 2024

giansta Mar 26, 2024

Thalley Mar 26, 2024

cvinayak Apr 23, 2024

jori-nordic commented Mar 27, 2024

giansta commented Apr 22, 2024

Thalley commented Apr 22, 2024

giansta commented Apr 22, 2024

Thalley commented Apr 22, 2024

giansta commented Apr 26, 2024

		@@ -0,0 +1 @@
		CONFIG_BT_CTLR_VS_SCAN_REQ_RX=y

Samples: Bluetooth: Vendor Specific: support for HCI Scan Request #68659

Samples: Bluetooth: Vendor Specific: support for HCI Scan Request #68659

Conversation

giansta commented Feb 6, 2024 • edited Loading

cvinayak commented Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvinayak commented Mar 5, 2024

Thalley commented Mar 12, 2024

giansta commented Mar 18, 2024

giansta commented Mar 18, 2024

jori-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley left a 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 left a 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

jori-nordic commented Mar 27, 2024

giansta commented Apr 22, 2024

Thalley commented Apr 22, 2024

giansta commented Apr 22, 2024

Thalley commented Apr 22, 2024

giansta commented Apr 26, 2024

giansta commented Feb 6, 2024 •

edited

Loading

cvinayak commented Mar 5, 2024 •

edited

Loading