Conversation
There was a problem hiding this comment.
- some errors must be propagated better
- UART drivers inclusion in bootloader should depend on whether UART is enabled in the config (default: off)
- No tests: at least one build test is needed, but at this stage it would also be useful to have a comparison of the footprint for the extra added layer. Please check footprint test, build on the same target/compiler with wolfHAL and compare footprints
- wrong submodule URL: it should be
https://github.com/wolfssl/wolfhal, notgit@github.com ...
config/wolfHAL/stm32wb55_nucleo.c
Outdated
| @@ -0,0 +1,101 @@ | |||
| #include <wolfHAL/platform/st/stm32wb55xx.h> | |||
|
|
|||
| whal_StRcc_PeriphClk periphClkEn[] = | |||
There was a problem hiding this comment.
These can't be static since they are externed in both hal/wolfhal.c and the test app
config/wolfHAL/stm32wb55_nucleo.c
Outdated
| WHAL_ST_RCC_PERIPH_FLASH, | ||
| }; | ||
|
|
||
| whal_Clock wbClock = { |
There was a problem hiding this comment.
These can't be static since they are externed in both hal/wolfhal.c and the test app
config/wolfHAL/stm32wb55_nucleo.c
Outdated
| }, | ||
| }; | ||
|
|
||
| whal_Gpio wbGpio = { |
There was a problem hiding this comment.
These can't be static since they are externed in both hal/wolfhal.c and the test app
config/wolfHAL/stm32wb55_nucleo.c
Outdated
| .pinCount = 3, | ||
| }; | ||
|
|
||
| whal_Uart wbUart = { |
There was a problem hiding this comment.
These can't be static since they are externed in both hal/wolfhal.c and the test app
config/wolfHAL/stm32wb55_nucleo.c
Outdated
| }, | ||
| }; | ||
|
|
||
| whal_Flash wbFlash = { |
There was a problem hiding this comment.
These can't be static since they are externed in both hal/wolfhal.c and the test app
config/wolfHAL/stm32wb55_nucleo.mk
Outdated
There was a problem hiding this comment.
inclusion of st_uart.o should be conditional to config (default is no uart enabled in bootloader)
hal/uart/uart_drv_wolfhal.c
Outdated
| int uart_rx(uint8_t *c) | ||
| { | ||
| whal_Uart_Recv(&wbUart, c, 1); | ||
| /* ALEX NOTE: this function also returns zero if no data is available... */ |
There was a problem hiding this comment.
a better handling of whal_ return values is required
AlexLanzano
force-pushed
the
wolfHAL-integration
branch
from
ab7ccb4 to
3e80589
Compare
AlexLanzano
force-pushed
the
wolfHAL-integration
branch
2 times, most recently
from
0743918 to
54cdcf8
Compare
rizlik
left a comment
rizlik
left a comment
There was a problem hiding this comment.
This is just a partial review, I need to dig into the wolfHAL design yet.
| WOLFHAL_TARGET=stm32wb | ||
| WOLFHAL_BOARD=stm32wb55_nucleo | ||
| WOLFHAL_FLASH_START=0x08000000 | ||
| WOLFHAL_FLASH_SIZE=0x100000 | ||
| WOLFHAL_NO_GPIO=1 |
There was a problem hiding this comment.
I'm not sure that WOLFHAL configuration must lives in the same configuration of wolfBoot. I would like to have a better separation. I'm not sure how to drive the wolfBoot toolchain though, probably there is the need of some integration in arch.mk and similar
There was a problem hiding this comment.
These are technically wolfBoot configuration options for the wolfBoot wolfHAL target. Maybe we need better naming for these?
| WOLFBOOT_SECTOR_SIZE=0x1000 | ||
| WOLFBOOT_PARTITION_SIZE=0x20000 | ||
| WOLFBOOT_PARTITION_BOOT_ADDRESS=0x08008000 | ||
| WOLFBOOT_PARTITION_BOOT_ADDRESS=0x08010000 |
There was a problem hiding this comment.
is this unrelated to wolfHAL?
There was a problem hiding this comment.
Will remove. This was to have enough room for debug symbols.
| #include <stdint.h> | ||
| #include <wolfHAL/wolfHAL.h> | ||
|
|
||
| extern whal_Uart wbUart; |
There was a problem hiding this comment.
NIT: if we really have an "magical" external whal_Uart, maybe we can either make it implicit or, better imo, add a whal_Uart whal_Uart_Get_Port(uint32_t id) or similar.
In general while I think the library must support compile time configuration and object, it might be nice to give a little flexibility so that can be used also in context where you want to dynamically create objects or configure the objects differently.
There was a problem hiding this comment.
Im hesitant to add Get functions like whal_Uart_GetPort since that would increase image size.
Maybe I could name the externed variables better to make it more explicit of what they are?
I.E. g_whalUart or g_boardUart?
| ifeq ($(DEBUG_UART),1) | ||
| endif |
3 participants
I hacked together this quick wolfHAL integration using the current implementation of wolfHAL that I already worked on.
This PR is more-so meant for discussion/buy-in on how wolfHAL should be used within wolfBoot rather than getting this PR merged in.
Here's an overview of what I did:
wolfhalTARGETstm32wb-wolfhal.config./config/wolfHAL/hal_*api functions using wolfHALuart_*api functions using wolfHALI've tested this on the stm32wb nucleo board and I'm able to boot fully into the target app
Here is the wolfHAL repo I used to build this https://github.com/AlexLanzano/wolfHAL/tree/wolfboot-integration
As you can see the design is unchanged from what I presented in the meeting. SInce I had the stm32wb drivers already done in this design I figured it would be best to just give the integration a shot and have you guys point out the specific parts you dont like