Make smp and sidewalk parallel #643
Conversation
This commit prepares PAL to be used alongside other BLE instances. Signed-off-by: Konrad Derda <konrad.derda@nordicsemi.no>
update unit tests Co-authored-by: Robert Gałat <robert.galat@nordicsemi.no> Co-authored-by: Konrad Derda <konrad.derda@nordicsemi.no> Signed-off-by: Konrad Derda <konrad.derda@nordicsemi.no> Signed-off-by: Robert Gałat <robert.galat@nordicsemi.no>
remove id from kconfig, as it should not be concern for the user. collisions are prevented by using enum. Signed-off-by: Robert Gałat <robert.galat@nordicsemi.no>
RobertGalatNordic
force-pushed
the
make_smp_and_sidewalk_parallel
branch
4 times, most recently
from
1b9529d
to
5a55397
Compare
|
RobertGalatNordic
force-pushed
the
make_smp_and_sidewalk_parallel
branch
9 times, most recently
from
5e115f6
to
7e16d63
Compare
remove disabling of Sidewalk when entering to DFU mode Signed-off-by: Robert Gałat <robert.galat@nordicsemi.no>
RobertGalatNordic
force-pushed
the
make_smp_and_sidewalk_parallel
branch
from
7e16d63
to
d9da400
Compare
define new name for BLE ADV for Sidewalk services. Signed-off-by: Robert Gałat <robert.galat@nordicsemi.no>
allow to restart adv when disconnected and log connection Signed-off-by: Robert Gałat <robert.galat@nordicsemi.no>
RobertGalatNordic
force-pushed
the
make_smp_and_sidewalk_parallel
branch
from
da45118
to
d373619
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
agree, this will be moved
|
|
||
| static uint32_t bt_enable_count = 0; | ||
|
|
||
| int app_bt_enable(bt_ready_cb_t cb) |
There was a problem hiding this comment.
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 (ret < 0) { | ||
| return ret; | ||
| } | ||
| } while (ret != BT_ID_SIDEWALK); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -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(); | |||
There was a problem hiding this comment.
Why to add advertisiement deinit here?
There was a problem hiding this comment.
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.
| * @brief BT ids used for extended advertising. | ||
| * This allows to identify connections from different extended adverticements. | ||
| */ | ||
| enum BT_id_values{ |
There was a problem hiding this comment.
don't use uppercase in type names
| _BT_ID_DEFAULT = BT_ID_DEFAULT, | ||
| BT_ID_SIDEWALK, |
There was a problem hiding this comment.
| _BT_ID_DEFAULT = BT_ID_DEFAULT, | |
| BT_ID_SIDEWALK, | |
| BT_ID_SIDEWALK = BT_ID_DEFAULT +1, |
There was a problem hiding this comment.
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
| _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"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| @@ -41,12 +53,18 @@ static struct bt_gatt_cb gatt_callbacks = { .att_mtu_updated = ble_mtu_cb }; | |||
| */ | |||
| static void ble_connect_cb(struct bt_conn *conn, uint8_t err) | |||
| { | |||
| const bt_addr_le_t *bt_addr_le; | |||
| static void connected(struct bt_conn *conn, uint8_t err) | ||
| { | ||
| struct bt_conn_info cinfo; | ||
| int ec; |
There was a problem hiding this comment.
| int ec; | |
| int ret; |
There was a problem hiding this comment.
This was copied from mesh sample, as I hope it will solve one of the issues I see in Jenkins, if so, I will refactor this code
CI parameters
extends on #603
Description
JIRA ticket:
Self review