Add network support for ST_B_L475E_IOT01A (through its ISM43362 module) - #3511
Open
Ellerbach wants to merge 28 commits into
Open
Add network support for ST_B_L475E_IOT01A (through its ISM43362 module)#3511Ellerbach wants to merge 28 commits into
Ellerbach wants to merge 28 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 SummarySummary by CodeRabbit
WalkthroughThe change adds Inventek ISM43362 Wi-Fi support for the ST_B_L475E_IOT01A target. It includes the driver, SPI transport, socket proxy, synchronous SNTP, network integration, target configuration, flash alignment, and documentation. ChangesISM43362 Wi-Fi and target integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant WifiAdapter
participant TargetNetwork
participant WIFI_API
participant ISM43362
participant SNTP
WifiAdapter->>TargetNetwork: connect and scan
TargetNetwork->>WIFI_API: initialize and configure module
WIFI_API->>ISM43362: exchange Wi-Fi commands over SPI
ISM43362-->>WIFI_API: return connection and network state
WIFI_API-->>TargetNetwork: return mapped status
TargetNetwork->>SNTP: trigger synchronous time update
SNTP->>WIFI_API: resolve server and exchange UDP packet
WIFI_API-->>SNTP: return NTP response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 39
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/DeviceInterfaces/Networking.Sntp/nf_networking_sntp_nanoFramework_Networking_Sntp.cpp`:
- Around line 8-18: Make the configured SNTP server names used by
set_Server1/set_Server2 available to Ism43362_Sntp_TriggerAutoSync(), replacing
its hardcoded pool.ntp.org and time.nist.gov values with s_ism43362SntpServer1
and s_ism43362SntpServer2 or a shared default configuration. Preserve the
existing defaults and ensure automatic sync reflects later application changes
to both server settings.
- Around line 20-31: Update the SNTP state handling in Start___STATIC__VOID and
the corresponding IsStarted implementation so the reported started state tracks
whether Start() was invoked, independently of the Ism43362_Sntp_Sync result.
Preserve s_ism43362SntpStarted for last-query success if needed, but introduce
or reuse separate start-state storage and ensure IsStarted returns that state
consistently after temporary network failures.
In `@targets/ChibiOS/_common/Target_Network.cpp`:
- Around line 241-251: Update the Network_Interface_Connect_Result path around
the s_sntpAutoSyncTriggered guard so Ism43362_Sntp_TriggerAutoSync() does not
run synchronously inside the managed connect poll loop. Defer the trigger until
after the connection result has been returned, or otherwise ensure its DNS and
UDP work cannot consume the connect timeout; preserve the one-time trigger
behavior.
- Around line 220-239: The Network_Interface_Connect_Result() synchronization
always updates Configs[0] and ignores configIndex. Use configIndex to select the
target configuration, validate it against NetworkInterfaceConfigs->Count, and
return an error when it is out of range before assigning IPv4Address,
IPv4NetMask, or IPv4GatewayAddress.
In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp`:
- Around line 493-504: Restrict the BlockStorageDevice_EraseBlock calls in the
Network and Wireless80211 store paths to the initial-allocation case by tracking
allocation state in the earlier configuration branches. For existing
configuration data or shared blocks, use
ConfigurationManager_UpdateConfigurationBlock() so adjacent records remain
intact, and stop the operation when erasure fails.
- Around line 762-768: Update the erase logic in the configuration-sector path
around BlockStorageDevice_GetDeviceInfo and BlockStorageDevice_EraseBlock to
enumerate every physical block whose address range intersects
__nanoConfig_start__ through __nanoConfig_end__. Align the first erase address
down to the containing block boundary, advance using each applicable region’s
actual block size, and erase each intersecting block start exactly once,
including sectors that begin mid-block or span differently sized blocks.
In `@targets/ChibiOS/_common/targetHAL_Network.cpp`:
- Around line 53-60: Update the WIFI_Init() handling in the
TARGET_HAS_WIFI_ISM43362 initialization block to check initStatus and log an
initialization failure, including the returned status when available. Remove the
silent (void)initStatus discard while preserving normal startup behavior when
initialization succeeds.
In
`@targets/ChibiOS/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp`:
- Around line 222-232: Capture the return status from WIFI_ListAccessPoints() in
the TARGET_HAS_WIFI_ISM43362 branch and propagate a failed scan to the managed
caller instead of using apList.count as a successful empty result. Update the
surrounding scan flow in the relevant WifiAdapter method to preserve the
existing result handling for successful scans while surfacing the API error
status.
- Around line 149-152: Update the WiFi connection wait logic around
Network_Interface_Connect_Result and WaitEvents so ISM43362 uses a short polling
timeout, allowing repeated connectivity checks until WIFI_IsConnected and IP
configuration succeed. Preserve the existing timeout behavior for other targets
and ensure the connection status still reports timeout only after the overall
wait expires.
In `@targets/ChibiOS/_WiFi/inventek/es_wifi_conf.h`:
- Line 65: Set ES_WIFI_USE_PING to 0 in the Wi-Fi configuration unless a caller
invokes ES_WIFI_Ping(); keep the optional feature flags unchanged and avoid
compiling unused ping support to preserve flash space.
In `@targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.c`:
- Line 65: Gate the ISM43362_DebugPrintf calls in SPI_WIFI_Init and the reset
and receive-timeout paths behind a compile-time trace macro. Make the macro
expand to nothing when CONFIG_NF_CLR_NO_TRACE is enabled, while preserving the
existing logging calls when tracing is enabled.
- Around line 21-31: Give the translation-unit-only variables txBuffer,
rxBuffer, spiDriver, and spiConfiguration static linkage and rename them with
the s_ prefix, updating all references. Rename the externally linked WiFiMutex
consistently to use the g_ prefix and update its declaration in es_wifi_conf.h.
Also rename cmddataRdyEdgeCount to use the s_ prefix and update every reference.
- Around line 204-214: Replace wrap-unsafe absolute deadline arithmetic with
unsigned elapsed-time checks in all three sites: in
targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.c lines 204-214, update
wait_cmddata_rdy_high to retain a start tick and compare elapsed GetTick() time
against timeout; in lines 147-168, update SPI_WIFI_ResetModule to use drainStart
and compare elapsed time against 1000; in lines 241-254, update
wait_cmddata_rdy_rising_event to use its existing startTick and remove
ticksToEnd. Preserve the existing timeout behavior and watchdog logic.
- Around line 349-374: Update SPI_WIFI_SendData so CS_UNSELECT executes on every
exit path after CS_SELECT, including successful sends, spiSend failures, and the
len <= 1 case. Ensure len <= 1 does not report success without transmitting;
preserve the existing SPI failure return while returning the appropriate result
only after the chip select is released.
- Around line 344-347: Update the ready-line timeout check in the command
transmission flow around wait_cmddata_rdy_high to compare its result against
ES_WIFI_STATUS_OK, matching SPI_WIFI_ResetModule(). Return
ES_WIFI_ERROR_SPI_FAILED for any non-OK status before asserting CS or
transmitting the command.
- Line 239: Declare wait_cmddata_rdy_rising_event with static, matching
wait_cmddata_rdy_high, while leaving SPI_WIFI_ResetModule, SPI_WIFI_Init,
SPI_WIFI_ReceiveData, and SPI_WIFI_SendData unchanged with their required
external linkage or usage.
- Around line 195-200: Update SPI_WIFI_DeInit to disable the
LINE_ISM43362_CMDTRDY EXTI event before stopping the driver, preventing
CmdDataRdyEdgeCallback from running after deinitialization. Guard the
spiStop(spiDriver) call so it executes only when spiDriver is non-null, while
preserving the ES_WIFI_STATUS_OK return.
- Around line 353-366: Bound the input length in the send path before
memcpy(txBuffer, data, len), ensuring odd-length padding remains within the
1024-byte txBuffer by rejecting any request larger than 1023 bytes. Preserve the
existing spiSend flow for valid lengths and return the established failure
result for oversized requests.
- Around line 149-173: Update the buffer-overflow guard in the drain loop around
rxBuffer and count to derive its limit from sizeof(rxBuffer) instead of the
literal 200, while preserving the existing batch-read behavior and timeout
handling.
- Around line 63-70: Update SPI_WIFI_Init so the ES_WIFI_RESET path validates
spiDriver before calling SPI_WIFI_ResetModule or reading the reset prompt;
return ES_WIFI_STATUS_ERROR when spiDriver is NULL, while preserving the
existing initialized flow.
- Around line 303-330: Update the receive loop in the transport function
containing WIFI_IS_CMDDATA_READY so it never writes beyond the requested len:
require space for both bytes before writing, and handle an odd remaining length
by buffering the final frame in tmp and copying only the remaining byte. Also
confirm that ES_WIFI_ReceiveData and all callers never pass len == 0; otherwise
remove the unbounded !len path or replace it with a safe caller-buffer limit.
- Around line 286-298: Update the failure branch surrounding
wait_cmddata_rdy_rising_event() to return ES_WIFI_ERROR_WAITING_DRDY_RISING
instead of the falling-edge error code, and propagate the result of
SPI_WIFI_ResetModule() so reset failures are reported to the caller.
- Around line 100-105: In the CMDDATA_RDY setup, update the initialization
sequence so palSetLineCallback() associates CmdDataRdyEdgeCallback with
LINE_ISM43362_CMDTRDY before palEnableLineEvent() enables rising-edge events;
keep cmddataRdyEdgeCount reset before enabling event handling.
In `@targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.h`:
- Around line 18-25: Remove the trailing semicolon after while (0) in the
WIFI_RESET_MODULE macro, leaving the do-while wrapper responsible for statement
termination so it remains safe in conditional contexts.
- Around line 7-8: Replace the generic WIFI_IO_H include guard in the ChibiOS
header with a file-specific guard such as ES_WIFI_IO_CHIBIOS_H, updating both
the `#ifndef` and matching `#define` while preserving the corresponding closing
guard.
In `@targets/ChibiOS/_WiFi/inventek/es_wifi.c`:
- Around line 843-887: The lock/unlock calls are unbalanced on failure and
timeout paths. In targets/ChibiOS/_WiFi/inventek/es_wifi.c lines 843-887, update
AT_RequestSendData() to call UNLOCK_WIFI() before the returns at lines 847, 883,
and 886; in lines 2019-2098, add LOCK_WIFI() at ES_WIFI_WaitServerConnection()
entry and UNLOCK_WIFI() before its final timeout return.
- Line 1139: Replace the sprintf call using Obj->FW_Rev as its format string
with a bounded string-copy operation that copies the firmware revision into
Obj->CmdData, ensuring NUL termination and preventing writes beyond the
destination buffer.
- Around line 2019-2098: Add LOCK_WIFI() at the start of
ES_WIFI_WaitServerConnection() before entering its loop, and add UNLOCK_WIFI()
immediately before the final ES_WIFI_STATUS_TIMEOUT return. Preserve the
existing unlocks on successful and error paths so every lock acquisition remains
balanced.
- Around line 437-440: Update case 1 in AT_ParseSingleAP() to apply the same
empty-token guard as AT_ParseAP() before executing ptr[strlen(ptr) - 1] = 0.
Only remove the final character and copy the SSID when the token is non-empty,
preserving the existing parsing behavior for valid tokens.
In `@targets/ChibiOS/_WiFi/inventek/es_wifi.h`:
- Around line 1-13: Update the leading comments in es_wifi.h and es_wifi_conf.h
to begin with the standard .NET Foundation and Contributors copyright header,
followed by a Portions Copyright line for the existing STMicroelectronics vendor
content, matching the accepted pattern in es_wifi_io_chibios.h. Preserve the
vendor headers after the project header.
In `@targets/ChibiOS/_WiFi/inventek/sntp_ism43362.cpp`:
- Around line 133-151: Update the SNTP response validation before extracting
ntpSeconds to reject packets whose leap indicator is 3 or whose stratum field is
0, using the response header fields in packet and returning false with the
existing debug-reporting pattern. Only allow timestamp conversion and
SetSystemTimeFromUnixEpoch after these synchronization checks pass.
In `@targets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpp`:
- Around line 386-464: Ensure SOCK_ioctl(SOCK_FIONREAD) lookahead caching is
compatible with SOCK_recvfrom() on UDP sockets. Prefer restricting peekBuf
population to TCP sockets, or update SOCK_recvfrom() to consume cached data
before calling WIFI_ReceiveDataFrom(), so Socket.Available followed by
ReceiveFrom does not lose datagram bytes.
- Around line 344-378: In targets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpp
lines 344-378, update SOCK_getaddrinfo to document that its static result
storage supports only one resolution at a time, or protect s_ipAddr, s_sockAddr,
and s_addrInfo with the driver mutex. In
targets/ChibiOS/_WiFi/inventek/sntp_ism43362.cpp lines 65-101, update the SNTP
code to copy res->ai_addr into a local SOCK_sockaddr_in before assigning
sin_port, avoiding mutation of SOCK_getaddrinfo’s shared storage.
- Around line 254-292: Update the receive-length calculation in the socket
receive method containing requestLen to guard non-positive len values before
casting to uint16_t. Return the existing zero-byte result for len <= 0, and
preserve the ES_WIFI_PAYLOAD_SIZE upper clamp for positive lengths so
WIFI_ReceiveData receives only valid request sizes.
- Around line 512-558: Update the writefds and readfds loops in the select
implementation to avoid calling SOCK_FD_CLR while iterating fd_array with a
changing fd_count. Traverse the original entries using a stable count or index,
record the ready sockets, and rebuild or filter each SOCK_fd_set after traversal
so swapped entries cannot be skipped; preserve readyCount and
disconnected-socket handling.
In `@targets/ChibiOS/_WiFi/inventek/wifi.c`:
- Around line 174-233: Update WIFI_GetIP_Address and WIFI_GetIP_Mask to check
the cached EsWifiObj.NetSettings.IsConnected flag instead of calling
ES_WIFI_IsConnected(), matching WIFI_GetGateway_Address and avoiding extra CS
AT-command round trips. Preserve the existing memcpy operations and status
results when connected.
- Around line 541-562: Update WIFI_ReceiveData to explicitly map
ES_WIFI_STATUS_TIMEOUT to WIFI_STATUS_TIMEOUT before the generic error return,
while preserving the existing OK and ES_WIFI_STATUS_UNEXPECTED_CLOSED_SOCKET
handling so timeout callers can retry instead of receiving a connection-reset
result.
- Around line 88-96: Update the SSID copy in the AP population loop to limit
reads from esWifiAPs.AP[APCount].SSID to its ES_WIFI_MAX_SSID_NAME_SIZE + 1-byte
source capacity, then explicitly NUL-terminate APs->ap[APCount].SSID within its
destination buffer. Preserve the existing AP iteration and field assignments.
In `@targets/ChibiOS/ST_B_L475E_IOT01A/common/targetHAL_ConfigurationManager.cpp`:
- Around line 14-19: Update InitialiseNetworkDefaultConfig to check whether
pconfig is null before calling memset; return false immediately for a null
pointer, while preserving the existing zero-initialization and success behavior
for valid configurations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f7af8be1-3a12-4d43-88cc-5a5ab11b1742
📒 Files selected for processing (25)
CMake/Modules/FindNF_Network.cmakeCMake/Modules/FindlwIP.cmakeCMake/binutils.ChibiOS.cmakeREADME.mdsrc/DeviceInterfaces/Networking.Sntp/nf_networking_sntp_nanoFramework_Networking_Sntp.cpptargets/ChibiOS/ST_B_L475E_IOT01A/CMakeLists.txttargets/ChibiOS/ST_B_L475E_IOT01A/README.mdtargets/ChibiOS/ST_B_L475E_IOT01A/common/targetHAL_ConfigurationManager.cpptargets/ChibiOS/ST_B_L475E_IOT01A/defconfigtargets/ChibiOS/ST_B_L475E_IOT01A/nanoCLR/chconf.htargets/ChibiOS/ST_B_L475E_IOT01A/nanoCLR/main.ctargets/ChibiOS/_WiFi/inventek/es_wifi.ctargets/ChibiOS/_WiFi/inventek/es_wifi.htargets/ChibiOS/_WiFi/inventek/es_wifi_conf.htargets/ChibiOS/_WiFi/inventek/es_wifi_io.htargets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.ctargets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.htargets/ChibiOS/_WiFi/inventek/sntp_ism43362.cpptargets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpptargets/ChibiOS/_WiFi/inventek/wifi.ctargets/ChibiOS/_WiFi/inventek/wifi.htargets/ChibiOS/_common/Target_Network.cpptargets/ChibiOS/_common/targetHAL_ConfigurationManager.cpptargets/ChibiOS/_common/targetHAL_Network.cpptargets/ChibiOS/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@targets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpp`:
- Around line 208-209: Validate int lengths before uint16_t conversion in the
send path at targets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpp:208-209 and the
send-to path at targets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpp:706-707:
reject negative values and split or reject values exceeding UINT16_MAX, then
call WIFI_SendData and WIFI_SendDataTo only with validated uint16_t-sized
lengths using the same policy in both locations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a997cdc6-78ea-47cc-a29a-7ed07b7c4d6a
📒 Files selected for processing (7)
targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.ctargets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.htargets/ChibiOS/_WiFi/inventek/sntp_ism43362.cpptargets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpptargets/ChibiOS/_common/Target_Network.cpptargets/ChibiOS/_common/targetHAL_Network.cpptargets/ChibiOS/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp (2)
337-347: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the aligned Wireless80211 allocation before the write.
The rounded
existingSizecan leave less than oneHAL_Configuration_Wireless80211record in the configuration sector. This path has no__nanoConfig_end__capacity check. If that occurs, the write at Line 502 targets bytes outside the logical configuration sector. Reject the allocation before settingstorageAddress.Proposed bounds check
`#endif` + const uint32_t c_configSectorSize = + (uint32_t)&__nanoConfig_end__ - (uint32_t)&__nanoConfig_start__; + const uint32_t c_wirelessConfigSize = sizeof(HAL_Configuration_Wireless80211); + if (existingSize > c_configSectorSize || + c_wirelessConfigSize > c_configSectorSize - existingSize) + { + return FALSE; + } + storageAddress = (uint32_t)&__nanoConfig_start__ + existingSize; isInitialAllocation = TRUE;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp` around lines 337 - 347, Validate the aligned existingSize against the configuration sector capacity before assigning storageAddress in the Wireless80211 allocation path. Ensure enough space remains for one HAL_Configuration_Wireless80211 record, reject the allocation when it would exceed __nanoConfig_end__, and only then set storageAddress and isInitialAllocation.
337-344: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResolve the owning erase block before the initial write.
storageAddressis a logical record address. On RP targets, the 256-byte alignment is for flash programming, whileRP2040FlashDriver_EraseBlock()andRP2350FlashDriver_EraseBlock()only acceptBytesPerBlockboundary addresses. If the non-erased config region is not at that boundary,BlockStorageDevice_EraseBlock()returnsFALSEand the first Wireless80211 config write fails. Compute the containing erase block start from the region/block map before erasing; if the configuration sector can share blocks with existing data, preserve and rewrite the rest of the erased region instead of aligning the address itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp` around lines 337 - 344, Update the initial-write path in targetHAL_ConfigurationManager to resolve the containing erase-block start from the region/block map before calling BlockStorageDevice_EraseBlock or the RP2040FlashDriver_EraseBlock/RP2350FlashDriver_EraseBlock implementations. Do not treat the 256-byte programming alignment of existingSize as the erase address; when the configuration sector shares an erase block with existing data, preserve and rewrite the other data in that block while erasing from the block boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp`:
- Around line 731-744: Update the config-sector erase/update flow around
configSectorCopy and the eraseOk loop to preserve bytes outside the logical
__nanoConfig_start__/__nanoConfig_end__ range in every intersecting physical
erase block. Stage the complete first and last affected blocks, merge the
updated configuration into that staged data, and restore each full block after
erasing, including the single-block case; alternatively ensure the config-sector
bounds cover complete erase blocks and add boundary coverage.
In `@targets/ChibiOS/_WiFi/inventek/sntp_ism43362.cpp`:
- Around line 181-190: Rename the constant SNTP_AUTOSYNC_BUDGET_MS to use the
required c_ prefix, and update the corresponding reference in
Ism43362_Sntp_TriggerAutoSync. Preserve the existing timeout value and
comparison behavior.
---
Outside diff comments:
In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp`:
- Around line 337-347: Validate the aligned existingSize against the
configuration sector capacity before assigning storageAddress in the
Wireless80211 allocation path. Ensure enough space remains for one
HAL_Configuration_Wireless80211 record, reject the allocation when it would
exceed __nanoConfig_end__, and only then set storageAddress and
isInitialAllocation.
- Around line 337-344: Update the initial-write path in
targetHAL_ConfigurationManager to resolve the containing erase-block start from
the region/block map before calling BlockStorageDevice_EraseBlock or the
RP2040FlashDriver_EraseBlock/RP2350FlashDriver_EraseBlock implementations. Do
not treat the 256-byte programming alignment of existingSize as the erase
address; when the configuration sector shares an erase block with existing data,
preserve and rewrite the other data in that block while erasing from the block
boundary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96754b32-f1b2-41e4-92a3-248627adcce5
📒 Files selected for processing (5)
src/DeviceInterfaces/Networking.Sntp/nf_networking_sntp_nanoFramework_Networking_Sntp.cpptargets/ChibiOS/_WiFi/inventek/sntp_ism43362.cpptargets/ChibiOS/_common/Target_Network.cpptargets/ChibiOS/_common/targetHAL_ConfigurationManager.cpptargets/ChibiOS/_common/targetHAL_Network.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp (3)
750-755: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the previous configuration after an update failure.
If a later
BlockStorageDevice_EraseBlockcall fails, earlier blocks remain erased and the function returns failure without restoringconfigSectorCopy. IfBlockStorageDevice_Writefails after the erases succeed, the same data loss occurs.Use a transactional or redundant configuration slot. At minimum, implement a verified rollback for every affected physical block before returning failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp` around lines 750 - 755, Update the configuration update flow surrounding the erase loop and subsequent BlockStorageDevice_Write so failures preserve the previous configuration: retain backups of every affected physical block, verify each erase/write operation, and restore all previously modified blocks from those backups before returning failure. Ensure rollback also covers partial erase progress and write failure, while keeping the existing successful update path unchanged.
281-282: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not insert first-time records over existing configuration.
If later configuration records exist, the current placement is unsafe. The Wireless address is derived from the Network size only. The enumeration path creates Wireless configuration before it enumerates X509 records. The initial-allocation path can then write over, or erase the physical block containing, an existing certificate or Wireless record.
Scan all existing record types before selecting the address. Otherwise, migrate and repack the complete configuration sector. Do not treat a first record of one type as an isolated block.
Also applies to: 300-300, 336-347, 485-500
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp` around lines 281 - 282, Update the initial-allocation and placement logic surrounding isInitialAllocation and its related address/erase handling to scan all existing configuration record types before selecting a storage address. Base placement on the complete configuration sector, not only Network size or the first record of one type, so existing Wireless, X509, and other records are never overwritten or erased; migrate and repack the full sector when necessary.
746-748: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the address-selected region for config erasing.
This common path assumes
blockInfo->Regions[0]covers__nanoConfig_start__, but targets define__nanoConfig_start__from theconfigsection in target-specific linker scripts. If the config region is not region zero, or if config data can span regions with different block sizes, the erase stride and boundary handling can target the wrong blocks. Select the region withBlockStorageDevice_FindRegionFromAddress(device, (uint32_t)&__nanoConfig_start__, ...)and handle region boundaries explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp` around lines 746 - 748, Update the config erase setup around BlockStorageDevice_GetDeviceInfo to locate the region containing __nanoConfig_start__ via BlockStorageDevice_FindRegionFromAddress instead of assuming blockInfo->Regions[0]. Handle transitions across region boundaries explicitly, using each region’s block size and bounds when calculating erase blocks.
♻️ Duplicate comments (2)
targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.c (2)
192-202: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear and validate the stopped SPI driver.
After
SPI_WIFI_DeInit()stopsspiDriver,SPI_WIFI_Init(ES_WIFI_RESET)can callSPI_WIFI_ResetModule()with a pointer to a stopped driver. ClearspiDriverafterspiStop(). Reject a reset request when no initialized driver exists.Proposed fix
if (spiDriver != NULL) { spiStop(spiDriver); + spiDriver = NULL; }if (mode == ES_WIFI_INIT) { // initialization } + else if (spiDriver == NULL) + { + return ES_WIFI_STATUS_ERROR; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.c` around lines 192 - 202, Update SPI_WIFI_DeInit to set spiDriver to NULL immediately after spiStop(spiDriver). In SPI_WIFI_ResetModule, reject reset requests when spiDriver is NULL, preserving normal reset behavior only for an initialized driver.
307-316: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn only the bytes copied to the caller buffer.
For
len == 1, this code reads two SPI bytes but copies one byte. It then incrementslengthanddataby two. The function returns2for a one-byte request. Callers can treat more bytes as valid than fit in their buffer.Proposed fix
- data[0] = tmp[0]; - if ((!len) || ((length + 1) < len)) - { - data[1] = tmp[1]; - } - length += 2; - data += 2; + uint16_t bytesToCopy = + (len == 0) ? sizeof(tmp) : MIN((uint16_t)(len - length), (uint16_t)sizeof(tmp)); + + memcpy(data, tmp, bytesToCopy); + length += bytesToCopy; + data += bytesToCopy;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.c` around lines 307 - 316, Update the SPI receive length accounting in the surrounding function so the returned count reflects only bytes copied to the caller buffer: when len is 1, keep draining both SPI bytes but increment the reported length and advance the caller buffer by only one byte; preserve two-byte accounting for full 16-bit units and ensure the function never returns more than the requested len.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@targets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpp`:
- Around line 550-567: Update the readfds handling in the socket-select routine
so connected sockets are not automatically marked readable. For each valid
connected socket, use the existing SOCK_FIONREAD lookahead for TCP sockets and
add it to readfds only when cached or newly probed data is available, or when
the probe reports closure; increment readyCount only for descriptors actually
added.
In `@targets/ChibiOS/_WiFi/inventek/wifi.c`:
- Around line 570-579: Update WIFI_ReceiveData so it distinguishes empty receive
timeouts from malformed responses: have the lower-level receive path return a
distinct timeout-only status, map only that status to WIFI_STATUS_TIMEOUT, and
return WIFI_STATUS_ERROR for other ES_WIFI_STATUS_IO_ERROR cases. Preserve
zeroing RcvDatalen for timeout results and the existing retry behavior for
genuine timeouts.
---
Outside diff comments:
In `@targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp`:
- Around line 750-755: Update the configuration update flow surrounding the
erase loop and subsequent BlockStorageDevice_Write so failures preserve the
previous configuration: retain backups of every affected physical block, verify
each erase/write operation, and restore all previously modified blocks from
those backups before returning failure. Ensure rollback also covers partial
erase progress and write failure, while keeping the existing successful update
path unchanged.
- Around line 281-282: Update the initial-allocation and placement logic
surrounding isInitialAllocation and its related address/erase handling to scan
all existing configuration record types before selecting a storage address. Base
placement on the complete configuration sector, not only Network size or the
first record of one type, so existing Wireless, X509, and other records are
never overwritten or erased; migrate and repack the full sector when necessary.
- Around line 746-748: Update the config erase setup around
BlockStorageDevice_GetDeviceInfo to locate the region containing
__nanoConfig_start__ via BlockStorageDevice_FindRegionFromAddress instead of
assuming blockInfo->Regions[0]. Handle transitions across region boundaries
explicitly, using each region’s block size and bounds when calculating erase
blocks.
---
Duplicate comments:
In `@targets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.c`:
- Around line 192-202: Update SPI_WIFI_DeInit to set spiDriver to NULL
immediately after spiStop(spiDriver). In SPI_WIFI_ResetModule, reject reset
requests when spiDriver is NULL, preserving normal reset behavior only for an
initialized driver.
- Around line 307-316: Update the SPI receive length accounting in the
surrounding function so the returned count reflects only bytes copied to the
caller buffer: when len is 1, keep draining both SPI bytes but increment the
reported length and advance the caller buffer by only one byte; preserve
two-byte accounting for full 16-bit units and ensure the function never returns
more than the requested len.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e9d882d-1471-4d1a-96b7-1ca11c44c5fc
📒 Files selected for processing (9)
targets/ChibiOS/ST_B_L475E_IOT01A/common/targetHAL_ConfigurationManager.cpptargets/ChibiOS/_WiFi/inventek/es_wifi.ctargets/ChibiOS/_WiFi/inventek/es_wifi_conf.htargets/ChibiOS/_WiFi/inventek/es_wifi_io_chibios.ctargets/ChibiOS/_WiFi/inventek/sntp_ism43362.cpptargets/ChibiOS/_WiFi/inventek/sockets_ism43362.cpptargets/ChibiOS/_WiFi/inventek/wifi.ctargets/ChibiOS/_common/targetHAL_ConfigurationManager.cpptargets/ChibiOS/_nanoCLR/System.Device.Wifi/sys_dev_wifi_native_System_Device_Wifi_WifiAdapter.cpp
josesimoes
reviewed
Sep 8, 2026
josesimoes
left a comment
josesimoes
left a comment
Member
There was a problem hiding this comment.
Brilliant work getting this up and running! Well done.
Most review comments are removing comments, which are repeatedly explaning the same thing about the module and how it works with lwIP.
We can't have the source files bloated with comments explaining every other code line.
What is relevant and the reasoning (if complex or deserving an explanation) goes to the PR description/context/motivation.
Co-authored-by: José Simões <jose.simoes@eclo.solutions>
josesimoes
reviewed
Sep 9, 2026
josesimoes
reviewed
Sep 9, 2026
josesimoes
approved these changes
Sep 9, 2026
josesimoes
left a comment
josesimoes
left a comment
Member
There was a problem hiding this comment.
Indeed STM32L4 requires double word alignment for writting. I've added a code suggestion for that. Please check. All the rest is quite allright. Pre-approving 😉 !
josesimoes
added
the
Platform: ChibiOS
josesimoes
changed the title
Co-authored-by: José Simões <jose.simoes@eclo.solutions>
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
3 participants
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.
Description
Motivation and Context
How Has This Been Tested?
On a real device.
Screenshots
Types of changes
Checklist