-
Notifications
You must be signed in to change notification settings - Fork 146
lib: stm32wba: 802.15.4 driver integration #311
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
lib: stm32wba: 802.15.4 driver integration #311
Conversation
539e08e
to
6631ae3
Compare
6631ae3
to
ebe3cdc
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.
For clarity, please split your changes in several commits:
- Initial commit(s) that make(s) required modifications prior the introduction of the new library (files moving and sp on)
- Dedicated commit that only introduces new lib + required CMake changes
Last, I don't see changes to the library update scripts. I really think this should be added to help maintainance
What is the plan for library update ?
ebe3cdc
to
89b8400
Compare
Changes included in this PR have been splitted in 2 commits :
|
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.
In the first commit, a lot of files are moved in but not updated in README or update script file. These files should be updated. This is mandatory for further maintenance.
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.
Sorry for the review split in multiple comments, this will be the last one.
I'm not opposed to reshuffling the CMakeLists.txt, and this probably welcome given the introduction of a new library and the fact that some components are shared between the 2.
Though, I think current proposal goes to far in this direction and makes things quite difficult to follow.
I'd propose to limit to 3/4 files instead of the 6 proposed currently (in my view, lesser is better, but I may miss technical constraints)
|
||
if(CONFIG_IEEE802154_STM32WBA) | ||
zephyr_include_directories(Inc) | ||
zephyr_include_directories(Core/Inc/802154) |
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.
We need to exclude this directory if the build is for Zigbee because ZB has its own app_conf.h file
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.
A change has been implemented to include this directory only if CONFIG_NET_L2_CUSTOM_IEEE802154_STM32WBA is not defined
89b8400
to
8fc6994
Compare
8fc6994
to
8704abd
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.
Minor nitpicking comments.
lib/stm32wba/CMakeLists.txt
Outdated
OR CONFIG_BT_TRANSMIT_POWER_CONTROL | ||
OR CONFIG_BT_SUBRATING | ||
OR CONFIG_BT_CTLR_ADV_PERIODIC_ADI_SUPPORT | ||
OR CONFIG_BT_EXT_ADV_CODING_SELECTION) |
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.
For consistency, the 10 lines above should be indented by 1 more space char.
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.
identation has been reviewed
lib/stm32wba/CMakeLists.txt
Outdated
set(LL_LIB "WBA6_LinkLayer_Thread_lib.a") | ||
else() | ||
message(STATUS "WBA6_LinkLayer15_4.a lib selected") | ||
set(LL_LIB "WBA6_LinkLayer15_4.a") |
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:
set(LL_LIB "WBA6_LinkLayer15_4.a") | |
set(LL_LIB "WBA6_LinkLayer15_4.a") |
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 change done
lib/stm32wba/README.rst
Outdated
|
||
Purpose: | ||
This library is used on STM32WBA series to port BLE controller library in | ||
This library is used on STM32WBA series to port BLE and IEEE802154 controller library in |
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.
Replace library
with libraries
?
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.
done
*/ | ||
|
||
#ifndef STM32WBA_802154_CALLOUTS_H_ | ||
#define STM32WBA_802154_CALLOUTS_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.
As per the header file names, it would be better the header guard macros are named STM32_802154_CALLBACKS_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.
header guard macro has been modified
8704abd
to
f24aa8b
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.
LGTM AFAICT.
* | ||
* @retval none | ||
*/ | ||
void (*stm32wba_802154_ral_cbk_ed_scan_done)(int8_t rssiResult); |
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.
Non-blokcing issue but for information:
These fields belong to struct stm32wba_802154_ral_cbk_dispatch_tbl
that is already explicitly prefixed stm32wba_802154_ral_cbk_
so there is no strong need for the prefix also in the field names.
f24aa8b
to
9b4878b
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.
Getting close..
rename ble folder and add/change CMakeList Signed-off-by: Vincent Tardy <[email protected]>
Add 802.15.4 driver integration Signed-off-by: Vincent Tardy <[email protected]>
9b4878b
to
04222c0
Compare
if (CONFIG_BT_STM32WBA) | ||
zephyr_include_directories(ble/stack/include) | ||
zephyr_include_directories(ble/stack/include/auto) | ||
zephyr_include_directories(link_layer/ll_cmd_lib/config/ble_full) |
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.
Ideally indentation should have been fixed in initial commit, but I' won't block for this at this point.
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.
Looks good to me, thanks!
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.
LGTM AFAICT.
It's a draft version to retrieve a first review
Add 802.15.4 driver integration
In the lib\stm32wba, 'BLE_TransparentMode' folder has been renamed 'ble'.
Add CMakeLists.txt file in /ble folder
Add IEEE802154 folder