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

pal: switch to multi-advertising BLE API #603

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

konradderda
Copy link

This commit prepares PAL to be used alongside other BLE instances.

CI parameters

Github_actions:
  #(branch, hash, pull/XXX/head)
  NRF_revision: main

  # Do not change after creating PR
  Create_NRF_PR: false
Jenkins:
  test-sdk-sidewalk: master

Description

JIRA ticket: KRKNWK-19430

Self review

  • There is no commented code.
  • There are no TODO/FIXME comments without associated issue ticket.
  • Commits are properly organized.
  • Change has been tested.
  • Tests were updated (if applicable).

@github-actions github-actions bot added the source PR changing src files label Sep 10, 2024
@konradderda
Copy link
Author

Regarding Change has been tested. tick - The device has registered and sent test message to cloud but I do not possess enough experience to test the change thoroughly.

Copy link

github-actions bot commented Sep 10, 2024

Sample diff used total
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello.ble_only RAM 1.16 KB 92.73 KB 0 B
ROM 8.25 KB 324.58 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello.ble_only.release RAM 1.16 KB 88.1 KB 0 B
ROM 7.51 KB 255.5 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello.release RAM 1.16 KB 102.88 KB 0 B
ROM 7.51 KB 331.33 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.demo RAM 1.16 KB 118.24 KB 0 B
ROM 8.28 KB 453.9 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.demo.ble_only RAM 1.16 KB 103.46 KB 0 B
ROM 8.26 KB 376.25 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.dut RAM 1.16 KB 141.99 KB 0 B
ROM 8.32 KB 519.51 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.dut.no_secure RAM 1.16 KB 141.98 KB 0 B
ROM 8.31 KB 511.82 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello RAM 1.16 KB 119.69 KB 0 B
ROM 8.26 KB 468.91 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello.ble_only RAM 1.16 KB 93.51 KB 0 B
ROM 8.27 KB 374.58 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello.ble_only.release RAM 1.16 KB 88.88 KB 0 B
ROM 7.54 KB 304.55 KB 0 B
nrf54l15dk/nrf54l15/cpuapp:sample.sidewalk.hello.release RAM 1.16 KB 103.66 KB 0 B
ROM 7.52 KB 380.37 KB 0 B
thingy53/nrf5340/cpuapp:sample.sidewalk.demo.ble_only RAM 2.07 KB 108.38 KB 0 B
ROM 3.38 KB 354.88 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.demo RAM 2.34 KB 120.34 KB 0 B
ROM 25.6 KB 476.28 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.demo.ble_only RAM 2.34 KB 105.27 KB 0 B
ROM 25.6 KB 393.91 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.dut RAM 2.34 KB 143.99 KB 0 B
ROM 25.67 KB 541.41 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.dut.no_secure RAM 2.34 KB 143.96 KB 0 B
ROM 25.66 KB 532.86 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello RAM 2.34 KB 121.57 KB 0 B
ROM 25.62 KB 491.32 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello.ble_only RAM 2.34 KB 95.1 KB 0 B
ROM 25.61 KB 392.26 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello.ble_only.release RAM 2.34 KB 90.29 KB 0 B
ROM 24.86 KB 324.27 KB 0 B
nrf52840dk/nrf52840:sample.sidewalk.hello.release RAM 2.34 KB 105.37 KB 0 B
ROM 24.88 KB 403.22 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.demo RAM 2.07 KB 113.55 KB 0 B
ROM 3.38 KB 398.84 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.demo.ble_only RAM 2.07 KB 98.62 KB 0 B
ROM 3.37 KB 315.32 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.dut RAM 2.07 KB 137.31 KB 0 B
ROM 3.43 KB 464.69 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.dut.no_secure RAM 2.07 KB 137.3 KB 0 B
ROM 3.44 KB 457.24 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello RAM 2.07 KB 115.04 KB 0 B
ROM 3.38 KB 414.13 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello.ble_only RAM 2.07 KB 88.71 KB 0 B
ROM 3.39 KB 313.92 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello.ble_only.release RAM 2.07 KB 84.06 KB 0 B
ROM 2.64 KB 243.68 KB 0 B
nrf5340dk/nrf5340/cpuapp:sample.sidewalk.hello.release RAM 2.07 KB 98.99 KB 0 B
ROM 2.62 KB 323.4 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.demo RAM 1.16 KB 117.46 KB 0 B
ROM 8.25 KB 403.9 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.demo.ble_only RAM 1.16 KB 102.69 KB 0 B
ROM 8.25 KB 326.26 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.dut RAM 1.16 KB 141.21 KB 0 B
ROM 8.32 KB 469.75 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.dut.no_secure RAM 1.16 KB 141.21 KB 0 B
ROM 8.32 KB 467.05 KB 0 B
nrf54l15dk/nrf54l15/cpuapp/ns:sample.sidewalk.hello RAM 1.16 KB 118.91 KB 0 B
ROM 8.27 KB 418.92 KB 0 B

@RobertGalatNordic
Copy link
Collaborator

RobertGalatNordic commented Sep 11, 2024

good job, but Sidewalk has also API for disabling BLE, I think that we should have some kind of global reference counter and do not disable the BLE stack if some other part of the application uses it

static sid_error_t ble_adapter_deinit(void)
{
LOG_DBG("Sidewalk -> BLE");
sid_ble_conn_deinit();
int err = bt_disable();
if (err) {
LOG_ERR("BT disable failed (error %d)", err);
return SID_ERROR_GENERIC;
}
return SID_ERROR_NONE;
}

update: maybe we should move ble init/deinit to sample in the mutliple adverising/connection use case?

Kconfig Show resolved Hide resolved
Copy link
Contributor

@ktaborowski ktaborowski left a comment

Choose a reason for hiding this comment

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

Thre is yet another place which modifies Bluetooth paramters utils/sidewalk_dfu/nordic_dfu.c
We shoould make sure it works correct (I tnik the CI tests will verify it, just make sure the correct suit passed). As a next step we can add Nordic DFU service as a second (third?) advertising set

subsys/sal/sid_pal/src/sid_ble_adapter.c Outdated Show resolved Hide resolved
@RobertGalatNordic
Copy link
Collaborator

Unit tests need to be reviewed. I made them green, but at this point I doubt if they test anything (or at list they does not clearly communicate what is actually tested)

@RobertGalatNordic RobertGalatNordic force-pushed the change_ble_adv_api branch 3 times, most recently from 681e4fe to ddde8b8 Compare November 12, 2024 12:52
@RobertGalatNordic
Copy link
Collaborator

This implementation does not introduce paralel SMP and Sidewalk connection, but lays a ground for such implementaiton by separating and filtering advertising and connections based on ID.

The parallel Sidewalk and SMP service will be made by separate PR, as it needs more sophisticated way of calling bt_disable to make sure that sidewalk does not disable BLE when it switches transport while SMP is working on DFU

This commit prepares PAL to be used alongside other BLE instances.

Signed-off-by: Konrad Derda <[email protected]>
update unit tests

Co-authored-by: Robert Gałat <[email protected]>
Co-authored-by: Konrad Derda <[email protected]>

Signed-off-by: Konrad Derda <[email protected]>
Signed-off-by: Robert Gałat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scripts source PR changing src files tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants