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: encrypt throughput sample #12265

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

martintv
Copy link
Contributor

@martintv martintv commented Sep 7, 2023

By the same logic we used when we decided to encrypt the llpm sample in #11720
we should also encrypt throughput sample. We should demonstrate the throughput we can get, with encryption on.

And conenct with 7.5ms interval to speed up encryption. The sample will switch to the interval selected by console (or 400ms if nothing is selected) before doing the throgputmeasurment.

Currently failing sample nightly test run, so that needs to be looked into before mergeing. https://jenkins-ncs.nordicsemi.no/blue/organizations/jenkins/latest%2Fsub%2Ftest-fw-nrfconnect-ble_samples/detail/master/9307/pipeline/

Disconnect with BT_HCI_ERR_INSTANT_PASSED

@martintv martintv marked this pull request as ready for review September 7, 2023 10:06
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Sep 7, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Sep 7, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_samples X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

Copy link
Contributor

@KAGA164 KAGA164 left a comment

Choose a reason for hiding this comment

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

Could security be enabled through Kconfig? The default option can be that encryption is always on.

@martintv
Copy link
Contributor Author

martintv commented Sep 7, 2023

Could security be enabled through Kconfig? The default option can be that encryption is always on.

we could do that, but we didnt for the llpm sample. I think we should just enable it, without kconfig. In the spirit that security shouldn't be optional. if anyone wants to turn it off for some reason, its easy to remove the call to bt_conn_set_security, but we shouldn't encourage that imo...

but I don't have so strong feeling here, I could add Kconfig, but we shuold do the same for llpm sample then

Copy link
Contributor

@grochu grochu left a comment

Choose a reason for hiding this comment

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

@martintv
Copy link
Contributor Author

martintv commented Sep 7, 2023

Please document the update in the changelog: https://github.com/nrfconnect/sdk-nrf/blob/main/doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

had a look, its confusing, Im 100% sure I would do it wrong, can whoever was going to review it do it themselfs? techwriter probably?

@jori-nordic jori-nordic changed the title samples: bluetooth: encrypt throgputsample samples: bluetooth: encrypt throughput sample Sep 7, 2023
#define INTERVAL_MIN 0x140 /* 320 units, 400 ms */
#define INTERVAL_MAX 0x140 /* 320 units, 400 ms */
#define INTERVAL_MIN 0x6 /* 6 units, 7.5 ms, only used to setup connection */
#define INTERVAL_MAX 0x6 /* 6 units, 7.5 ms, only used to setup connection */
Copy link
Contributor

Choose a reason for hiding this comment

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

is this to get the data length update instant faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, and encrypting the link faster on initial connection.

@grochu
Copy link
Contributor

grochu commented Sep 8, 2023

had a look, its confusing, Im 100% sure I would do it wrong, can whoever was going to review it do it themselfs? techwriter probably?

I'd propose this:
":ref:ble_throughput sample:

  • Enabled encryption in the sample - the measured throughput is calculated over the encrypted data, which is how most of the Bluetooth products use this protocol."

@peknis could you please check this statement? Adding you as reviewer of this PR. Thanks
@martintv the text will depend on whether you decide to add a Kconfig option there.

Pekka says: Looks ok to me. Just add the entry to the changelog file. I will then check it once again.

@grochu grochu requested a review from peknis September 8, 2023 08:20
@martintv martintv force-pushed the encrypt_throgput branch 2 times, most recently from 2a7c617 to d9cb39d Compare September 11, 2023 07:06
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. and removed changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Sep 11, 2023
@@ -288,6 +288,10 @@ Bluetooth samples

* Updated by disabling the :kconfig:option:`CONFIG_BT_SETTINGS_CCC_LAZY_LOADING` Kconfig option as a workaround fix for the `Zephyr issue #61033`_.

* :ref:`ble_throughput` sample:

* Enabled encryption in the sample - the measured throughput is calculated over the encrypted data, which is how most of the Bluetooth products use this protocol.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Enabled encryption in the sample - the measured throughput is calculated over the encrypted data, which is how most of the Bluetooth products use this protocol.
* Enabled encryption in the sample.
The measured throughput is calculated over the encrypted data, which is how most of the Bluetooth products use this protocol.

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

And conenct with 7.5ms interval to speed up encryption.

Signed-off-by: Martin Tverdal <martin.tverdal@nordicsemi.no>
@jfischer-no jfischer-no merged commit 307e67b into nrfconnect:main Nov 8, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants