-
Notifications
You must be signed in to change notification settings - Fork 146
lib: stm32wba: Fix Link Layer and LE integration on STM32WBA #305
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: Fix Link Layer and LE integration on STM32WBA #305
Conversation
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 general, it is unclear why this PR is needed as #280 already aligned lib on v1.6.0. Am I mixing something ?
* This Synopsys DWC Bluetooth Low Energy Combo Link Layer/MAC software and | ||
* associated documentation ( hereinafter the "Software") is an unsupported | ||
* proprietary work of Synopsys, Inc. unless otherwise expressly agreed to in | ||
* writing between Synopsys and you. The Software IS NOT an item of Licensed | ||
* Software or a Licensed Product under any End User Software License Agreement | ||
* or Agreement for Licensed Products with Synopsys or any supplement thereto. | ||
* Synopsys is a registered trademark of Synopsys, Inc. Other names included in | ||
* the SOFTWARE may be the trademarks of their respective owners. |
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 Zephyr licence compatibility purposes, files in this module where updated to the latest License which doesn't contain these lines. Please keep it as is. Apply to all files
See #262.
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.
License comments in Link Layer header files are changed as required
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.
Files should reflect Cube package library and changes should be documented and justified, ideally one by commit and
in this particular case, I think a deeper discussion is required.
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.
LOG Module is reverted. I will insert a new specific PR for this feature
12c1ca9
to
4c5c07f
Compare
This PR is due to the fact that in previous PR#280 (aligned lib on v1.6.0), the header files of the Link Layer have not been updated correctly. |
lib/stm32wba/STM32_WPAN/link_layer/ll_cmd_lib/inc/common_types.h
Outdated
Show resolved
Hide resolved
lib/stm32wba/STM32_WPAN/link_layer/ll_cmd_lib/inc/common_types.h
Outdated
Show resolved
Hide resolved
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 we want to be strict with Zephyr guidelines we should modify some modify not required changes, tabs, spaces.
If we want to maintain exactly the same cube code we can keep the changes.
For me LGTM, but I leave @erwango to decide.
@Chastillon fyi |
Sure, we should keep the files as close as Cube, with the exception of line endings (use dos2unix) and trailing white spaces. These 2 changes are enforced when files are updated with the dedicated script. |
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.
To ease the work for the next update and fix the trailing spaces issues I suggest that you run the update script (which should already have been the case at last update).
lib/stm32wba/STM32_WPAN/link_layer/ll_cmd_lib/config/ble_full/ll_fw_config.h
Outdated
Show resolved
Hide resolved
lib/stm32wba/STM32_WPAN/link_layer/ll_cmd_lib/inc/rfd_dev_config.h
Outdated
Show resolved
Hide resolved
4c5c07f
to
e6daffc
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.
@vtardy-st, thanks a lot for this PR. I suggested some changes to make this PR minimal, and I have asked few questions regarding the actual usage of some files (It is requested in ST strategy in Zephyr to avoid adding files not used).
Is this PR needed to add 15.4 support ? If yes, I do not see 15.4 libraries added in the list of blob files (zephyr/module.yml
), if not, ignore this comment.
lib/stm32wba/STM32_WPAN/link_layer/ll_cmd_lib/inc/rfd_dev_config.h
Outdated
Show resolved
Hide resolved
lib/stm32wba/STM32_WPAN/link_layer/ll_sys/inc/ll_sys_sequencer.h
Outdated
Show resolved
Hide resolved
Actually, PR should contain required changes to have files on par with the Cube release they're extracted from (with the exception of trailing white space removal) here: https://github.com/STMicroelectronics/STM32CubeWBA/tree/v1.6.0/Middlewares/ST. |
e6daffc
to
384b73d
Compare
An update has been done in order to be fully aligned with the CubeMx release 1.6.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.
@vtardy-st, thanks for these alignments with v1.6.0 for BLE part.
I submitted few comments/request to change in order to:
- Reject changes which are not intended and probably due to some files moved between 1.5.0 and 1.6.0. In this case, the script could not apply changes correctly.
- Review files who has been actually changed to align to 1.6.0 and they seems to be OK
Finally, I have a doubts that some files are not needed at all, for example:
lib/stm32wba/Common/WPAN/Modules/Flash
, I think this is managed by Zephyr NVMlib/stm32wba/Common/WPAN/Modules/Log
, I doubt this is used or integrated with Zephyr logging mechanismlib/stm32wba/Common/WPAN/Modules/RTDebug
, I do not think this is used at all.
For the latter points, if confirmed, I would consider them in a separate PR or when we will move to 1.7.0
lib/stm32wba/BLE_TransparentMode/System/Config/Log/log_module_conf.h
Outdated
Show resolved
Hide resolved
384b73d
to
3ad5e33
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 for 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.
Thanks for the update.
@vtardy-st Before merging, we need a zephyr PR targeting this PR. Can you open it ? |
Already mentioned in the previous comments: |
I missed it indeed, perfect! Let's wait CI result after rebase |
Update Link Layer header files and LE porting files to be fully aligned on STM32Cube_FW_WBA Release 1.6.0. Signed-off-by: Vincent Tardy <[email protected]>
3ad5e33
to
dd35fd2
Compare
some header files and system files of the Link Layer in zephyr are not aligned with the header file and system files of the Link Layer in STM32Cube_FW_WBA Release 1.6.0