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

Make smp and sidewalk parallel #643

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Kconfig.dependencies
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,22 @@ config SIDEWALK_BLE
imply BT_CTLR_CONN_RSSI
imply BT_CTLR_TX_PWR_DYNAMIC_CONTROL
imply BT_CTLR_ADVANCED_FEATURES
imply BT_EXT_ADV
help
Sidewalk Bluetooth Low Energy (BLE) module

config SIDEWALK_BLE_NAME
string "BLE name adverticed for Sidewalk"
default "SID_APP"

config BT_ID_MAX
default 3 if SIDEWALK_DFU
default 2

config BT_APP_IFC
bool "Enable BT app wrapper"
default y if SIDEWALK_BLE

config SIDEWALK_ASSERT
bool
default SIDEWALK
Expand Down
8 changes: 8 additions & 0 deletions samples/sid_end_device/Kconfig.defconfig
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ config BT_PERIPHERAL_PREF_LATENCY
config BT_PERIPHERAL_PREF_TIMEOUT
default 400

config BT_EXT_ADV_MAX_ADV_SET
default 3 if SIDEWALK_DFU
default 2

config BT_MAX_CONN
default 2 if SIDEWALK_DFU
default 1

config NVS_LOOKUP_CACHE_SIZE
default 256 if NVS

Expand Down
4 changes: 0 additions & 4 deletions samples/sid_end_device/src/hello/app.c
Original file line number Diff line number Diff line change
Expand Up @@ -269,13 +269,9 @@ static void app_btn_dfu_state(uint32_t unused)
ARG_UNUSED(unused);
static bool go_to_dfu_state = true;
if (go_to_dfu_state) {
sidewalk_event_send(sidewalk_event_exit, NULL, NULL);
sidewalk_event_send(app_event_enter_dfu_mode, NULL, NULL);
application_state_working(&global_state_notifier, false);
} else {
sidewalk_event_send(app_event_exit_dfu_mode, NULL, NULL);
sidewalk_event_send(sidewalk_event_autostart, NULL, NULL);
application_state_working(&global_state_notifier, true);
}

go_to_dfu_state = !go_to_dfu_state;
Expand Down
2 changes: 0 additions & 2 deletions samples/sid_end_device/src/sensor_monitoring/app.c
Original file line number Diff line number Diff line change
Expand Up @@ -182,11 +182,9 @@ static void app_btn_dfu_state(uint32_t unused)
ARG_UNUSED(unused);
static bool go_to_dfu_state = true;
if (go_to_dfu_state) {
sidewalk_event_send(sidewalk_event_exit, NULL, NULL);
sidewalk_event_send(app_event_enter_dfu_mode, NULL, NULL);
} else {
sidewalk_event_send(app_event_exit_dfu_mode, NULL, NULL);
sidewalk_event_send(sidewalk_event_autostart, NULL, NULL);
}

go_to_dfu_state = !go_to_dfu_state;
Expand Down
5 changes: 4 additions & 1 deletion samples/sid_end_device/sysbuild/ipc_radio/prj.conf
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,14 @@ CONFIG_BT_HCI_RAW=y
CONFIG_BT_CTLR_ASSERT_HANDLER=y
CONFIG_BT_PERIPHERAL=y
CONFIG_BT_CENTRAL=n
CONFIG_BT_MAX_CONN=1
CONFIG_BT_MAX_CONN=2
CONFIG_BT_BUF_ACL_RX_SIZE=502
CONFIG_BT_BUF_ACL_TX_SIZE=251
CONFIG_BT_CTLR_DATA_LENGTH_MAX=251

CONFIG_BT_CTLR_ADV_SET=3
CONFIG_BT_CTLR_ADV_EXT=y

# IPC
CONFIG_IPC_SERVICE=y
CONFIG_MBOX=y
Expand Down
2 changes: 1 addition & 1 deletion scripts/ci/license.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ licenses:
- sidewalk.*/subsys/config/common/src/.*(c|h)$
- sidewalk.*/subsys/hal(/.*)+h$
- sidewalk.*/subsys/hal/src/memory.c$
- sidewalk.*/subsys/sal/common/.*(c|h)$
- sidewalk.*/subsys/sal/common/.*/sid_.*(c|h)$
- sidewalk.*/subsys/semtech/include/semtech_radio_ifc.h$
- sidewalk.*/tools/.*
- sidewalk.*/tests/validation/storage_kv/.*(c|h)$
Expand Down
2 changes: 1 addition & 1 deletion subsys/config/common/src/app_ble_config.c
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ static const sid_ble_cfg_gatt_profile_t ble_profile[] = {
};

static const sid_ble_config_t ble_cfg = {
.name = CONFIG_BT_DEVICE_NAME,
.name = CONFIG_SIDEWALK_BLE_NAME,
.mtu = CONFIG_BT_L2CAP_TX_MTU,
.is_adv_available = true,
.mac_addr_type = SID_BLE_CFG_MAC_ADDRESS_TYPE_STATIC_RANDOM,
Expand Down
2 changes: 2 additions & 0 deletions subsys/sal/common/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ zephyr_include_directories(sid_pal_ifc)
zephyr_include_directories(sid_time_ops)

add_subdirectory_ifdef(CONFIG_SIDEWALK_ON_DEV_CERT sid_on_dev_cert)

zephyr_library_sources(sid_ifc/bt_app_callbacks.c)
43 changes: 43 additions & 0 deletions subsys/sal/common/sid_ifc/bt_app_callbacks.c
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not a good place dor this file (in this directory there are files to be changed when Sidewalk libraries are updates) - move to sid_pal/include?

Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/
#include <bt_app_callbacks.h>
#include <zephyr/bluetooth/bluetooth.h>
#include <stdbool.h>
#include <zephyr/kernel.h>

static uint32_t bt_enable_count = 0;

int app_bt_enable(bt_ready_cb_t cb)
Copy link
Contributor

Choose a reason for hiding this comment

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

In genreal, I would recommend to keep the naming convention of Sidewalk module. The app_ prefix is used for sample configuration like app_ble_config. For ble code we used sid_ble_ prefixes (sid_ble_advert, sid_ble_connectopn)

{
if (bt_enable_count == 0) {
int ret = bt_enable(cb);
if (ret == 0) {
bt_enable_count++;
}
return ret;
}

bt_enable_count++;
if (cb) {
cb(0);
}
return 0;
}

int app_bt_disable()
{
if (bt_enable_count <= 0) {
bt_enable_count = 0;
return -EALREADY;
}
if (bt_enable_count == 1) {
bt_enable_count = 0;
return bt_disable();
} else {
bt_enable_count--;
return 0;
}
}
50 changes: 50 additions & 0 deletions subsys/sal/common/sid_ifc/bt_app_callbacks.h
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not a good place dor this file (in this directory there are files to be changed when Sidewalk libraries are updates) - move to sid_pal/include?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree, this will be moved

Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright (c) 2024 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
*/

#ifndef SID_PAL_APP_CALLBACKS_H
#define SID_PAL_APP_CALLBACKS_H

#include <stdbool.h>
#include <zephyr/bluetooth/bluetooth.h>

#if defined (CONFIG_BT_APP_IFC)

/**
* @brief Wrapper for @bt_enable, with reference tracking.
* Real @bt_enable is called only of first call to app_bt_enable
*
* @param cb callback passed to @bt_enable
* @return int result from @bt_enable call or 0 if called multiple times
*/
int app_bt_enable(bt_ready_cb_t cb);


/**
* @brief Wrapper for @bt_disable.
* This function removes internal reference.
* If the internal reference counter shows 0, real @bt_disable is called
*
* @return int result from @bt_disable or 0 if app_bt_enable has been called more than app_bt_disable
*/
int app_bt_disable();


/**
* @brief BT ids used for extended advertising.
* This allows to identify connections from different extended adverticements.
*/
enum BT_id_values{
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use uppercase in type names

_BT_ID_DEFAULT = BT_ID_DEFAULT,
BT_ID_SIDEWALK,
Comment on lines +40 to +41
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
_BT_ID_DEFAULT = BT_ID_DEFAULT,
BT_ID_SIDEWALK,
BT_ID_SIDEWALK = BT_ID_DEFAULT +1,

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 do not think it is required, enums automatically build values as previous +1.

The Ids are described in documentation for BLE as just an id in array of advertiser sets, so it is guaranteed by the API documentation, that they will be starting from 0 (default), and any new ID is just last one +1 up to the total defined size of the advertising array

#if defined (CONFIG_SIDEWALK_DFU)
BT_ID_SMP_DFU,
#endif
_BT_ID_MAX
};

BUILD_ASSERT(_BT_ID_MAX <= CONFIG_BT_ID_MAX, "Too many BT Ids! increase CONFIG_BT_ID_MAX, to match _BT_ID_MAX");
Comment on lines +45 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea to get the fast feedback to developer during build, but I think the BLE module should assert when you try to use too many ids.
My recommendation is to remove this, to keep code simple, and easier to maintain

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 think if we removed this build assert, we would find out we do not have enough BT_ID_MAX in runtime, when we try to get new ID, and IMO that is a little too late.

#endif
#endif
14 changes: 14 additions & 0 deletions subsys/sal/sid_pal/include/sid_ble_advert.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@

#include <stdint.h>

/**
* @brief Initialize Bluetooth Advertising.
*
* @return Zero on success or (negative) error code on failure.
*/
int sid_ble_advert_init(void);

/**
* @brief Deinitialize Bluetooth Advertising.
*
* @return Zero on success or (negative) error code on failure.
*/
int sid_ble_advert_deinit(void);

/**
* @brief Start Bluetooth advertising.
*
Expand Down
42 changes: 39 additions & 3 deletions subsys/sal/sid_pal/src/sid_ble_adapter.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <zephyr/bluetooth/hci.h>
#include <zephyr/bluetooth/bluetooth.h>
#include <bt_app_callbacks.h>

#include <zephyr/bluetooth/gatt.h>
#include <zephyr/bluetooth/uuid.h>
Expand Down Expand Up @@ -213,14 +214,35 @@ static sid_error_t ble_adapter_set_tx_pwr(int8_t tx_power)
return SID_ERROR_NONE;
}

static int create_ble_id(void)
{
int ret;
size_t count = 0;

/* Check if Bluetooth identites weren't already created. */
bt_id_get(NULL, &count);
if (count > BT_ID_SIDEWALK) {
return 0;
}

do {
ret = bt_id_create(NULL, NULL);
if (ret < 0) {
return ret;
}
} while (ret != BT_ID_SIDEWALK);
Copy link
Contributor

Choose a reason for hiding this comment

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

risk of inifinite loop?

Why isn't the id saved in some static struct and compared in callback? I see something like this is done in nrodic_dfu and some sources in ncs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will not get an infinite loop here, because in previous call, we checked if the advertising sets were loaded from settings, if yes, than we do not go into this loop.

If we end up in the loop it means, that there are some ID that we still expect, so we call the function to get them.
We are guaranteed to get them one by one starting from 1.
if we run out of IDs BLE will return negative error, and we break the loop.

I think this can be rewritten to better express the behaviour


return 0;
}

static sid_error_t ble_adapter_init(const sid_ble_config_t *cfg)
{
LOG_DBG("Sidewalk -> BLE");
ARG_UNUSED(cfg);

LOG_INF("Enable BT");
int err_code;
err_code = bt_enable(NULL);
err_code = app_bt_enable(NULL);
switch (err_code) {
case -EALREADY:
case 0:
Expand All @@ -231,6 +253,18 @@ static sid_error_t ble_adapter_init(const sid_ble_config_t *cfg)
return SID_ERROR_GENERIC;
}

err_code = create_ble_id();
if (err_code) {
LOG_ERR("BT ID init failed (err: %d)", err_code);
return SID_ERROR_GENERIC;
}

err_code = sid_ble_advert_init();
if (err_code) {
LOG_ERR("BT Advertisement failed (err: %d)", err_code);
return SID_ERROR_GENERIC;
}

sid_ble_conn_init();

return SID_ERROR_NONE;
Expand Down Expand Up @@ -404,8 +438,10 @@ static sid_error_t ble_adapter_deinit(void)
{
LOG_DBG("Sidewalk -> BLE");
sid_ble_conn_deinit();

int err = bt_disable();
sid_ble_advert_deinit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why to add advertisiement deinit 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.

Because we can free the advertising from BLE stack. Maybe for now it is not very useful, but I figured out if we asked for some resources, it would be nice to give them back when we are done working with them.

bt_id_delete(BT_ID_SIDEWALK);
bt_id_reset(BT_ID_SIDEWALK, NULL, NULL);
int err = app_bt_disable();

if (err) {
LOG_ERR("BT disable failed (error %d)", err);
Expand Down
Loading