wolfHAL Design Review#37
Open
AlexLanzano wants to merge 1 commit intowolfSSL:tmpfrom
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR provides a stripped-down, additive snapshot of the wolfHAL codebase for a top-to-bottom design review (not intended for merging), showcasing the core HAL abstractions, STM32 platform drivers, example boards, tests, and documentation.
Changes:
- Adds core wolfHAL public headers (GPIO/UART/Flash/Timer/IRQ + core helpers like regmap/timeout/bitops/endian) and corresponding generic dispatch implementations.
- Introduces STM32WB and STM32L1 platform/board support (drivers, board init, linker scripts, startup/IVT).
- Adds a minimal test framework plus host “core” unit tests and HW-facing board tests; includes a blinky example and onboarding docs.
Reviewed changes
Copilot reviewed 82 out of 83 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfHAL/wolfHAL.h | Umbrella include header for core wolfHAL modules. |
| wolfHAL/uart/uart.h | Generic UART API + driver vtable definitions. |
| wolfHAL/uart/stm32wb_uart.h | STM32WB UART driver config + API surface. |
| wolfHAL/uart/stm32l1_uart.h | STM32L1 UART aliasing to STM32F4 UART. |
| wolfHAL/uart/stm32f4_uart.h | STM32F4 UART driver config and API surface. |
| wolfHAL/timer/timer.h | Generic timer API + driver vtable definitions. |
| wolfHAL/timer/systick.h | SysTick timer driver configuration and API. |
| wolfHAL/timeout.h | Timeout abstraction macros for polling/delays. |
| wolfHAL/regmap.h | Register access helpers (read/write/update/poll). |
| wolfHAL/power/stm32l1_pwr.h | STM32L1 power (PWR) helper API. |
| wolfHAL/power/power.h | Board-level power handle type definition. |
| wolfHAL/platform/st/stm32wb55xx.h | STM32WB55 convenience initializers/macros. |
| wolfHAL/platform/st/stm32l152re.h | STM32L152 convenience initializers/macros. |
| wolfHAL/platform/arm/cortex_m4.h | Cortex-M4 device macros (SysTick/NVIC regmaps). |
| wolfHAL/platform/arm/cortex_m3.h | Cortex-M3 device macros (SysTick/NVIC regmaps). |
| wolfHAL/irq/irq.h | Generic IRQ controller abstraction + vtable. |
| wolfHAL/irq/cortex_m4_nvic.h | NVIC driver API/config definitions. |
| wolfHAL/gpio/stm32wb_gpio.h | STM32WB GPIO pin packing + driver API/config. |
| wolfHAL/gpio/stm32l1_gpio.h | STM32L1 GPIO aliasing to STM32WB GPIO. |
| wolfHAL/gpio/gpio.h | Generic GPIO API + driver vtable definitions. |
| wolfHAL/flash/stm32wb_flash.h | STM32WB flash driver config + helpers. |
| wolfHAL/flash/stm32l1_flash.h | STM32L1 flash driver config + helpers. |
| wolfHAL/flash/flash.h | Generic flash API + driver vtable definitions. |
| wolfHAL/error.h | Shared error code definitions. |
| wolfHAL/endian.h | Endian load/store helper functions. |
| wolfHAL/clock/stm32wb_rcc.h | STM32WB RCC helper APIs/config types. |
| wolfHAL/clock/stm32l1_rcc.h | STM32L1 RCC helper APIs/config types. |
| wolfHAL/clock/clock.h | Board-level clock handle type definition. |
| wolfHAL/bitops.h | Bitfield encode/decode helpers. |
| tests/uart/test_uart.c | Generic UART API argument/NULL handling tests. |
| tests/test.h | Minimal C test framework + assertions/printf-like output. |
| tests/main.c | HW test runner (board + per-module tests). |
| tests/gpio/test_stm32wb_gpio.c | STM32WB GPIO platform register-level tests. |
| tests/gpio/test_stm32l1_gpio.c | STM32L1 GPIO platform tests via source include aliasing. |
| tests/gpio/test_gpio.c | Generic GPIO API tests. |
| tests/core/test_timeout.c | Host-side timeout macro unit tests. |
| tests/core/test_endian.c | Host-side endian helper unit tests. |
| tests/core/test_bitops.c | Host-side bitops unit tests. |
| tests/core/main.c | Host-side unit test runner. |
| tests/core/Makefile | Build rules for host-side unit tests. |
| tests/clock/test_stm32wb_clock.c | STM32WB RCC platform test(s). |
| tests/clock/test_clock.c | Placeholder for generic clock tests (none by design). |
| tests/README.md | Documentation for running HW and host-side tests. |
| tests/Makefile | HW test build orchestration (boards/tests discovery). |
| src/uart/uart.c | Generic UART vtable dispatch implementations. |
| src/uart/stm32wb_uart.c | STM32WB polled UART driver implementation. |
| src/uart/stm32l1_uart.c | STM32L1 UART driver implementation via source include aliasing. |
| src/uart/stm32f4_uart.c | STM32F4 polled UART driver implementation. |
| src/timer/timer.c | Generic timer vtable dispatch implementations. |
| src/timer/systick.c | SysTick timer driver implementation. |
| src/power/stm32l1_pwr.c | STM32L1 power helper implementation. |
| src/irq/cortex_m4_nvic.c | NVIC driver implementation. |
| src/gpio/stm32wb_gpio.c | STM32WB GPIO driver implementation. |
| src/gpio/stm32l1_gpio.c | STM32L1 GPIO driver implementation via source include aliasing. |
| src/gpio/gpio.c | Generic GPIO vtable dispatch implementations. |
| src/flash/stm32l1_flash.c | STM32L1 flash driver implementation. |
| src/flash/flash.c | Generic flash vtable dispatch implementations. |
| src/clock/stm32wb_rcc.c | STM32WB RCC helper implementation. |
| src/clock/stm32l1_rcc.c | STM32L1 RCC helper implementation. |
| examples/blinky/main.c | Simple blinky example using wolfHAL + board layer. |
| examples/blinky/Makefile | Build rules for blinky example with board integration. |
| examples/README.md | Documentation for building/running examples. |
| docs/adding_an_example.md | Guide for adding new example applications. |
| docs/adding_a_test.md | Guide for adding new tests (generic + platform-specific). |
| docs/adding_a_board.md | Guide for adding new board support packages. |
| boards/stm32wb55xx_nucleo/linker.ld | STM32WB55 linker script for demos/tests. |
| boards/stm32wb55xx_nucleo/ivt.c | STM32WB55 startup + vector table. |
| boards/stm32wb55xx_nucleo/board.mk | STM32WB55 toolchain/flags/sources for builds. |
| boards/stm32wb55xx_nucleo/board.h | STM32WB55 board API + global device instances. |
| boards/stm32wb55xx_nucleo/board.c | STM32WB55 device instantiation + Board_Init/Deinit. |
| boards/stm32l152re_nucleo/linker.ld | STM32L152 linker script for demos/tests. |
| boards/stm32l152re_nucleo/ivt.c | STM32L152 startup + vector table. |
| boards/stm32l152re_nucleo/board.mk | STM32L152 toolchain/flags/sources for builds. |
| boards/stm32l152re_nucleo/board.h | STM32L152 board API + global device instances. |
| boards/stm32l152re_nucleo/board.c | STM32L152 device instantiation + Board_Init/Deinit. |
| boards/README.md | Board BSP conventions and supported boards list. |
| README.md | Top-level repository overview and navigation. |
| .gitignore | Ignore rules for build artifacts and generated files. |
Comments suppressed due to low confidence (7)
wolfHAL/regmap.h:1
- The regmap helpers claim 32-bit register access but use
size_tfor both addresses and register values, and cast tovolatile size_t*. On 64-bit hosts or targets wheresize_tisn’t 32-bit, this can perform incorrect-width MMIO reads/writes and break hardware access. Recommend usinguintptr_tfor MMIO addresses (base,offset) anduint32_t(or explicitly sized types) for register values/pointers (e.g.,volatile uint32_t*), with separate helpers if 8/16/64-bit registers are needed.
wolfHAL/regmap.h:1 - The regmap helpers claim 32-bit register access but use
size_tfor both addresses and register values, and cast tovolatile size_t*. On 64-bit hosts or targets wheresize_tisn’t 32-bit, this can perform incorrect-width MMIO reads/writes and break hardware access. Recommend usinguintptr_tfor MMIO addresses (base,offset) anduint32_t(or explicitly sized types) for register values/pointers (e.g.,volatile uint32_t*), with separate helpers if 8/16/64-bit registers are needed.
wolfHAL/regmap.h:1 - The regmap helpers claim 32-bit register access but use
size_tfor both addresses and register values, and cast tovolatile size_t*. On 64-bit hosts or targets wheresize_tisn’t 32-bit, this can perform incorrect-width MMIO reads/writes and break hardware access. Recommend usinguintptr_tfor MMIO addresses (base,offset) anduint32_t(or explicitly sized types) for register values/pointers (e.g.,volatile uint32_t*), with separate helpers if 8/16/64-bit registers are needed.
src/uart/stm32wb_uart.c:1 - UART transmit writes are implemented as a read-modify-write (
whal_Reg_Update) targetingUART_TDR_REG, which is typically write-only on STM32 USARTv2. A RMW can read an undefined value (or fault) and is not appropriate for FIFO/data registers. Additionally, the loop never waits for TX space (e.g., TXE/TXFNF) before writing the next byte, so it can overrun/overwrite the transmit register/FIFO depending on the peripheral. Use a pure write helper for TDR, poll a ‘TX ready’ flag before each write, and (optionally) poll TC once after the entire buffer to ensure completion.
src/timer/systick.c:1 - SysTick reload semantics are typically ‘RELOAD = (ticks_per_period - 1)’. Writing
cyclesPerTickdirectly yields a period ofcyclesPerTick + 1cycles. Either rename/document the config field as the raw reload value, or programcyclesPerTick - 1(with a defined behavior forcyclesPerTick == 0) socyclesPerTickmatches the name and expected timing.
tests/test.h:1 - Negating
valcan overflow forINT_MIN(undefined behavior in C). This can corrupt output or crash when printing the most-negative int. A robust fix is to convert using an unsigned magnitude (or a wider signed type) without ever doingval = -valonint.
wolfHAL/error.h:1 - The comment states
whal_Erroris a signed 16-bit type, but the code usesint(commonly 32-bit on the embedded toolchains shown). Either update the comment to match reality (signed int) or change the typedef to an explicitly-sized type (e.g.,int16_t) if a 16-bit ABI is required.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 82 out of 83 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 82 out of 83 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+211
to
+215
| /* Validate address alignment and bounds */ | ||
| if (addr & 0xf || addr < cfg->startAddr || addr + dataSz > cfg->startAddr + cfg->size) { | ||
| return WHAL_EINVAL; | ||
| } | ||
|
|
Comment on lines
+110
to
+118
| #if !defined(WHAL_CFG_STM32WB_GPIO_DIRECT_API_MAPPING) && \ | ||
| !defined(WHAL_CFG_STM32F4_GPIO_DIRECT_API_MAPPING) && \ | ||
| !defined(WHAL_CFG_STM32H5_GPIO_DIRECT_API_MAPPING) && \ | ||
| !defined(WHAL_CFG_STM32C0_GPIO_DIRECT_API_MAPPING) && \ | ||
| !defined(WHAL_CFG_STM32F0_GPIO_DIRECT_API_MAPPING) | ||
| /* | ||
| * @brief Driver instance for STM32 GPIO. | ||
| */ | ||
| extern const whal_GpioDriver whal_Stm32wb_Gpio_Driver; |
Comment on lines
+33
to
+40
| #if !defined(WHAL_CFG_STM32WB_UART_DIRECT_API_MAPPING) && \ | ||
| !defined(WHAL_CFG_STM32WB_UART_DMA_DIRECT_API_MAPPING) && \ | ||
| !defined(WHAL_CFG_STM32H5_UART_DIRECT_API_MAPPING) && \ | ||
| !defined(WHAL_CFG_STM32C0_UART_DIRECT_API_MAPPING) | ||
| /* | ||
| * @brief Polled UART driver. Implements Init, Deinit, Send, Recv. | ||
| */ | ||
| extern const whal_UartDriver whal_Stm32wb_Uart_Driver; |
Comment on lines
+81
to
+96
| whal_Error whal_Stm32f4_Uart_Send(whal_Uart *uartDev, const void *data, size_t dataSz); | ||
|
|
||
| /* | ||
| * @brief Receive a buffer over UART. | ||
| * | ||
| * @param uartDev UART device instance. | ||
| * @param data Receive buffer. | ||
| * @param dataSz Number of bytes to receive. | ||
| * | ||
| * @retval WHAL_SUCCESS Transfer completed. | ||
| * @retval WHAL_EINVAL Invalid arguments. | ||
| */ | ||
| whal_Error whal_Stm32f4_Uart_Recv(whal_Uart *uartDev, void *data, size_t dataSz); | ||
| #endif /* !WHAL_CFG_STM32F4_UART_DIRECT_API_MAPPING */ | ||
|
|
||
| #endif /* WHAL_STM32F4_UART_H */ |
Comment on lines
+49
to
+52
| whal_Reg_Update(reg->base, SYSTICK_RVR_REG, | ||
| SYSTICK_RVR_RELOAD_Msk, | ||
| whal_SetBits(SYSTICK_RVR_RELOAD_Msk, SYSTICK_RVR_RELOAD_Pos, cfg->cyclesPerTick)); | ||
|
|
Comment on lines
+126
to
+130
| for (size_t i = 0; i < dataSz; ++i) { | ||
| whal_Error err; | ||
| whal_Reg_Update(reg->base, UART_TDR_REG, UART_TDR_Msk, | ||
| whal_SetBits(UART_TDR_Msk, UART_TDR_Pos, buf[i])); | ||
|
|
Comment on lines
+211
to
+237
| /* Validate address alignment and bounds */ | ||
| if (addr & 0xf || addr < cfg->startAddr || addr + dataSz > cfg->startAddr + cfg->size) { | ||
| return WHAL_EINVAL; | ||
| } | ||
|
|
||
| /* Check if flash is busy or suspended */ | ||
| whal_Reg_Get(regmap->base, FLASH_SR_REG, FLASH_SR_BSY_Msk, FLASH_SR_BSY_Pos, &bsy); | ||
| whal_Reg_Get(regmap->base, FLASH_SR_REG, FLASH_SR_PESD_Msk, FLASH_SR_PESD_Pos, &pesd); | ||
|
|
||
| if (bsy || pesd) { | ||
| return WHAL_ENOTREADY; | ||
| } | ||
|
|
||
| /* Clear all error flags by writing 1 to each */ | ||
| whal_Reg_Update(regmap->base, FLASH_SR_REG, FLASH_SR_ALL_ERR, 0xffffffff); | ||
|
|
||
| whal_Error err = WHAL_SUCCESS; | ||
|
|
||
| if (write) { | ||
| /* Enable flash programming mode */ | ||
| whal_Reg_Update(regmap->base, FLASH_CR_REG, FLASH_CR_PG_Msk, 1); | ||
|
|
||
| /* Program data in 64-bit (8 byte) double-word chunks */ | ||
| for (size_t i = 0; i < dataSz; i += 8) { | ||
| uint32_t *flashAddr = (uint32_t *)(addr + i); | ||
| uint32_t *dataAddr = (uint32_t *)(data + i); | ||
|
|
|
|
||
| /* Wait for erase to complete */ | ||
| err = whal_Reg_ReadPoll(regmap->base, FLASH_SR_REG, | ||
| FLASH_SR_CFGBSY_Msk, 0, cfg->timeout); |
Comment on lines
+36
to
+84
| static inline void whal_Reg_Update(size_t base, size_t offset, size_t mask, | ||
| size_t value) | ||
| { | ||
| volatile size_t *reg = (size_t *)(base + offset); | ||
| *reg = (*reg & ~mask) | (value & mask); | ||
| } | ||
|
|
||
| /* | ||
| * @brief Read a masked field from a memory-mapped register. | ||
| * | ||
| * @param base Base address of the register block. | ||
| * @param offset Byte offset from @p base to the register. | ||
| * @param msk Bit mask selecting the field to extract. | ||
| * @param pos Bit position of the field's LSB. | ||
| * @param value Output storage for the decoded field. | ||
| * | ||
| * @note No return value. Callers are responsible for passing valid inputs. | ||
| */ | ||
| static inline void whal_Reg_Get(size_t base, size_t offset, size_t msk, | ||
| size_t pos, size_t *value) | ||
| { | ||
| size_t val = *(volatile size_t *)(base + offset); | ||
| *value = whal_GetBits(msk, pos, val); | ||
| } | ||
|
|
||
| /* | ||
| * @brief Write a 32-bit value to a memory-mapped register. | ||
| * | ||
| * @param base Base address of the register block. | ||
| * @param offset Byte offset from @p base to the register. | ||
| * @param value Value to write. | ||
| */ | ||
| static inline void whal_Reg_Write(size_t base, size_t offset, size_t value) | ||
| { | ||
| *(volatile size_t *)(base + offset) = value; | ||
| } | ||
|
|
||
| /* | ||
| * @brief Read a 32-bit value from a memory-mapped register. | ||
| * | ||
| * @param base Base address of the register block. | ||
| * @param offset Byte offset from @p base to the register. | ||
| * | ||
| * @return The register value. | ||
| */ | ||
| static inline size_t whal_Reg_Read(size_t base, size_t offset) | ||
| { | ||
| return *(volatile size_t *)(base + offset); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wolfHAL design review
This PR is not for merging. It's a simplified stripped-down version of the wolfHAL
main branch opened against an empty parent branch (
tmp) so you guys canread the codebase top-to-bottom as a single additive diff (~10k lines)
rather than navigating around an existing tree. The actual code already
lives on
main.Purpose of wolfHAL
wolfHAL is a hardware abstraction layer meant to consolidate the
driver-porting work that today gets duplicated across our codebases
wolfCrypt, wolfBoot, wolfHSM, wolfIP, and any other project in the future that needs to
talk to hardware. Right now each project carries its own porting layer,
so adding a new platform means re-implementing roughly the same drivers
multiple times in slightly different shapes. wolfHAL is the single
target for that work: port a platform once, get it everywhere.
The design requirements:
against
whal_Gpio_Set,whal_Uart_Send,whal_Flash_Read, etc.,and that source compiles unchanged whether the underlying driver
is on-chip STM32WB GPIO, an external SPI NOR, or a Linux char
device. Moving a project to a new platform is a board-config
change, not a code change.
like the STM32C0 (32 KB flash, 12 KB RAM), wolfHAL has to disappear
into a minimal footprint.
enormously. pin assignments, clock trees, power sequencing,
on-chip vs. external peripherals, OS-mediated vs. bare-metal and
a user porting wolfHAL to their hardware should not need to edit
the driver source. All board-specific choices live in the board's
board.c(device instances, configuration structs, init order),build flags, and chip-specific helpers; driver code stays vanilla
and upstreamable.
How to read this PR
docs/getting_started.mdfor the conceptual modeldocs/writing_a_driver.mdfor the driver-category breakdown(covers the full peripheral catalog — most types aren't in this PR)
boards/stm32wb55xx_nucleo/board.cto see device/board configurationsrc/gpio/stm32wb_gpio.c) to see thevtable + DIRECT_API_MAPPING pattern
src/gpio/stm32l1_gpio.c)Patterns to look at
src/gpio/gpio.c,src/uart/uart.csrc/gpio/stm32wb_gpio.csrc/clock/stm32wb_rcc.c,src/power/stm32l1_pwr.cDIRECT_API_MAPPINGsrc/gpio/stm32wb_gpio.c(#ifdefblock)#includesrc/gpio/stm32l1_gpio.c,src/uart/stm32l1_uart.cExt_helpers for non-generic opswhal_Stm32wb_Flash_Ext_SetLatencyboards/stm32wb55xx_nucleo/board.cwolfHAL/timeout.h, used throughoutWhat's included
wolfHAL/<type>/<type>.h) for the devicetypes the demo boards exercise: gpio, uart, flash, timer, plus the
board-level handles (clock, power)
stm32wb55xx_nucleo(Cortex-M4, 64 MHz via PLL)stm32l152re_nucleo(Cortex-M3, 32 MHz via PLL + voltage scaling)on both and power on L1
#include "stm32wb_gpio.c"(register-compatible). L1 UART similarlyreuses the F4 UART driver. Different from the macro-based
DIRECT_API_MAPPINGaliasing.conventions
What's intentionally excluded
To keep the diff readable, the following are dropped from this snapshot
but exist on
mainand follow the same patterns:block, eth, eth_phy, sensor, ipc
"peripheral driver" category is documented in
docs/writing_a_driver.mdbut isn't physically demonstrated here