-
Notifications
You must be signed in to change notification settings - Fork 150
stm32wba : ble-802.15.4 concurrent mode integration #319
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
stm32wba : ble-802.15.4 concurrent mode integration #319
Conversation
131fbdb to
7cb891e
Compare
628d59a to
f48981b
Compare
f48981b to
e29c333
Compare
lib/stm32wba/README.rst
Outdated
|
|
||
| * Discard "static" implementation of ll_sys_bg_temperature_measurement_init to allow specific zephyr implementation | ||
| Impacted file: ll_sys_if.c | ||
| ll_sys.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems no real change here, we could revert ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change has been reverted
|
Hello @mathieuchopstm and @etienne-lms. Could you please have a look ? |
e29c333 to
8f70ccc
Compare
mathieuchopstm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some refinement needed but seems overall OK
| #ifndef __ZEPHYR__ | ||
| #define RADIO_SW_LOW_INTR_PRIO (15) /* 2.4GHz RADIO low ISR priority */ | ||
| #else | ||
| #define RADIO_SW_LOW_INTR_PRIO (14) /* 2.4GHz RADIO low ISR priority */ | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interrupt priority should come from DT anyways so I'm a bit surprised this is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interrupt priority constant is used in the link_layer_register_isr() function defined in zephyr\soc\st\stm32\stm32wbax\hci_if\linklayer_plat_adapt.c
If change should be done, it will be done in another PR
| #ifndef MAX | ||
| #define MAX( x, y ) (((x)>(y))?(x):(y)) | ||
| #endif | ||
|
|
||
| #ifndef MIN | ||
| #define MIN( x, y ) (((x)<(y))?(x):(y)) | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this header is visible from the Zephyr side, please use an approach similar to VCNano8000e stack instead: add private compile definition for the library only and gate these behind the private definition
hal_stm32/lib/vc8000nanoe/CMakeLists.txt
Lines 4 to 6 in 286dd28
| zephyr_library() | |
| zephyr_library_compile_definitions(H1_INTERNAL) |
hal_stm32/lib/vc8000nanoe/inc/enccommon.h
Lines 156 to 159 in 286dd28
| #ifdef H1_INTERNAL | |
| #define MAX(a, b) ((a) > (b) ? (a) : (b)) | |
| #define MIN(a, b) ((a) < (b) ? (a) : (b)) | |
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is herited from CubeMx
8f70ccc to
c059c4c
Compare
|
|
||
| BPKA_error = 0; | ||
|
|
||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably with default case in switch/case instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is heritated from CubeMx. Do you think this change is required ?
|
I can't mark conversations are resolved so I added 🚀 reaction to my own "resolved" messages instead. |
c059c4c to
ac58b83
Compare
|
@vtardy-st Please rebase |
ac58b83 to
7229677
Compare
…ries Add WBA6_LinkLayer_BLE_Basic_15_4_lib_Zephyr.a and WBA6_LinkLayer_BLE_Full_15_4_lib_Zephyr.a Signed-off-by: Vincent Tardy <[email protected]>
7229677 to
d271137
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combination of:
- files duplicated depending on the use case (see comment)
- functions adapted in place instead of in zephyr as it should actually be done (see comment)
makes the libraries more and more complex to review and maintain.
I don't think this is the right direction and approach should be reviewed.
69d663c to
d9dbc78
Compare
erwango
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. This is going in the good direction. I guess we can fix the remaining issue of Zephyr adaptation done in this repo instead of zephyr/ in a next repo.
Looking to https://github.com/vtardy-st/hal_stm32/blob/stm32wba_1_7_0_ble_802154_concurrent/lib/stm32wba/README.rst, there is indentation issues to fix to have the file readable, it would be nice to fix this (as an additional commit for issues that are not coming from current PR)
Otherwise LGTM
d9dbc78 to
f7b46f3
Compare
mathieuchopstm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor nits + #319 (comment) left to address.
I think these are non-blocking though.
| #ifndef __ZEPHYR__ | ||
| uint32_t linklayer_slp_clk_src = LL_RCC_RADIOSLEEPSOURCE_NONE; | ||
|
|
||
| #ifdef __ZEPHYR__ | ||
| LINKLAYER_PLAT_EnableBackupDomainAccess(); | ||
| /* Get the Link Layer sleep timer clock source */ | ||
| linklayer_slp_clk_src = LL_RCC_RADIO_GetSleepTimerClockSource(); | ||
| if(linklayer_slp_clk_src == LL_RCC_RADIOSLEEPSOURCE_NONE) | ||
| { | ||
| /* If there is no clock source defined, should be selected before */ | ||
| assert_param(0); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be tabs indentations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation type is this file is herited from STM32Cube
| /* Enable AHB5ENR peripheral clock (bus CLK) */ | ||
| __HAL_RCC_RADIO_CLK_ENABLE(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep tabs as indentation
| /* Enable AHB5ENR peripheral clock (bus CLK) */ | |
| __HAL_RCC_RADIO_CLK_ENABLE(); | |
| /* Enable AHB5ENR peripheral clock (bus CLK) */ | |
| __HAL_RCC_RADIO_CLK_ENABLE(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation type is this file is herited from STM32Cube
Add support of ble-ieee 802.15.4 concurrent mode Signed-off-by: Vincent Tardy <[email protected]>
Porting of modes ble, ieee 802.15.4 and concurrent are refactorised in one unique RF_Integration folder supporting the 3 different mode configurations. Signed-off-by: Vincent Tardy <[email protected]>
Rework the README.rst file to fix indentation issues. Signed-off-by: Vincent Tardy <[email protected]>
f41c870 to
5c66606
Compare
Integration of the BLE-802.15.4 concurrent mode on stm32wba based on CubeWBA release 1.7.0