Skip to content

ESP-NOW Support - #3448

Open
torbacz wants to merge 12 commits into
nanoframework:mainfrom
torbacz:espnbow
Open

ESP-NOW Support#3448
torbacz wants to merge 12 commits into
nanoframework:mainfrom
torbacz:espnbow

Conversation

@torbacz

@torbacz torbacz commented Jun 25, 2026

Copy link
Copy Markdown

Description

Motivation and Context

How Has This Been Tested?

  • Tested on ESP32-S3 - one sender and one receiver
  • Tested version without encryption and broadcast
  • Tested version with encryption and real mac addresses

Screenshots

image

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dev Containers (changes related with Dev Containers, has no impact on code or features)
  • Dependencies/declarations (update dependencies or assembly declarations and changes associated, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds ESP-NOW support across build configuration, runtime type plumbing, native bindings, controller/event handling, and ESP32 preset defconfigs.

Changes

ESP-NOW feature plumbing

Layer / File(s) Summary
Build switches and package discovery
CMake/Modules/FindNF_NativeAssemblies.cmake, CMake/Modules/FindnanoFramework.Espnow.cmake, CMake/Modules/NF_Kconfig.cmake, Kconfig.apis
Adds the EspNow CMake option, find-module, Kconfig mapping, and API configuration entry.
Runtime event types
src/HAL/Include/nanoHAL_v2.h, src/CLR/Include/nanoCLR_Runtime.h, src/CLR/Core/TypeSystem.cpp
Adds the ESP-NOW event category and well-known sent/received event type mappings.
Native bindings and registration
targets/ESP32/_nanoCLR/nanoFramework.Espnow/*native.*
Declares ESP-NOW native data layouts, CLR bindings, event handlers, and native assembly registration.
ESP-NOW controller runtime
targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpp
Implements initialization, disposal, sending, peer registration, and native send/receive callbacks.
Event dispatch and payload copy
targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp
Copies native payloads into managed event objects and dispatches ESP-NOW runtime subevents.
ESP32 preset enablement
targets/ESP32/defconfig/*_defconfig
Enables CONFIG_API_NANOFRAMEWORK_ESPNOW=y across ESP32 presets and normalizes selected header formatting.

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
Loading

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested labels: Area: Config-and-Build

Suggested reviewers: alberk8, josesimoes

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: adding ESP-NOW support.
Description check ✅ Passed The description is clearly related to the ESP-NOW feature work and testing included in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🟠 Major

Rename the module file to FindnanoFramework.EspNow.cmake.
PerformSettingsForApiEntry("nanoFramework.EspNow") drives find_package(nanoFramework.EspNow), so CMake looks for FindnanoFramework.EspNow.cmake. The current filename FindnanoFramework.Espnow.cmake will 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

📥 Commits

Reviewing files that changed from the base of the PR and between 156de71 and 62af28b.

📒 Files selected for processing (48)
  • CMake/Modules/FindNF_NativeAssemblies.cmake
  • CMake/Modules/FindnanoFramework.Espnow.cmake
  • CMake/Modules/NF_Kconfig.cmake
  • Kconfig.apis
  • src/CLR/Core/TypeSystem.cpp
  • src/CLR/Include/nanoCLR_Runtime.h
  • src/HAL/Include/nanoHAL_v2.h
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.cpp
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.h
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpp
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp
  • targets/ESP32/defconfig/AtomS3_defconfig
  • targets/ESP32/defconfig/ESP32_BLE_REV0_defconfig
  • targets/ESP32/defconfig/ESP32_BLE_REV3_defconfig
  • targets/ESP32/defconfig/ESP32_C3_BETA_defconfig
  • targets/ESP32/defconfig/ESP32_C3_defconfig
  • targets/ESP32/defconfig/ESP32_C5_THREAD_defconfig
  • targets/ESP32/defconfig/ESP32_C61_defconfig
  • targets/ESP32/defconfig/ESP32_C6_THREAD_defconfig
  • targets/ESP32/defconfig/ESP32_ETHERNET_KIT_1.2_defconfig
  • targets/ESP32/defconfig/ESP32_GenericDisplay_REV0_defconfig
  • targets/ESP32/defconfig/ESP32_H2_THREAD_defconfig
  • targets/ESP32/defconfig/ESP32_LILYGO_defconfig
  • targets/ESP32/defconfig/ESP32_OLIMEX_WROVER_defconfig
  • targets/ESP32/defconfig/ESP32_OLIMEX_defconfig
  • targets/ESP32/defconfig/ESP32_P4_REV_LESS3_defconfig
  • targets/ESP32/defconfig/ESP32_P4_defconfig
  • targets/ESP32/defconfig/ESP32_PICO_defconfig
  • targets/ESP32/defconfig/ESP32_PSRAM_BLE_GenericGraphic_REV3_defconfig
  • targets/ESP32/defconfig/ESP32_PSRAM_REV0_defconfig
  • targets/ESP32/defconfig/ESP32_PSRAM_REV3_IPV6_defconfig
  • targets/ESP32/defconfig/ESP32_PSRAM_REV3_defconfig
  • targets/ESP32/defconfig/ESP32_PSRAM_XTAL26_REV0_defconfig
  • targets/ESP32/defconfig/ESP32_REV0_defconfig
  • targets/ESP32/defconfig/ESP32_REV3_defconfig
  • targets/ESP32/defconfig/ESP32_S2_defconfig
  • targets/ESP32/defconfig/ESP32_S3_OCTAL_defconfig
  • targets/ESP32/defconfig/ESP32_S3_QUAD_defconfig
  • targets/ESP32/defconfig/ESP32_WESP32_defconfig
  • targets/ESP32/defconfig/ESP32_WT32_ETH01_defconfig
  • targets/ESP32/defconfig/ESP_WROVER_KIT_defconfig
  • targets/ESP32/defconfig/LilygoTWatch2020_defconfig
  • targets/ESP32/defconfig/LilygoTWatch2021_defconfig
  • targets/ESP32/defconfig/M5Core2_defconfig
  • targets/ESP32/defconfig/M5Core_defconfig
  • targets/ESP32/defconfig/M5Stack_defconfig
  • targets/ESP32/defconfig/M5StickCPlus_defconfig
  • targets/ESP32/defconfig/M5StickC_defconfig

Comment thread Kconfig.apis
Comment thread targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.h Outdated
Comment thread targets/ESP32/defconfig/ESP32_C5_THREAD_defconfig Outdated
Comment thread targets/ESP32/defconfig/ESP32_H2_THREAD_defconfig Outdated
Comment thread targets/ESP32/defconfig/ESP32_P4_defconfig Outdated
@josesimoes josesimoes added the Platform: ESP32 Everything related specifically with ESP32 platform label Jun 26, 2026

@josesimoes josesimoes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start. I'll probably have more comments after reviewing the managed PR.

Comment thread targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.h Outdated
Comment thread targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.h Outdated
Comment thread targets/ESP32/defconfig/ESP32_C5_THREAD_defconfig Outdated
Comment thread targets/ESP32/defconfig/ESP32_H2_THREAD_defconfig Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 62af28b and 99e3fb8.

📒 Files selected for processing (1)
  • targets/ESP32/defconfig/ESP32_C5_THREAD_defconfig

Comment thread targets/ESP32/defconfig/ESP32_C5_THREAD_defconfig
@torbacz
torbacz requested a review from josesimoes June 28, 2026 17:20

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 99e3fb8 and 229ecaf.

📒 Files selected for processing (6)
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.h
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController.cpp
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp
  • targets/ESP32/defconfig/ESP32_H2_THREAD_defconfig
  • targets/ESP32/defconfig/ESP32_P4_REV_LESS3_defconfig
  • targets/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

Comment on lines +10 to +11
static const int EVENT_ESPNOW_DATASENT = 1;
static const int EVENT_ESPNOW_DATARECV = 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

Comment on lines +7 to +8
static const int EVENT_ESPNOW_DATASENT = 1;
static const int EVENT_ESPNOW_DATARECV = 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 value

Use PushValueAndClear consistently in both event handlers.

ProcessEvent_DataSent (line 48) uses stack.PushValueAndClear() while ProcessEvent_DataRecv (line 89) uses stack.PushValue(). Both construct a fresh managed object via NewObjectFromIndex, so the clear is harmless in both and ensures no stale data lingers. Use PushValueAndClear in ProcessEvent_DataRecv for 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 win

Guard CopyByteArrayToCLRArray against zero-length memcpy.

When length is 0, CLR_RT_HeapBlock_Array::CreateInstance may produce a zero-element array whose GetFirstElement() returns NULL, making memcpy(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

📥 Commits

Reviewing files that changed from the base of the PR and between 229ecaf and 2229428.

📒 Files selected for processing (3)
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.cpp
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native.h
  • targets/ESP32/_nanoCLR/nanoFramework.Espnow/nanoFramework_espnow_native_nanoFramework_Espnow_EspNowController__EspNowEventHandler.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Platform: ESP32 Everything related specifically with ESP32 platform Status: under review Type: enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants