ST: STM32WB0: Implement BLE feature #79734

HoZHel · 2024-10-11T18:37:11Z

Provide BLE support for STM32WB0x:

Use hal_stm32 PR to provide the binary blob and required files to build a BLE controller library.
Provide STM32WB0 HCI driver

Prior building a Zephyr BLE application, user should run west blobs fetch hal_stm32 to fetch binary blob so as to provide BLE controller support.

BLOB request: #79652 (TSC Approved on 23/10/24)
hal_stm32: zephyrproject-rtos/hal_stm32#236

erwango · 2024-10-15T08:51:45Z

subsys/bluetooth/host/Kconfig

@@ -344,7 +344,7 @@ config BT_USER_DATA_LEN_UPDATE
 	  info is available in the connection info.

 config BT_AUTO_DATA_LEN_UPDATE
-	bool "Auto-initiate Data Length Update procedure"
+	bool "Auto-initiate Data Length Update procedure" if !BT_STM32WB0
 	depends on BT_DATA_LEN_UPDATE


Changes to this file should not be required, based on the changes to soc/st/stm32/stm32wb0x/Kconfig.defconfig.

As this config has prompt, it is user configurable. So, if in an application, like BT shell (available under tests/bluetooth/shell), it is activated in prj.conf then the default value will be ignored as well as changes in soc/st/stm32/stm32wb0x/Kconfig.defconfig.

Ok, I see. Then is is the case as well for bunch of these options (BT_HCI_ACL_FLOW_CONTROL for instance) and yet this is never used. Anyway, I let this to subsystem maintainers.

What's the reason these options must remain disabled for this controller? Is it because the controller doesn't support them? Does it incorrectly report support for them when querying its features over HCI? (the host shouldn't initiate such procedures if the controller reports that it doesn't support them).

Either way, having HCI driver specific references in the host Kconfig is not the right way to deal with this. What the right/best way is depends on the answers to my earlier questions.

Zephyr host starts several BLE procedures in parallel during connection (perform_auto_initiated_procedures in subsys/bluetooth/host/conn.c); however, ST controller does not support this "parallelism" and according to the ST Bluetooth stack experts, host should not initiate a new procedure before previous one is completed. In such case, ST controller returns a "controller busy" error (0x3A) which is not managed by the host and it's only reported on the console.
We may request further clarification in the future, but for the time being to avoid showing console error, I disabled CONFIG_BT_AUTO_PHY_UPDATE and CONFIG_BT_AUTO_DATA_LEN_UPDATE.

By the way, I will remove any changes in the host part added for handling special use cases to minimize the impact on any other piece of code.

@alwa-nordic @jhedberg, I was reporting the opinion of our BLE stack team people and I will try to involve them in the conversation. On the other hand, I suspect it could be a fairly long discussion. Our target was to have this PR approved before next code freeze. Is there anyway we could push this PR for approval ? or you prefer to clean the controller busy with a quirk or a
discussion with host guys ?

If we do the quirk: I suggest we do it defensive programming-style, with a delayed retry. The delay should be timeout-based, but optionally some events can reduce or skip the timeout.

@alwa-nordic, you mean to implement the quirk at driver level which should implement a delayed retry when controller busy is detected? Or you mean that host part should be changed to make a delayed retry when controller busy is returned? I would reckon that last approach would extend the host to properly handle controller busy cases.

I am fine with this PR adding an override of the default y. But only if @jhedberg also agrees. In this case, you should also consider adding BUILD_ASSERT in your driver to deny these configs completely.

Aside: I even think we should consider globally removing the default y. I think explicit is better than implicit, and especially as new features are introduced, it's safer not to automatically add extra effects. We could have snippets or a config for "recommended getting-started config", that selects "most" features and automatics, but is explicitly asked for by the application developer.

@alwa-nordic, you mean to implement the quirk at driver level which should implement a delayed retry when controller busy is detected? Or you mean that host part should be changed to make a delayed retry when controller busy is returned? I would reckon that last approach would extend the host to properly handle controller busy cases.

Either in the Host or the driver is fine by me. The driver could implement this with vendor-specific events, so it might be more performant. OTOH, in the Host, defensive programming is never a bad idea.

@alwa-nordic @jhedberg, I was reporting the opinion of our BLE stack team people and I will try to involve them in the conversation. On the other hand, I suspect it could be a fairly long discussion. Our target was to have this PR approved before next code freeze. Is there anyway we could push this PR for approval ? or you prefer to clean the controller busy with a quirk or a discussion with host guys ?

Optimally, the fix should be in the Controller. So if we could convince them, that would be great. I welcome the discussion.

zephyrbot · 2024-10-15T12:15:46Z

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

Name	Old Revision	New Revision	Diff
hal_stm32	zephyrproject-rtos/hal_stm32@`019d825`	zephyrproject-rtos/hal_stm32@`d5fb525` (`main`)	zephyrproject-rtos/[email protected]

Additional metadata changed:

Name	URL	Submodules	West cmds	`module.yml`
hal_stm32				✏

⛔ DNM label due to: 1 project with metadata changes

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

jhedberg · 2024-10-15T13:15:28Z

subsys/bluetooth/host/Kconfig

@@ -344,7 +344,7 @@ config BT_USER_DATA_LEN_UPDATE
 	  info is available in the connection info.

 config BT_AUTO_DATA_LEN_UPDATE
-	bool "Auto-initiate Data Length Update procedure"
+	bool "Auto-initiate Data Length Update procedure" if !BT_STM32WB0
 	depends on BT_DATA_LEN_UPDATE


What's the reason these options must remain disabled for this controller? Is it because the controller doesn't support them? Does it incorrectly report support for them when querying its features over HCI? (the host shouldn't initiate such procedures if the controller reports that it doesn't support them).

Either way, having HCI driver specific references in the host Kconfig is not the right way to deal with this. What the right/best way is depends on the answers to my earlier questions.

jhedberg · 2024-10-15T13:17:55Z

subsys/bluetooth/Kconfig

-	default y if !BT_CTLR && !BT_STM32_IPM && !BT_ESP32  && !BT_STM32WBA
+	default y if !BT_CTLR && !BT_STM32_IPM && !BT_ESP32 && !BT_STM32WBA && !BT_STM32WB0


This is starting to get messy. We should probably instead create a virtual option that the HCI drivers select and then here we only depend on that one virtual option. That said, do you realize that BT_CTLR is actually a generic option for describing any local (same memory & CPU) controller? I think we should work toward having all such HCI drivers enable BT_CTRL and associated options. Nordic already enables BT_CTRL for the upstream open source split link layer, as well for their downstream closed source SoftDevice controller, but no other HCI driver does this at the moment.

jhedberg · 2024-10-15T13:18:35Z

drivers/bluetooth/hci/hci_stm32wb0.c

+	static uint8_t legacy_cmd_issued = FALSE, extended_cmd_issued =	FALSE;
+	uint8_t allowed = TRUE;


Why aren't these bool?

I'll fix it.

dts/bindings/bluetooth/st,hci-stm32wb0.yaml

alwa-nordic · 2024-11-07T11:35:05Z

@erwango Is this still for v4.0.0?

erwango · 2024-11-08T08:14:15Z

@erwango Is this still for v4.0.0?

@alwa-nordic Indeed, this will rather go in v4.1.0. We identified a late license issue. This is now fixed but too late for v4.0.0

erwango · 2024-11-22T07:51:42Z

@HoZHel Would you mind rebasing ?

HoZHel · 2024-11-22T11:21:26Z

@HoZHel Would you mind rebasing ?

I'm waiting for the merge on hal_stm32 side. After that, I'll rebase and fix the SHA for west.yaml

Introduce Kconfig parameter for STM32WB0x HCI Bluetooth driver. Signed-off-by: Ali Hozhabri <[email protected]>

Add a yaml file required by STM32WB0x bluetooth HCI driver. Signed-off-by: Ali Hozhabri <[email protected]>

Add Bluetooth HCI driver for STM32WB0x series. Modify CMakeLists.txt to compile the driver based on its kconfig parameter. Signed-off-by: Ali Hozhabri <[email protected]>

Add BLE feature to STM32WB0x series at SOC level. Signed-off-by: Ali Hozhabri <[email protected]>

Dedicate RAM section on STM32WB0x for BLE part based on the number of radio tasks and device type. Signed-off-by: Ali Hozhabri <[email protected]>

…UPDATE Put the default value for BT_AUTO_PHY_UPDATE and BT_AUTO_DATA_LEN_UPDATE to "n" at SOC level since they cause "controller busy" due to starting several parallel BLE procedures during connection by "perform_auto_initiated_procedures" function. At the moment, ST controller does not support parallelism, i.e. host should not initiate a new procedure before previous one is completed. Disable CONFIG_BT_HCI_ACL_FLOW_CONTROL at SOC level. Signed-off-by: Ali Hozhabri <[email protected]>

Enable BLE feature for Nucleo-WB09KE and Nucleo-WB05KZ. Dedicate 32KB and 8KB at the end of flash memory to storage partition on Nucleo-WB09KE and Nucleo-WB05KZ respectively. Add ble tag to the both devices yaml file. Signed-off-by: Ali Hozhabri <[email protected]>

HoZHel · 2024-11-22T13:18:31Z

Rebased to resolve conflict with soc/st/stm32/stm32wb0x/soc.c and west.yaml

Modify west.yaml to update hal_stm32. Signed-off-by: Ali Hozhabri <[email protected]>

HoZHel · 2024-11-25T08:47:42Z

Updated west.yaml to have the correct SHA.

msmttchr

Lgtm

HoZHel mentioned this pull request Oct 11, 2024

Introduce STM32WB0x Binary BLOB to provide BLE controller #79652

Closed

2 tasks

erwango added area: Bluetooth platform: STM32 ST Micro STM32 manifest-hal_stm32 labels Oct 14, 2024

erwango reviewed Oct 15, 2024

View reviewed changes

HoZHel force-pushed the STM32WB0x_BLE branch from 4446e0d to 75a4879 Compare October 15, 2024 12:14

zephyrbot added manifest DNM This PR should not be merged (Do Not Merge) labels Oct 15, 2024

HoZHel force-pushed the STM32WB0x_BLE branch from 75a4879 to 937420a Compare October 15, 2024 12:34

HoZHel marked this pull request as ready for review October 15, 2024 12:35

HoZHel requested review from alwa-nordic, jhedberg and Vudentz as code owners October 15, 2024 12:35

zephyrbot added the area: Bluetooth Host label Oct 15, 2024

zephyrbot requested review from FRASTM, gautierg-st, GeorgeCGV, hermabe, marwaiehm-st, mathieuchopstm, sjanc, Thalley and theob-pro October 15, 2024 12:36

zephyrbot assigned jhedberg and alwa-nordic Oct 15, 2024

Thalley removed their request for review October 15, 2024 12:53

jhedberg requested changes Oct 15, 2024

View reviewed changes

jhedberg reviewed Oct 16, 2024

View reviewed changes

dts/bindings/bluetooth/st,hci-stm32wb0.yaml Outdated Show resolved Hide resolved

HoZHel force-pushed the STM32WB0x_BLE branch from 937420a to 3ca6f41 Compare October 16, 2024 14:23

jhedberg previously approved these changes Oct 24, 2024

View reviewed changes

alwa-nordic previously approved these changes Oct 24, 2024

View reviewed changes

erwango modified the milestones: v4.0.0, v4.1.0 Nov 8, 2024

HoZHel added 7 commits November 22, 2024 13:52

bluetooth: Kconfig: Introduce Kconfig parameter for STM32WB0x series

23af1f9

Introduce Kconfig parameter for STM32WB0x HCI Bluetooth driver. Signed-off-by: Ali Hozhabri <[email protected]>

dts: bindings: bluetooth: Add yaml file required by STM32WB0x HCI driver

527ed70

Add a yaml file required by STM32WB0x bluetooth HCI driver. Signed-off-by: Ali Hozhabri <[email protected]>

drivers: bluetooth: hci: Add Bluetooth HCI driver for STM32WB0x

4948a0b

Add Bluetooth HCI driver for STM32WB0x series. Modify CMakeLists.txt to compile the driver based on its kconfig parameter. Signed-off-by: Ali Hozhabri <[email protected]>

dts: arm: st: wb0: Add BLE feature to STM32WB0x at SOC level

1aa8ac9

Add BLE feature to STM32WB0x series at SOC level. Signed-off-by: Ali Hozhabri <[email protected]>

soc: stm32: stm32wb0x: Dedicate RAM section for BLE part

1cb6e68

Dedicate RAM section on STM32WB0x for BLE part based on the number of radio tasks and device type. Signed-off-by: Ali Hozhabri <[email protected]>

HoZHel dismissed stale reviews from alwa-nordic and jhedberg via 7a7ae2f November 22, 2024 13:16

HoZHel force-pushed the STM32WB0x_BLE branch from 6b6751b to 7a7ae2f Compare November 22, 2024 13:16

zephyrbot added the area: Bluetooth HCI Bluetooth HCI Driver label Nov 22, 2024

jhedberg previously approved these changes Nov 22, 2024

View reviewed changes

west.yml: Update hal_stm32

4d09440

Modify west.yaml to update hal_stm32. Signed-off-by: Ali Hozhabri <[email protected]>

HoZHel dismissed jhedberg’s stale review via 4d09440 November 25, 2024 08:46

HoZHel force-pushed the STM32WB0x_BLE branch from 7a7ae2f to 4d09440 Compare November 25, 2024 08:46

jhedberg approved these changes Nov 25, 2024

View reviewed changes

erwango approved these changes Nov 25, 2024

View reviewed changes

erwango removed the DNM This PR should not be merged (Do Not Merge) label Nov 25, 2024

msmttchr approved these changes Nov 25, 2024

View reviewed changes

kartben merged commit 4875f29 into zephyrproject-rtos:main Nov 25, 2024
29 of 30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ST: STM32WB0: Implement BLE feature #79734

ST: STM32WB0: Implement BLE feature #79734

HoZHel commented Oct 11, 2024 •

edited by erwango

Loading

erwango Oct 15, 2024

HoZHel Oct 15, 2024 •

edited

Loading

erwango Oct 15, 2024

jhedberg Oct 15, 2024

HoZHel Oct 16, 2024

msmttchr Oct 17, 2024

msmttchr Oct 17, 2024

alwa-nordic Oct 17, 2024 •

edited

Loading

alwa-nordic Oct 17, 2024 •

edited

Loading

alwa-nordic Oct 17, 2024

zephyrbot commented Oct 15, 2024 •

edited

Loading

jhedberg Oct 15, 2024

jhedberg Oct 15, 2024

jhedberg Oct 15, 2024

HoZHel Oct 16, 2024

HoZHel Oct 22, 2024

alwa-nordic commented Nov 7, 2024

erwango commented Nov 8, 2024

erwango commented Nov 22, 2024

HoZHel commented Nov 22, 2024

HoZHel commented Nov 22, 2024

HoZHel commented Nov 25, 2024

msmttchr left a comment

		default y if !BT_CTLR && !BT_STM32_IPM && !BT_ESP32 && !BT_STM32WBA
		default y if !BT_CTLR && !BT_STM32_IPM && !BT_ESP32 && !BT_STM32WBA && !BT_STM32WB0

		static uint8_t legacy_cmd_issued = FALSE, extended_cmd_issued = FALSE;
		uint8_t allowed = TRUE;

ST: STM32WB0: Implement BLE feature #79734

ST: STM32WB0: Implement BLE feature #79734

Conversation

HoZHel commented Oct 11, 2024 • edited by erwango Loading

Choose a reason for hiding this comment

HoZHel Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwa-nordic Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

alwa-nordic Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zephyrbot commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwa-nordic commented Nov 7, 2024

erwango commented Nov 8, 2024

erwango commented Nov 22, 2024

HoZHel commented Nov 22, 2024

HoZHel commented Nov 22, 2024

HoZHel commented Nov 25, 2024

msmttchr left a comment

Choose a reason for hiding this comment

HoZHel commented Oct 11, 2024 •

edited by erwango

Loading

HoZHel Oct 15, 2024 •

edited

Loading

alwa-nordic Oct 17, 2024 •

edited

Loading

alwa-nordic Oct 17, 2024 •

edited

Loading

zephyrbot commented Oct 15, 2024 •

edited

Loading