Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bluetooth: Add flag to force device name to appear in AD data #34332

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

nordicjm
Copy link
Collaborator

This adds a new flag, BT_LE_ADV_OPT_FORCE_NAME_IN_AD, which can be used to
force the Bluetooth GAP device name to appear in the advertising data rather
than the scan response data of an advert with scan response data.

Signed-off-by: Jamie McCrae jamie.mccrae@lairdconnect.com

I looked into adding a test but it seems the only way to do that is using babblesim which isn't supported on windows so I'm open to suggestions on how to create such a test and actually test it.

Copy link
Member

@jhedberg jhedberg left a comment

Choose a reason for hiding this comment

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

I guess this is alright, although I'm not sure why requiring users to manually do this if they need to have it in the ad is so bad (since that would encourage them to use the scan response data, which is the recommended option).

@Thalley
Copy link
Collaborator

Thalley commented Apr 16, 2021

@lairdjm What is the use case for this?

Sounds like the only reason to do this, is that you want to advertise while keeping scan data exclusively for something else, otherwise you could just disable scanable to achieve the same result.

@nordicjm
Copy link
Collaborator Author

nordicjm commented Apr 16, 2021

Users should be given the option. The need for this comes from a bad piece of code in an old design that scans for a device with a particular name instead of doing the correct thing and searching for the services it's advertising, but it's too late to fix that software now. You cannot disable being scannable and remaining connectable because of this code in subsys/bluetooth/host/adv.c:

	if (param->options & BT_LE_ADV_OPT_CONNECTABLE) {
		scannable = true;

The device that does the scanning does a passive scan, not an active scan, so there is no scan response data to view.

@Thalley
Copy link
Collaborator

Thalley commented Apr 16, 2021

Users should be given the option. The need for this comes from a bad piece of code in an old design that scans for a device with a particular name instead of doing the correct thing and searching for the services it's advertising, but it's too late to fix that software now. You cannot disable being scannable and remaining connectable because of this code in subsys/bluetooth/host/adv.c:

	if (param->options & BT_LE_ADV_OPT_CONNECTABLE) {
		scannable = true;

The device that does the scanning does a passive scan, not an active scan, so there is no scan response data to view.

Hmm, right, for legacy advertising that's not possible. Good point. And I suppose that the old design also doesn't do scan requests for the name, but only looks for the advertising data?

Comment on lines 520 to 528
/** Put GAP device name into advert data
*
* Requires @ref BT_LE_ADV_OPT_USE_NAME to be set. Will place the
* GAP device name into the advertising data rather than the scan
* response data.
*/
BT_LE_ADV_OPT_FORCE_NAME_IN_AD = BIT(18),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If what @jhedberg says that this is not the recommended way of doing it, I think it's worth mentioning here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wouldn't say it's not the recommended way of doing it, it's very much down to the application. If you've got a single service and want to advertise that with the device name and it all fits into the advertising data, it should go there, it doesn't actually make any sense in that scenario to put it in the scan response nor does it make any sense to have any scan response data at all as all that would do is waste battery life if it's a sensor. However, if you're advertising multiple 128-bit UUID services then that will use a lot of advert space and it would be more important to see what services were being advertised rather than the device name.

subsys/bluetooth/host/adv.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/hci_core.h Show resolved Hide resolved
This adds a new flag, BT_LE_ADV_OPT_FORCE_NAME_IN_AD, which can be used
to force the Bluetooth GAP device name to appear in the advertising
data rather than the scan response data of an advert with scan response
data.

Signed-off-by: Jamie McCrae <jamie.mccrae@lairdconnect.com>
@@ -389,6 +389,9 @@ enum {
* advertising data.
* If the GAP device name does not fit into advertising data it will be
* converted to a shortened name if possible.
* @ref BT_LE_ADV_OPT_FORCE_NAME_IN_AD can be used to force the device
Copy link
Contributor

Choose a reason for hiding this comment

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

We have have had this discussion a few times already, and the last agreement was to document how the application can do this themselves instead.
It is documented below.

Last issue here: #27692 (comment)

@jhedberg @Thalley Do we wish to revert this decision then, it keeps getting requested so makes sense to listen to it.

But then I think we should remove the below statement again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a very good question. If it indeed does get keep requested, that's a good indication that the API can be improved (e.g. by the result of this PR).

I am a bit reluctant to add this, as it is already fairly easy for the application. We even have:

	 *  The application can set the device name itself by including the
	 *  following in the advertising data.
	 *  @code
	 *  BT_DATA(BT_DATA_NAME_COMPLETE, name, strlen(name))
	 *  @endcode

In the documentation for BT_LE_ADV_OPT_USE_NAME (which I also originally missed), which is even being used in the beacon and peripheral_ots samples. Maybe there's a way we can make that even clearer somehow?

@lairdjm Given this, are you still in favor of adding this option from your PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am in favor of having my PR accepted, I prefer the option so that it can be supplied like this.

@joerchan
Copy link
Contributor

I looked into adding a test but it seems the only way to do that is using babblesim which isn't supported on windows so I'm open to suggestions on how to create such a test and actually test it.

@lairdjm Simplest would be to update the shell and do a simple test there subsys/bluetooth/shell/bt.c, you can use tests/bluetooth/shell for this. But let's first decide if this is an enhancement that we want. We have discussed this before and not added this then. The application can do this itself, and the OPT_USE_NAME is simply just for convenience sake.

@Thalley Thalley self-requested a review April 19, 2021 13:48
Thalley
Thalley previously approved these changes Apr 19, 2021
@Thalley Thalley self-requested a review April 19, 2021 13:48
@Thalley Thalley dismissed their stale review April 19, 2021 13:49

Awaiting future response from Joakim

@joerchan
Copy link
Contributor

Awaiting future response from Joakim

I'm OK with adding it, but I would like @jhedberg to agree since he was against this change previously.

@Thalley
Copy link
Collaborator

Thalley commented Apr 20, 2021

Awaiting future response from Joakim

I'm OK with adding it, but I would like @jhedberg to agree since he was against this change previously.

I guess he was just reluctant, like me, of adding an option bit that doesn't add any functionality, but rather add a new way of doing the same as was possible before. However, the option bit to set the name in the scan data can isn't any different, so I guess this is just as good as the name option bit :)

@jhedberg
Copy link
Member

I'm OK with adding it, but I would like @jhedberg to agree since he was against this change previously.

I guess he was just reluctant, like me, of adding an option bit that doesn't add any functionality, but rather add a new way of doing the same as was possible before.

That would indeed be an accurate assessment of how I feel about this :)

IIRC we added the original flag so that the stack could auto-manage and update the advertised name in case the GAP name characteristic is writable. However I see what we could have avoided quite a bit of complexity if we had just left this up to the application to sort out and do manually.

@nordicjm nordicjm deleted the adv_ad_name branch August 20, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants