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

Samples: Bluetooth: Minor changes to improve the user experience #68870

Conversation

rugeGerritsen
Copy link
Collaborator

This PR contains some minor changes which make it easier to get started with some of the Bluetooth samples.

@@ -1,11 +1,3 @@
CONFIG_BT=y
CONFIG_LOG=y
CONFIG_BT_ISO_CENTRAL=y
CONFIG_BT_SMP=y
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider adding a note to the readme on how to encrypt data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. The sample uses

#if defined(CONFIG_BT_SMP)
	iso_chan.required_sec_level = BT_SECURITY_L2,
#endif /* CONFIG_BT_SMP */

So that if SMP is enabled, it automatically encrypts the CIS

Thalley
Thalley previously approved these changes Feb 12, 2024
samples/bluetooth/central_iso/README.rst Outdated Show resolved Hide resolved
@@ -1,11 +1,3 @@
CONFIG_BT=y
CONFIG_LOG=y
CONFIG_BT_ISO_CENTRAL=y
CONFIG_BT_SMP=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed. The sample uses

#if defined(CONFIG_BT_SMP)
	iso_chan.required_sec_level = BT_SECURITY_L2,
#endif /* CONFIG_BT_SMP */

So that if SMP is enabled, it automatically encrypts the CIS

jhedberg
jhedberg previously approved these changes Feb 12, 2024
@rugeGerritsen rugeGerritsen dismissed stale reviews from jhedberg and Thalley via 5afe7b2 February 13, 2024 07:46
@rugeGerritsen rugeGerritsen force-pushed the improve_bt_sample_user_experience branch from 3351812 to 5afe7b2 Compare February 13, 2024 07:46
Thalley
Thalley previously approved these changes Feb 13, 2024
@@ -35,7 +35,7 @@ static void device_found(const bt_addr_le_t *addr, int8_t rssi, uint8_t type,
}

/* connect only to devices in close proximity */
if (rssi < -70) {
if (rssi < -50) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are some bsim tests failing because of changes similar to this. I'm looking into finding where this can be changed

@Thalley
Copy link
Collaborator

Thalley commented Feb 13, 2024

@rugeGerritsen Looks like the RSSI change is failing some of the BSIM tests, as they get -59 and your new cutoff is -50

@rugeGerritsen
Copy link
Collaborator Author

@rugeGerritsen Looks like the RSSI change is failing some of the BSIM tests, as they get -59 and your new cutoff is -50

Let me know if you know where this comes from. Otherwise I can change the limit to -60 dBm

@Thalley
Copy link
Collaborator

Thalley commented Feb 13, 2024

@rugeGerritsen Looks like the RSSI change is failing some of the BSIM tests, as they get -59 and your new cutoff is -50

Let me know if you know where this comes from. Otherwise I can change the limit to -60 dBm

@aescolar might know

@aescolar
Copy link
Member

@rugeGerritsen
For the default channel model (NtNcable), its default channel attenuation is 60dB.
So you need to lower it to say 40dB.
So for example for tests/bsim/bluetooth/ll/multiple_id/tests_scripts/multiple.sh
in this line
https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/bsim/bluetooth/ll/multiple_id/tests_scripts/multiple.sh#L21
add -argschannel -at=40
So the line looks like:
-D=2 -sim_length=4500e6 $@ -argschannel -at=40

Meaning, with -argschannel you say that the next parameters are for the channel model,
-at=40 is this channel parameter to adjust the attenuation.

@rugeGerritsen
Copy link
Collaborator Author

@rugeGerritsen For the default channel model (NtNcable), its default channel attenuation is 60dB. So you need to lower it to say 40dB. So for example for tests/bsim/bluetooth/ll/multiple_id/tests_scripts/multiple.sh in this line https://github.com/zephyrproject-rtos/zephyr/blob/main/tests/bsim/bluetooth/ll/multiple_id/tests_scripts/multiple.sh#L21 add -argschannel -at=40 So the line looks like: -D=2 -sim_length=4500e6 $@ -argschannel -at=40

Meaning, with -argschannel you say that the next parameters are for the channel model, -at=40 is this channel parameter to adjust the attenuation.

Thanks for the answer. I assumed it was something along those lines. Should I apply this to all bsim tests or would you rather like to keep it as is?

@aescolar
Copy link
Member

Should I apply this to all bsim tests

I'd do it just to the ones that are affected by this PR samples changes.
(Other tests may have their own requirements for the RSSI levels)

When the Bluetooth central samples in an open air environment
it is very likely that there are multiple devices nearby with
a received signal strength stronger than -70 dBm.

To avoid connecting to the wrong device, make the check stricter.

For tests using those samples, the NtNcable attenuation is changed
from the default 60 dBm to 40 dBm to satisfy the new requirement.

Signed-off-by: Rubin Gerritsen <[email protected]>
Some SoCs in the nRF52 series do not support encrypting and decrypting
isochronous channels packets.

To make it easier to get started with the central_iso sample,
we want to turn of SMP by default. Without this config,
the sample still serves its purpose.
Encrypting the isochronous channel is as simple as
enabling BT_SMP.

Signed-off-by: Rubin Gerritsen <[email protected]>
Extend the sample documentation for the central and peripheral
iso samples so that they become easier to get started with.

Signed-off-by: Rubin Gerritsen <[email protected]>
@fabiobaltieri
Copy link
Member

@jhedberg should these go for 3.6? Samples are kindof documentation imo

@henrikbrixandersen @MaureenHelm

@henrikbrixandersen
Copy link
Member

@jhedberg should these go for 3.6? Samples are kindof documentation imo

@henrikbrixandersen @MaureenHelm

I think it's a little late in the release cycle for non-critical changes like this. I'd suggest aiming for v3.7.0.

@fabiobaltieri fabiobaltieri added this to the v3.7.0 milestone Feb 14, 2024
@jhedberg
Copy link
Member

@jhedberg should these go for 3.6? Samples are kindof documentation imo

I'm fine waiting until 3.7.

rugeGerritsen added a commit to rugeGerritsen/sdk-zephyr that referenced this pull request Feb 19, 2024
…below -50

When the Bluetooth central samples in an open air environment
it is very likely that there are multiple devices nearby with
a received signal strength stronger than -70 dBm.

To avoid connecting to the wrong device, make the check stricter.

For tests using those samples, the NtNcable attenuation is changed
from the default 60 dBm to 40 dBm to satisfy the new requirement.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
rugeGerritsen added a commit to rugeGerritsen/sdk-zephyr that referenced this pull request Feb 19, 2024
…SMP=y

Some SoCs in the nRF52 series do not support encrypting and decrypting
isochronous channels packets.

To make it easier to get started with the central_iso sample,
we want to turn of SMP by default. Without this config,
the sample still serves its purpose.
Encrypting the isochronous channel is as simple as
enabling BT_SMP.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
rugeGerritsen added a commit to rugeGerritsen/sdk-zephyr that referenced this pull request Feb 19, 2024
Extend the sample documentation for the central and peripheral
iso samples so that they become easier to get started with.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
rlubos pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request Feb 19, 2024
…below -50

When the Bluetooth central samples in an open air environment
it is very likely that there are multiple devices nearby with
a received signal strength stronger than -70 dBm.

To avoid connecting to the wrong device, make the check stricter.

For tests using those samples, the NtNcable attenuation is changed
from the default 60 dBm to 40 dBm to satisfy the new requirement.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
rlubos pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request Feb 19, 2024
…SMP=y

Some SoCs in the nRF52 series do not support encrypting and decrypting
isochronous channels packets.

To make it easier to get started with the central_iso sample,
we want to turn of SMP by default. Without this config,
the sample still serves its purpose.
Encrypting the isochronous channel is as simple as
enabling BT_SMP.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
rlubos pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request Feb 19, 2024
Extend the sample documentation for the central and peripheral
iso samples so that they become easier to get started with.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
@aescolar aescolar merged commit 360d49a into zephyrproject-rtos:main Feb 26, 2024
24 checks passed
jfischer-no pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request Mar 8, 2024
…below -50

When the Bluetooth central samples in an open air environment
it is very likely that there are multiple devices nearby with
a received signal strength stronger than -70 dBm.

To avoid connecting to the wrong device, make the check stricter.

For tests using those samples, the NtNcable attenuation is changed
from the default 60 dBm to 40 dBm to satisfy the new requirement.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
(cherry picked from commit 2c8d14a)
jfischer-no pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request Mar 8, 2024
…SMP=y

Some SoCs in the nRF52 series do not support encrypting and decrypting
isochronous channels packets.

To make it easier to get started with the central_iso sample,
we want to turn of SMP by default. Without this config,
the sample still serves its purpose.
Encrypting the isochronous channel is as simple as
enabling BT_SMP.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
(cherry picked from commit b1980ed)
jfischer-no pushed a commit to nrfconnect/sdk-zephyr that referenced this pull request Mar 8, 2024
Extend the sample documentation for the central and peripheral
iso samples so that they become easier to get started with.

Upstream PR: zephyrproject-rtos/zephyr#68870

Signed-off-by: Rubin Gerritsen <[email protected]>
(cherry picked from commit eb59f8d)
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.

8 participants