zephyr
zephyr copied to clipboard
Introduction of Atmosic Platform
This is Atmosic Technologies' first set of changes enabling the build of samples/hello_world for board ATMEVK-3330-QN-5.
More to come in future pull requests are additional boards in the same family of BLE SoC platforms (atm33evk) as well as similar content for two other families of platforms.
This change set is intentionally kept small to make it easy to review and get through the first level of issues such as setting up a new HAL for our platform within the zephyrproject-rtos space. The content can be found here: https://github.com/Atmosic/_hal_atmosic.
Hello @hzarnani, and thank you very much for your first pull request to the Zephyr project! Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary. If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊
The following west manifest projects have been modified in this Pull Request:
| Name | Old Revision | New Revision | Diff |
|---|---|---|---|
| hal_atmosic | None | c760881bad95fa31d8906508880fab676ba8d6f0 | N/A |
Note: This message is automatically posted and updated by the Manifest GitHub Action.
Needs converting to hwmv2
Thanks for the feedback. Will do.
Initial comments after having a very quick look at the hal repo:
- You're adding a sample to the hal repo, why?
- You're adding ramfunc and other linker files to the repo, why?
- You have mbedtls and micro ecc in the repo, zephyr doesn't even use micro ecc, why is it there?
- You are seemingly adding a bunch of random files depending upon configuration options, and hidden compiling with libraries e.g. mbedtls - this looks wrong
- There are parts of the code doing things with bluetooth, does the device work with the zephyr bluetooth controller and host without binary blobs? If not, that should not be there
- You're adding what looks to be hidden libraries e.g. "AT command support", what is this? Libraries for functionality should be in zephyr itself not a hal directory, we do not allow hidden hal libraries like that, hal is for hardware
Initial comments after having a very quick look at the hal repo:
- You're adding a sample to the hal repo, why?
Yes, the spe example is a minimal secure support application (much smaller than TF-M) to enable Zephyr applications to run in nonsecure mode.
- You're adding ramfunc and other linker files to the repo, why?
Yes, these are necessary to support binary libraries in our self-hosted repositories.
- You have mbedtls and micro ecc in the repo, zephyr doesn't even use micro ecc, why is it there?
These support mbedtls crypto acceleration on our platform through a combination of hardware and optimized software.
- You are seemingly adding a bunch of random files depending upon configuration options, and hidden compiling with libraries e.g. mbedtls - this looks wrong
We have a significant amount of code supporting multiple platforms and environments in self-hosted repositories that aren't shown here. This is our first attempt to peel out a reasonable set of software to upstream to zephyrproject.org.
If the feedback here is that this first step should reduce hal_atmosic to absolutely nothing more than the minimal hardware peripherals necessary for hello_world and then grow it later, we can go that route.
- There are parts of the code doing things with bluetooth, does the device work with the zephyr bluetooth controller and host without binary blobs? If not, that should not be there
Our platforms use the zephyr bluetooth host, but we have our own bluetooth controller, which has to be in binary form for a variety of reasons.
- You're adding what looks to be hidden libraries e.g. "AT command support", what is this? Libraries for functionality should be in zephyr itself not a hal directory, we do not allow hidden hal libraries like that, hal is for hardware
We can move this to our self-hosted repos, if it is offensive.
Fundamental problems:
- The HAL (hal_atmosic) uses Zephyr APIs
We should be able to migrate that content out to our self-hosted repos.
- The HAL contains CMake and Kconfig, those really belong in zephyr/modules
- No zephyr/modules/hal_atmosic folder
A little history here - we began our development back on Zephyr v0.6, and things have changed many times since then. Is there a definitive document that covers 2 and 3?
- The HAL contains too much stuff that has nothing to do with a HAL
We can migrate that content out to our self-hosted repos.
Yes, these are necessary to support binary libraries in our self-hosted repositories.
Then the commit is not acceptable
- The HAL contains CMake and Kconfig, those really belong in zephyr/modules
- No zephyr/modules/hal_atmosic folder
A little history here - we began our development back on Zephyr v0.6, and things have changed many times since then. Is there a definitive document that covers 2 and 3?
I would appreciate some clarity on both of these points.
There seem to be plenty of CMake counter examples: https://github.com/zephyrproject-rtos/hal_ambiq/blob/main/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_nxp/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_renesas/blob/main/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_nuvoton/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_xtensa/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_ti/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_stm32/blob/main/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_silabs/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_st/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_microchip/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_infineon/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_atmel/blob/master/CMakeLists.txt https://github.com/zephyrproject-rtos/hal_hpmicro/blob/zephyr/CMakeLists.txt ...
With respect to our hal's modular Kconfig hierarchy, are you suggesting that we flatten that into a single Kconfig.<manufacturer> under zephyr/modules? Or perhaps migrate said hierarchy under zephyr/modules/hal_atmosic? Please help me understand why that is necessary/preferred vs. having Kconfig in the hal itself.
Yes, these are necessary to support binary libraries in our self-hosted repositories.
Then the commit is not acceptable
They also support source code currently in hal_atmosic:
rev-2/include/armgcc/compiler.h:#define __UNINIT __attribute__((section(".uninit." AT)))
rev-5/include/armgcc/compiler.h:#define __UNINIT __attribute__((section(".uninit." AT)))
drivers/atm_bp_clock/atm_bp_clock.c: __attribute__((section(".data_text"))) __STATIC
drivers/atm_bp_clock/atm_bp_clock.c:__attribute__((noinline, section(".data_text"))) void
drivers/atm_bp_clock/atm_bp_clock.c:__attribute__((noinline, section(".data_text"))) void
Nearly all of the code in hal_atmosic has a common origin and is used by several other environments. Read: supporting, but not specific to Zephyr.
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.
Hi, when will the integration mature to be merged with the Zephyr official release? My team is evaluating Atmosic ATM330e to replace our previous MCU. It runs on Zephyr, and we also plan to use Zephyr for quick porting.
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.