ESP-NOW Support - #3448
Conversation
|
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:
📝 WalkthroughWalkthroughAdds ESP-NOW support across build configuration, runtime type plumbing, native bindings, controller/event handling, and ESP32 preset defconfigs. ChangesESP-NOW feature plumbing
Sequence Diagram(s)sequenceDiagram
participant EspNowController
participant ESP_NOW
participant CLRRuntime
participant EspNowEventHandler
EspNowController->>ESP_NOW: initialize, send, or add peer
ESP_NOW->>EspNowController: send or receive callback
EspNowController->>CLRRuntime: post ESP-NOW event payload
CLRRuntime->>EspNowEventHandler: dispatch ProcessEvent
EspNowEventHandler->>CLRRuntime: return managed event object
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMake/Modules/FindnanoFramework.Espnow.cmake (1)
1-44: 🎯 Functional Correctness | 🟠 MajorRename the module file to
FindnanoFramework.EspNow.cmake.
PerformSettingsForApiEntry("nanoFramework.EspNow")drivesfind_package(nanoFramework.EspNow), so CMake looks forFindnanoFramework.EspNow.cmake. The current filenameFindnanoFramework.Espnow.cmakewill not be found on case-sensitive systems.🤖 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 `@CMake/Modules/FindnanoFramework.Espnow.cmake` around lines 1 - 44, The CMake find-module name is mismatched with the package name used by PerformSettingsForApiEntry("nanoFramework.EspNow"), so find_package(nanoFramework.EspNow) will not locate it on case-sensitive systems. Rename the module to match the expected FindnanoFramework.EspNow.cmake naming and keep the package-handling logic in this module aligned with nanoFramework.EspNow_INCLUDE_DIRS and nanoFramework.EspNow_SOURCES so CMake can discover it correctly.
🤖 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 `@Kconfig.apis`:
- Around line 105-109: The API_NANOFRAMEWORK_ESPNOW Kconfig option is missing
the runtime events dependency/selection needed by its managed sent/received
event plumbing. Update the config entry for API_NANOFRAMEWORK_ESPNOW so it also
enables API_NANOFRAMEWORK_RUNTIME_EVENTS, following the same pattern used by the
older event-driven APIs in the Kconfig file, and verify the ESP-NOW feature
cannot be selected without the runtime event support it requires.
In
`@targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpp`:
- Around line 115-138: The EspNowController send path uses dereferenced managed
arrays before validating them, so update the send method around esp_now_send()
to check peerMacArg, dataArg, and their lengths before any access. Ensure
peerMacArg has at least 6 bytes for the MAC, dataArg is non-null, and dataLen is
clamped/validated against dataArg->m_numOfElements before passing buffers to
esp_now_send(). Keep the validation in the same method that reads stack.Arg1(),
stack.Arg2(), and stack.Arg3().
- Around line 7-8: The ESP-NOW event queue is reusing shared static payload
storage in the native controller, so later callbacks can overwrite earlier
queued data before managed dispatch. Update
Library_nanoFramework_EspNow_native_nanoFramework_EspNow_EspNowController so
each enqueue in the send and receive callback paths owns its own stable payload
storage, using per-event allocation, a ring buffer, or another queue-backed
object that remains valid until the CLR consumes it. Make sure the
dataSentEventData and dataRecvEventData usage does not point multiple pending
events at the same mutable instance.
- Around line 73-83: The ESP-NOW initialization flow in EspNowController leaves
partial setup behind if callback registration fails after esp_now_init()
succeeds. Update the callback registration path around esp_now_register_recv_cb
and esp_now_register_send_cb so that any failure triggers cleanup of the
already-initialized ESP-NOW state before returning, reusing the existing
init/cleanup pattern used in this controller. Ensure the failure path resets the
half-initialized controller state consistently before setting the
ESPNOW_ERR_ESPNOW_INIT result.
In `@targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.h`:
- Around line 5-6: The include guard in nanoFramework_espnow_native.h is not
using the required SCREAMING_SNAKE_CASE and starts with a reserved-style leading
underscore. Update the guard macro at the top of the file and the matching
`#endif` comment to use NANOFRAMEWORK_ESPNOW_NATIVE_H consistently. Make sure the
change is applied in the header’s include guard definition and closing comment
so the symbol name matches throughout.
In `@targets/ESP32/defconfig/ESP32_C5_THREAD_defconfig`:
- Line 1: The generated header/preset metadata is stale: the
ESP32_C5_THREAD_defconfig file is still being labeled as ESP32_C5_UART. Update
the source consumed by preset_to_defconfig.py or the preset name used for this
defconfig so the generated comment/header reflects ESP32_C5_THREAD, and verify
the regeneration path for the Thread-specific preset.
In `@targets/ESP32/defconfig/ESP32_H2_THREAD_defconfig`:
- Line 15: Disable or gate the CONFIG_API_NANOFRAMEWORK_ESPNOW setting in the
ESP32_H2_THREAD defconfig, since this preset does not enable Wi‑Fi support.
Update the ESP32_H2_THREAD preset so it no longer unconditionally sets
CONFIG_API_NANOFRAMEWORK_ESPNOW=y, and instead only enables it on Wi‑Fi-capable
targets or removes it from this configuration entirely.
In `@targets/ESP32/defconfig/ESP32_P4_defconfig`:
- Line 16: Disable the ESP-NOW option in the generic ESP32-P4 preset by removing
the CONFIG_API_NANOFRAMEWORK_ESPNOW setting from ESP32_P4_defconfig; this preset
should not expose that feature unless the build is specifically targeting an
external Wi‑Fi companion setup. Update the preset entry in the
ESP32_P4_defconfig configuration so the default capability set matches the stock
ESP32-P4 hardware profile.
---
Outside diff comments:
In `@CMake/Modules/FindnanoFramework.Espnow.cmake`:
- Around line 1-44: The CMake find-module name is mismatched with the package
name used by PerformSettingsForApiEntry("nanoFramework.EspNow"), so
find_package(nanoFramework.EspNow) will not locate it on case-sensitive systems.
Rename the module to match the expected FindnanoFramework.EspNow.cmake naming
and keep the package-handling logic in this module aligned with
nanoFramework.EspNow_INCLUDE_DIRS and nanoFramework.EspNow_SOURCES so CMake can
discover it correctly.
🪄 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
Run ID: b78ef552-222c-464b-85de-2986a5b8d4b1
📒 Files selected for processing (48)
CMake/Modules/FindNF_NativeAssemblies.cmakeCMake/Modules/FindnanoFramework.Espnow.cmakeCMake/Modules/NF_Kconfig.cmakeKconfig.apissrc/CLR/Core/TypeSystem.cppsrc/CLR/Include/nanoCLR_Runtime.hsrc/HAL/Include/nanoHAL_v2.htargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.cpptargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.htargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpptargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpptargets/ESP32/defconfig/AtomS3_defconfigtargets/ESP32/defconfig/ESP32_BLE_REV0_defconfigtargets/ESP32/defconfig/ESP32_BLE_REV3_defconfigtargets/ESP32/defconfig/ESP32_C3_BETA_defconfigtargets/ESP32/defconfig/ESP32_C3_defconfigtargets/ESP32/defconfig/ESP32_C5_THREAD_defconfigtargets/ESP32/defconfig/ESP32_C61_defconfigtargets/ESP32/defconfig/ESP32_C6_THREAD_defconfigtargets/ESP32/defconfig/ESP32_ETHERNET_KIT_1.2_defconfigtargets/ESP32/defconfig/ESP32_GenericDisplay_REV0_defconfigtargets/ESP32/defconfig/ESP32_H2_THREAD_defconfigtargets/ESP32/defconfig/ESP32_LILYGO_defconfigtargets/ESP32/defconfig/ESP32_OLIMEX_WROVER_defconfigtargets/ESP32/defconfig/ESP32_OLIMEX_defconfigtargets/ESP32/defconfig/ESP32_P4_REV_LESS3_defconfigtargets/ESP32/defconfig/ESP32_P4_defconfigtargets/ESP32/defconfig/ESP32_PICO_defconfigtargets/ESP32/defconfig/ESP32_PSRAM_BLE_GenericGraphic_REV3_defconfigtargets/ESP32/defconfig/ESP32_PSRAM_REV0_defconfigtargets/ESP32/defconfig/ESP32_PSRAM_REV3_IPV6_defconfigtargets/ESP32/defconfig/ESP32_PSRAM_REV3_defconfigtargets/ESP32/defconfig/ESP32_PSRAM_XTAL26_REV0_defconfigtargets/ESP32/defconfig/ESP32_REV0_defconfigtargets/ESP32/defconfig/ESP32_REV3_defconfigtargets/ESP32/defconfig/ESP32_S2_defconfigtargets/ESP32/defconfig/ESP32_S3_OCTAL_defconfigtargets/ESP32/defconfig/ESP32_S3_QUAD_defconfigtargets/ESP32/defconfig/ESP32_WESP32_defconfigtargets/ESP32/defconfig/ESP32_WT32_ETH01_defconfigtargets/ESP32/defconfig/ESP_WROVER_KIT_defconfigtargets/ESP32/defconfig/LilygoTWatch2020_defconfigtargets/ESP32/defconfig/LilygoTWatch2021_defconfigtargets/ESP32/defconfig/M5Core2_defconfigtargets/ESP32/defconfig/M5Core_defconfigtargets/ESP32/defconfig/M5Stack_defconfigtargets/ESP32/defconfig/M5StickCPlus_defconfigtargets/ESP32/defconfig/M5StickC_defconfig
josesimoes
left a comment
There was a problem hiding this comment.
This is a great start. I'll probably have more comments after reviewing the managed PR.
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/ESP32/defconfig/ESP32_C5_THREAD_defconfig`:
- Line 1: The defconfig header contains a UTF-8 BOM before the leading comment,
so the first line is not a plain comment. Remove the BOM from the file so the
ESP32_C5_THREAD defconfig starts with a normal “#” header and avoids
generated-file churn.
🪄 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
Run ID: 50a90cca-649a-46ea-89d8-6b8bb7d269a1
📒 Files selected for processing (1)
targets/ESP32/defconfig/ESP32_C5_THREAD_defconfig
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp`:
- Around line 10-11: The file-scope constants in
EspNowController__EspNowEventHandler.cpp do not follow the native code naming
convention, so rename EVENT_ESPNOW_DATASENT and EVENT_ESPNOW_DATARECV to use the
c_ prefix. Update their declarations and any references in the same translation
unit to match the new names, keeping the change consistent with the repo’s
constant naming rule for .cpp files.
In
`@targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpp`:
- Around line 7-8: The file-scope event constants in the EspNow controller do
not follow the repository’s C/C++ naming convention. Rename the constants in
EspNowController.cpp from EVENT_ESPNOW_DATASENT and EVENT_ESPNOW_DATARECV to use
the required c_ prefix, and update any references within the same translation
unit to match the new names so the symbols remain consistent.
🪄 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
Run ID: 7415f80c-f604-43af-84d3-e929d94e2a60
📒 Files selected for processing (6)
targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.htargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpptargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpptargets/ESP32/defconfig/ESP32_H2_THREAD_defconfigtargets/ESP32/defconfig/ESP32_P4_REV_LESS3_defconfigtargets/ESP32/defconfig/ESP32_P4_defconfig
💤 Files with no reviewable changes (3)
- targets/ESP32/defconfig/ESP32_H2_THREAD_defconfig
- targets/ESP32/defconfig/ESP32_P4_REV_LESS3_defconfig
- targets/ESP32/defconfig/ESP32_P4_defconfig
| static const int EVENT_ESPNOW_DATASENT = 1; | ||
| static const int EVENT_ESPNOW_DATARECV = 2; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rename these file-scope constants to use the repo prefix.
EVENT_ESPNOW_DATASENT and EVENT_ESPNOW_DATARECV do not follow the repository naming rule for constants in .cpp files. Rename them with the c_ prefix for consistency with the rest of the native code. As per coding guidelines, **/*.{c,cpp}: Prefix constants with 'c_'.
🤖 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/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp`
around lines 10 - 11, The file-scope constants in
EspNowController__EspNowEventHandler.cpp do not follow the native code naming
convention, so rename EVENT_ESPNOW_DATASENT and EVENT_ESPNOW_DATARECV to use the
c_ prefix. Update their declarations and any references in the same translation
unit to match the new names, keeping the change consistent with the repo’s
constant naming rule for .cpp files.
Source: Coding guidelines
| static const int EVENT_ESPNOW_DATASENT = 1; | ||
| static const int EVENT_ESPNOW_DATARECV = 2; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Rename these file-scope constants to use the repo prefix.
EVENT_ESPNOW_DATASENT and EVENT_ESPNOW_DATARECV do not follow the C/C++ constant naming rule used in this repository. Rename them with the c_ prefix before more call sites depend on the current spellings. As per coding guidelines, **/*.{c,cpp}: Prefix constants with 'c_'.
🤖 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/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpp`
around lines 7 - 8, The file-scope event constants in the EspNow controller do
not follow the repository’s C/C++ naming convention. Rename the constants in
EspNowController.cpp from EVENT_ESPNOW_DATASENT and EVENT_ESPNOW_DATARECV to use
the required c_ prefix, and update any references within the same translation
unit to match the new names so the symbols remain consistent.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp (2)
79-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
PushValueAndClearconsistently in both event handlers.
ProcessEvent_DataSent(line 48) usesstack.PushValueAndClear()whileProcessEvent_DataRecv(line 89) usesstack.PushValue(). Both construct a fresh managed object viaNewObjectFromIndex, so the clear is harmless in both and ensures no stale data lingers. UsePushValueAndClearinProcessEvent_DataRecvfor consistency.♻️ Proposed fix
// create nanoFramework.EspNow.EspNowController.DataRecvEventInternal instance to return - CLR_RT_HeapBlock &clrRet = stack.PushValue(); + CLR_RT_HeapBlock &clrRet = stack.PushValueAndClear();🤖 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/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp` around lines 79 - 123, ProcessEvent_DataRecv is inconsistent with ProcessEvent_DataSent because it uses stack.PushValue() instead of PushValueAndClear() before creating the managed event object. Update ProcessEvent_DataRecv in Library_nanoFramework_EspNow_native_nanoFramework_EspNow_EspNowControllerEventListener to use PushValueAndClear like ProcessEvent_DataSent, since both paths allocate a fresh object with NewObjectFromIndex and should clear any stale stack data for consistency.
13-36: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
CopyByteArrayToCLRArrayagainst zero-lengthmemcpy.When
lengthis 0,CLR_RT_HeapBlock_Array::CreateInstancemay produce a zero-element array whoseGetFirstElement()returnsNULL, makingmemcpy(NULL, src, 0)technically undefined behavior. A peer sending an empty ESP-NOW packet would reach this path. Add an early return for zero-length.🛡️ Proposed fix
HRESULT Library_nanoFramework_EspNow_native_nanoFramework_EspNow_EspNowControllerEventListener::CopyByteArrayToCLRArray( CLR_RT_HeapBlock &target, uint8_t *src, CLR_UINT32 length) { NANOCLR_HEADER(); + if (length == 0) + { + NANOCLR_CHECK_HRESULT( + CLR_RT_HeapBlock_Array::CreateInstance(target, 0, g_CLR_RT_WellKnownTypes.m_UInt8)); + NANOCLR_NOCLEANUP(); + } + CLR_UINT8 *firstTargetByte; CLR_RT_HeapBlock_Array *targetArray; NANOCLR_CHECK_HRESULT(CLR_RT_HeapBlock_Array::CreateInstance(target, length, g_CLR_RT_WellKnownTypes.m_UInt8));🤖 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/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp` around lines 13 - 36, `CopyByteArrayToCLRArray` needs a zero-length guard before copying data. In `Library_nanoFramework_EspNow_native_nanoFramework_EspNow_EspNowControllerEventListener::CopyByteArrayToCLRArray`, check `length` immediately after the array allocation path and return success without calling `GetFirstElement()` or `memcpy` when it is 0. Keep the normal copy path unchanged for non-empty payloads so empty ESP-NOW packets are handled safely.
🤖 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.
Outside diff comments:
In
`@targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp`:
- Around line 79-123: ProcessEvent_DataRecv is inconsistent with
ProcessEvent_DataSent because it uses stack.PushValue() instead of
PushValueAndClear() before creating the managed event object. Update
ProcessEvent_DataRecv in
Library_nanoFramework_EspNow_native_nanoFramework_EspNow_EspNowControllerEventListener
to use PushValueAndClear like ProcessEvent_DataSent, since both paths allocate a
fresh object with NewObjectFromIndex and should clear any stale stack data for
consistency.
- Around line 13-36: `CopyByteArrayToCLRArray` needs a zero-length guard before
copying data. In
`Library_nanoFramework_EspNow_native_nanoFramework_EspNow_EspNowControllerEventListener::CopyByteArrayToCLRArray`,
check `length` immediately after the array allocation path and return success
without calling `GetFirstElement()` or `memcpy` when it is 0. Keep the normal
copy path unchanged for non-empty payloads so empty ESP-NOW packets are handled
safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: f32c87a6-06c0-4116-ab20-b5fc377dd5f3
📒 Files selected for processing (3)
targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.cpptargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.htargets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist