Skip to content

Conversation

@vtardy-st
Copy link
Contributor

Integration of the BLE-802.15.4 concurrent mode on stm32wba based on CubeWBA release 1.7.0
Add new sample application ble-802.15.4 echo_client concurrent mode

@github-actions
Copy link

github-actions bot commented Oct 17, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_stm32 zephyrproject-rtos/hal_stm32@55e1597 zephyrproject-rtos/hal_stm32#319 zephyrproject-rtos/hal_stm32#319/files

Additional metadata changed:

Name URL Submodules West cmds module.yml Blobs
hal_stm32 2x 🆕

DNM label due to: 1 project with PR revision, 1 project with metadata changes and 2 blob changes

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@github-actions github-actions bot added manifest manifest-hal_stm32 DNM (manifest) This PR should not be merged (controlled by action-manifest) Binary Blobs Added labels Oct 17, 2025
@hermabe hermabe removed their request for review October 17, 2025 08:23
@vtardy-st vtardy-st marked this pull request as draft October 17, 2025 09:33
@vtardy-st vtardy-st force-pushed the stm32wba_1_7_0_ble_802154_concurrent branch from 37cc906 to 17625f4 Compare October 17, 2025 10:02
@vtardy-st vtardy-st force-pushed the stm32wba_1_7_0_ble_802154_concurrent branch from 17625f4 to 6bb9f3c Compare October 17, 2025 13:56
@vtardy-st vtardy-st force-pushed the stm32wba_1_7_0_ble_802154_concurrent branch from 6bb9f3c to 1c54b5e Compare October 30, 2025 09:21
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

For commit "soc: stm32: add parameter in link_layer_register_isr() function", suggested commit header line:

-soc: stm32: add parameter in link_layer_register_isr() function
+soc: st: stm32wba: hci_if: allow forcing ISR registration

I think the 3 first commits should be squashed unless what build would fail on the 1st commit and on the 2nd commits. Suggestion commit message:

soc: st: stm32wba: hci_if: allow forcing ISR registration

Add parameter to the link_layer_register_isr() ...

Update Bluetooth hci_stm32wba.c and IEEE 802.15.4 ieee802154_stm32wba.c
drivers accordingly.

Signed-off-by: ...

Can the west manifest be updated independently (in a specific commit), or is it needed regarding changes in the 3 first commits?



void link_layer_register_isr(void)
void link_layer_register_isr(uint8_t force)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use bool instead for force and is_isr_registered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

force parameter and is_isr_registered variable types have been modified to 'bool' type

@@ -0,0 +1,61 @@
# Private config options for ble HR - Networking echo-client sample app

# Copyright (c) 2018 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add ST copyright since you also modify the content you initially started from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ST copyright has been added in modified files coming from an existing copyrighted implementation

# Copyright (c) 2018 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

mainmenu "Ble Heart Rate - Networking echo-client sample application"
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
mainmenu "Ble Heart Rate - Networking echo-client sample application"
mainmenu "BLE Heart Rate - Networking echo-client sample application"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change done

@@ -0,0 +1,68 @@
.. zephyr:code-sample:: stm32_ble_hr_802154_echo_client
:name: Ble HR - 802154 Echo client (advanced)
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
:name: Ble HR - 802154 Echo client (advanced)
:name: BLE HR - 802154 Echo client (advanced)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change done

.. zephyr:code-sample:: stm32_ble_hr_802154_echo_client
:name: Ble HR - 802154 Echo client (advanced)

Implement a Bluetooth Heart Rate - 802154 client.
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
Implement a Bluetooth Heart Rate - 802154 client.
Implement a Bluetooth |reg| Heart Rate - 802154 client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change done

Comment on lines 2 to 3
description: Test concurrent mode Ble HeartRate - IEEE802.15.4 echo client
name: Ble Heart Rate - Socket Echo Client
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
description: Test concurrent mode Ble HeartRate - IEEE802.15.4 echo client
name: Ble Heart Rate - Socket Echo Client
description: Test concurrent mode BLE Heart Rate - IEEE802.15.4 echo client
name: BLE Heart Rate - Socket Echo Client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change done


/*
* Copyright (c) 2024 Nordic Semiconductor ASA
* Copyright (c) 2015-2016 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

For all these source file, are they raw dump of existing implementation or did you start from an existing copyrighted implementation you modified. In the latter case, I think you should also add ST copyright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

app_ble.c is heritated from main.c of bluetooth peripheral hr and includes some ST changes.
ST copyright has been added and other copyrights have been kept


#include "common.h"

static bool hrf_ntf_enabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add an empty line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change done

static bool hrf_ntf_enabled;
#if defined(CONFIG_BT)
static const struct bt_data ad[] = {
BT_DATA_BYTES(BT_DATA_FLAGS, (BT_LE_AD_GENERAL | BT_LE_AD_NO_BREDR)),
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_DATA_BYTES(BT_DATA_FLAGS, (BT_LE_AD_GENERAL | BT_LE_AD_NO_BREDR)),
BT_DATA_BYTES(BT_DATA_FLAGS, BT_LE_AD_GENERAL | BT_LE_AD_NO_BREDR),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change done

#define BLINK_ONOFF K_MSEC(500)

static struct k_work_delayable blink_work;
static bool led_is_on;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer without inner indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change done

Add parameter to the link_layer_register_isr() to force
or not the link layer isr registration in case of multiple
function calls

Update Bluetooth hci_stm32wba.c driver and
IEEE 802.15.4 ieee802154_stm32wba.c driver accordingly.

Signed-off-by: Vincent Tardy <[email protected]>
Integration of the BLE-802.15.4 concurrent mode on stm32wba.

Signed-off-by: Vincent Tardy <[email protected]>
@vtardy-st vtardy-st force-pushed the stm32wba_1_7_0_ble_802154_concurrent branch from 1c54b5e to 0bf002f Compare October 30, 2025 16:43
@vtardy-st
Copy link
Contributor Author

For commit "soc: stm32: add parameter in link_layer_register_isr() function", suggested commit header line:

-soc: stm32: add parameter in link_layer_register_isr() function
+soc: st: stm32wba: hci_if: allow forcing ISR registration

I think the 3 first commits should be squashed unless what build would fail on the 1st commit and on the 2nd commits. Suggestion commit message:

soc: st: stm32wba: hci_if: allow forcing ISR registration

Add parameter to the link_layer_register_isr() ...

Update Bluetooth hci_stm32wba.c and IEEE 802.15.4 ieee802154_stm32wba.c
drivers accordingly.

Signed-off-by: ...

Can the west manifest be updated independently (in a specific commit), or is it needed regarding changes in the 3 first commits?

The first three commits have been squashed as suggested

@vtardy-st vtardy-st force-pushed the stm32wba_1_7_0_ble_802154_concurrent branch from 0bf002f to da83d2c Compare October 30, 2025 16:52
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Some of the sonarqubecloud reports are worth to be addressed IMHO. See the few comments below.

if (ret < 0) {
LOG_ERR("Failed to set TLS_SEC_TAG_LIST option (%s): %d",
data->proto, errno);
ret = -errno;
Copy link
Contributor

Choose a reason for hiding this comment

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

As noticed by sonarqubecloud, error here is discarded. return ret seems missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure that the fact that 'set TLS_SEC_TAG_LIST option' fails is critical:
If yes, I agree 'return ret' seems missing.
Else, we could just remove 'ret = -errno;'

{
/* This means that we did not receive response in time. */
struct udp_control *ctrl = CONTAINER_OF(timer, struct udp_control, rx_timer);
struct sample_data *data = (ctrl == conf.ipv4.udp.ctrl) ? &conf.ipv4 : &conf.ipv6;
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
struct sample_data *data = (ctrl == conf.ipv4.udp.ctrl) ? &conf.ipv4 : &conf.ipv6;
struct sample_data __maybe_unused *data = (ctrl == conf.ipv4.udp.ctrl) ? &conf.ipv4 : &conf.ipv6;

if (ret < 0) {
LOG_ERR("Failed to set TLS_SEC_TAG_LIST option (%s): %d",
data->proto, errno);
ret = -errno;
Copy link
Contributor

Choose a reason for hiding this comment

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

return -errno;?
Ditto at line 232?

Add sample Bluetooth Heart Rate - 802154 Echo Client

Signed-off-by: Vincent Tardy <[email protected]>
@vtardy-st vtardy-st force-pushed the stm32wba_1_7_0_ble_802154_concurrent branch from da83d2c to 615a0b8 Compare October 31, 2025 13:34
@sonarqubecloud
Copy link

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.

4 participants