feat: add dual-board ESP-NOW architecture for Python script execution#125
Open
crispyberry wants to merge 1 commit intomainfrom
Open
feat: add dual-board ESP-NOW architecture for Python script execution#125crispyberry wants to merge 1 commit intomainfrom
crispyberry wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds ESP-NOW-based remote Python execution: Board B (MicroPython) receives chunked scripts over ESP-NOW, executes them, captures stdout/exceptions, and returns chunked results; Board A gains an ESP-NOW manager, CLI to set peer MAC, and a tool/CLI command to send scripts and receive results. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as BoardA CLI / Tool
participant Manager as ESP-NOW Manager
participant NVS as NVS
participant BoardB as BoardB Executor
rect rgba(100,150,200,0.5)
CLI->>NVS: read peer MAC (if needed)
CLI->>Manager: espnow_send_script(code, timeout)
Manager->>Manager: split code into chunks, build headers
Manager->>BoardB: SCRIPT_START + chunk
BoardB->>Manager: ACK (on_send_cb)
Manager->>BoardB: SCRIPT_CHUNK ... SCRIPT_END
BoardB->>Manager: ACKs
end
rect rgba(150,200,100,0.5)
BoardB->>BoardB: reassemble script
BoardB->>BoardB: exec(code) (capture stdout/exc)
end
rect rgba(200,150,100,0.5)
BoardB->>Manager: RESULT_START + chunk(s)
Manager->>Manager: reassemble result
Manager->>CLI: return result buffer
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Board A (MimiClaw) sends Python scripts via ESP-NOW to Board B (MicroPython executor) and receives execution results. This enables extensible skill execution without being limited by ESP32's C-only firmware environment. - Add espnow_manager module with chunked send/receive protocol - Add run_python tool for LLM agent to execute Python on Board B - Add set_espnow_peer CLI command and config_show/reset support - Add executor/ directory with MicroPython boot.py and main.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
crispyberry
force-pushed
the
feat/dual-board-espnow
branch
from
81dabd8 to
3957d67
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
main/espnow/espnow_manager.h (1)
23-33: Consider documenting the blocking behavior.The function blocks until the result is received or timeout expires. Adding a note like "Blocks until result is received" would help callers understand threading implications.
📝 Suggested documentation enhancement
/** * Send a Python script to Board B and wait for the execution result. + * This function blocks until the result is received or the timeout expires. * * `@param` script Null-terminated Python source code * `@param` result Buffer to receive execution output * `@param` result_size Size of result buffer * `@param` timeout_ms Max wait time in milliseconds * `@return` ESP_OK on success */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/espnow/espnow_manager.h` around lines 23 - 33, The comment should be expanded to document that espnow_send_script blocks the calling thread until a response is received or the timeout elapses; update the function comment (espnow_send_script) to explicitly state "Blocks until result is received or timeout expires" and note threading implications (caller should not call from time-sensitive/ISR contexts and should handle potential long waits). Also mention the timeout_ms behavior (maximum wait in ms) and return semantics on timeout to make expected behavior explicit.main/mimi_config.h (1)
124-124: Document the relationship betweenMIMI_ESPNOW_RESULT_BUFand caller buffer sizes.Per the context in
main/tools/tool_run_python.c,espnow_send_scriptuses this 4KB buffer internally regardless of theoutput_sizeparameter passed by callers. Meanwhile,agent_loop.callocates 8KB andserial_cli.callocates 4KB for tool output.This isn't a bug (no buffer overflow), but outputs exceeding 4KB will be silently truncated. Consider either:
- Adding a comment here explaining this is the effective maximum result size
- Having
espnow_send_scriptrespect the caller'sresult_sizeparameter📝 Suggested documentation
-#define MIMI_ESPNOW_RESULT_BUF 4096 +#define MIMI_ESPNOW_RESULT_BUF 4096 /* Max result size from Board B */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/mimi_config.h` at line 124, The constant MIMI_ESPNOW_RESULT_BUF (4096) is the effective limit used inside espnow_send_script regardless of callers' output_size/result_size, causing silent truncation when callers (e.g., agent_loop.c which allocates 8KB and serial_cli.c which allocates 4KB) request larger buffers; update main/mimi_config.h to document this behavior clearly by adding a concise comment next to the MIMI_ESPNOW_RESULT_BUF definition that states it is the maximum result size returned by espnow_send_script, mentions that callers passing output_size/result_size larger than this will be truncated to 4096, and suggests either increasing the constant or modifying espnow_send_script to honor the caller's requested size if needed.main/espnow/espnow_manager.c (1)
74-78: Sequence update occurs even if copy is skipped.On line 78,
s_result_seqis updated unconditionally after theifblock, but the copy only happens if the bounds check passes. If the buffer is too small atRESULT_START, the sequence advances without storing data, potentially causing later chunks to be accepted at wrong offsets.This is unlikely to occur in practice since
RESULT_STARTis the first message ands_result_lenis 0, but for consistency, consider moving the sequence update inside the conditional or using the same>=pattern asRESULT_CHUNK.♻️ Suggested fix for consistency
case ESPNOW_MSG_RESULT_START: { uint32_t total = data[3] | (data[4] << 8) | (data[5] << 16) | (data[6] << 24); s_result_total = total; s_result_len = 0; s_result_seq = 0; - if (payload_len > 0 && s_result_buf && s_result_len + payload_len < s_result_size) { + if (payload_len > 0 && s_result_buf && s_result_len + payload_len < s_result_size - 1) { memcpy(s_result_buf + s_result_len, payload, payload_len); s_result_len += payload_len; + s_result_seq = seq + 1; + } else if (payload_len == 0) { + s_result_seq = seq + 1; } - s_result_seq = seq + 1; ESP_LOGD(TAG, "Result start: total=%lu", (unsigned long)total); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/espnow/espnow_manager.c` around lines 74 - 78, The sequence number s_result_seq is advanced unconditionally after handling RESULT_START even when the payload copy is skipped; modify the logic so s_result_seq is only updated when the payload was actually appended (i.e., move the s_result_seq = seq + 1 assignment inside the same conditional that performs the memcpy and s_result_len update, or change the bounds check to mirror RESULT_CHUNK’s >= check and only advance when the copy succeeds) to ensure later chunks are validated against the correct offset (referencing s_result_buf, s_result_len, s_result_size, s_result_seq, and the RESULT_START/RESULT_CHUNK handling).main/cli/serial_cli.c (1)
506-509: Consider usingsizeoffor maintainability.The loop uses a hardcoded
6which must be manually updated whenever thenamespacesarray changes. Usingsizeof(namespaces)/sizeof(namespaces[0])would be more robust.♻️ Suggested fix
const char *namespaces[] = { MIMI_NVS_WIFI, MIMI_NVS_TG, MIMI_NVS_LLM, MIMI_NVS_PROXY, MIMI_NVS_SEARCH, MIMI_NVS_ESPNOW }; - for (int i = 0; i < 6; i++) { + for (size_t i = 0; i < sizeof(namespaces) / sizeof(namespaces[0]); i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/cli/serial_cli.c` around lines 506 - 509, The for-loop iterating over the const char *namespaces[] uses a hardcoded 6; update the loop bound to compute the array length dynamically (for example replace 6 with sizeof(namespaces) / sizeof(namespaces[0]) or an ARRAY_SIZE(namespaces) macro) so the loop in serial_cli.c (the namespaces array and the for (int i = 0; 6; ...) loop) automatically adapts when namespaces changes.executor/main.py (1)
125-128: Silent exception handling is acceptable here but could log for debugging.The
try-except-passwhen adding a peer is flagged by static analysis (S110), but it's reasonable since the peer may already exist. For debugging purposes, you could optionally log the exception.♻️ Optional: log the exception
try: e.add_peer(peer_mac) - except Exception: - pass # peer may already exist + except Exception as ex: + print('add_peer:', ex) # peer may already exist🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/main.py` around lines 125 - 128, The silent except around e.add_peer(peer_mac) should preserve behavior but record the failure for debugging: catch Exception as exc and log a short message (including peer_mac and exc) at debug or warning level using the module's/logger instance instead of a bare pass so failures to add an already-existing peer remain non-fatal but are visible in logs; keep the try/except around the call to e.add_peer to retain current control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@executor/main.py`:
- Around line 142-157: The MSG_SCRIPT_END branch should guard against spurious
packets by checking the receiving flag before processing; modify the
MSG_SCRIPT_END handling (where script_buf is used and execute_script/send_result
are called) to only append payload, decode, call execute_script, send_result,
and reset script_buf/receiving when receiving is True, otherwise ignore the
packet so you don't execute an empty or stale script buffer.
In `@executor/README.md`:
- Around line 42-54: Add language specifiers to the two fenced code blocks
containing the "Board B MAC: aa:bb:cc:dd:ee:ff" / "On Board A, run:
set_espnow_peer aa:bb:cc:dd:ee:ff" example and the "set_espnow_peer
aa:bb:cc:dd:ee:ff" / "restart" commands by changing the opening backticks from
``` to ```text (or ```console) so the blocks render consistently; update the
blocks that include the "Board B MAC" line and the block with "set_espnow_peer"
and "restart" accordingly.
In `@main/cli/serial_cli.c`:
- Around line 592-600: The code ignores the return value of nvs_set_blob so a
failed write can still be committed; change logic in the block that opens NVS
(nvs_open) to check esp_err_t from nvs_set_blob, handle errors by logging via
printf/esp_err_to_name(err) and skipping nvs_commit/nvs_close as appropriate (or
still close but return error), and only call nvs_commit(nvs) when nvs_set_blob
returns ESP_OK; use the same err variable (esp_err_t err) for these checks and
ensure nvs_close(nvs) always runs before returning.
In `@main/espnow/espnow_manager.c`:
- Around line 148-174: The init path leaks FreeRTOS objects: ensure
s_event_group and s_mutex are cleaned up on any subsequent failure and that if
one creation fails the other is deleted; after calling xEventGroupCreate() and
xSemaphoreCreateMutex() check each result and on error delete the
already-created object(s) and return ESP_ERR_NO_MEM, and likewise if
esp_now_init() or esp_now_add_peer() fails delete both s_event_group and s_mutex
before returning (and only call esp_now_deinit() where appropriate); reference
s_event_group, s_mutex, xEventGroupCreate, xSemaphoreCreateMutex, esp_now_init,
esp_now_add_peer and esp_now_deinit when implementing the cleanup.
In `@main/tools/tool_run_python.c`:
- Around line 14-18: The error message in tool_run_python.c incorrectly
references the CLI command 'set espnow_peer'; update the string passed to
snprintf so it names the correct command 'set_espnow_peer' (underscore, no
space). Locate the check using espnow_is_ready() in the function surrounding
this snippet and replace the command text in the output buffer message to
"Configure peer with 'set_espnow_peer XX:XX:XX:XX:XX:XX'".
---
Nitpick comments:
In `@executor/main.py`:
- Around line 125-128: The silent except around e.add_peer(peer_mac) should
preserve behavior but record the failure for debugging: catch Exception as exc
and log a short message (including peer_mac and exc) at debug or warning level
using the module's/logger instance instead of a bare pass so failures to add an
already-existing peer remain non-fatal but are visible in logs; keep the
try/except around the call to e.add_peer to retain current control flow.
In `@main/cli/serial_cli.c`:
- Around line 506-509: The for-loop iterating over the const char *namespaces[]
uses a hardcoded 6; update the loop bound to compute the array length
dynamically (for example replace 6 with sizeof(namespaces) /
sizeof(namespaces[0]) or an ARRAY_SIZE(namespaces) macro) so the loop in
serial_cli.c (the namespaces array and the for (int i = 0; 6; ...) loop)
automatically adapts when namespaces changes.
In `@main/espnow/espnow_manager.c`:
- Around line 74-78: The sequence number s_result_seq is advanced
unconditionally after handling RESULT_START even when the payload copy is
skipped; modify the logic so s_result_seq is only updated when the payload was
actually appended (i.e., move the s_result_seq = seq + 1 assignment inside the
same conditional that performs the memcpy and s_result_len update, or change the
bounds check to mirror RESULT_CHUNK’s >= check and only advance when the copy
succeeds) to ensure later chunks are validated against the correct offset
(referencing s_result_buf, s_result_len, s_result_size, s_result_seq, and the
RESULT_START/RESULT_CHUNK handling).
In `@main/espnow/espnow_manager.h`:
- Around line 23-33: The comment should be expanded to document that
espnow_send_script blocks the calling thread until a response is received or the
timeout elapses; update the function comment (espnow_send_script) to explicitly
state "Blocks until result is received or timeout expires" and note threading
implications (caller should not call from time-sensitive/ISR contexts and should
handle potential long waits). Also mention the timeout_ms behavior (maximum wait
in ms) and return semantics on timeout to make expected behavior explicit.
In `@main/mimi_config.h`:
- Line 124: The constant MIMI_ESPNOW_RESULT_BUF (4096) is the effective limit
used inside espnow_send_script regardless of callers' output_size/result_size,
causing silent truncation when callers (e.g., agent_loop.c which allocates 8KB
and serial_cli.c which allocates 4KB) request larger buffers; update
main/mimi_config.h to document this behavior clearly by adding a concise comment
next to the MIMI_ESPNOW_RESULT_BUF definition that states it is the maximum
result size returned by espnow_send_script, mentions that callers passing
output_size/result_size larger than this will be truncated to 4096, and suggests
either increasing the constant or modifying espnow_send_script to honor the
caller's requested size if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0f78c79-c957-4b29-ab53-d70f6091fb36
📒 Files selected for processing (12)
executor/README.mdexecutor/boot.pyexecutor/main.pymain/CMakeLists.txtmain/cli/serial_cli.cmain/espnow/espnow_manager.cmain/espnow/espnow_manager.hmain/mimi.cmain/mimi_config.hmain/tools/tool_registry.cmain/tools/tool_run_python.cmain/tools/tool_run_python.h
Comment on lines
+142
to
+157
| elif msg_type == MSG_SCRIPT_END: | ||
| if payload: | ||
| script_buf.extend(payload) | ||
|
|
||
| code = bytes(script_buf).decode('utf-8') | ||
| print('Received script ({} bytes), executing...'.format(len(code))) | ||
|
|
||
| result = execute_script(code) | ||
| print('Result ({} bytes): {}'.format(len(result), result[:100])) | ||
|
|
||
| if peer_mac: | ||
| send_result(peer_mac, result) | ||
|
|
||
| # Reset state | ||
| script_buf = bytearray() | ||
| receiving = False |
There was a problem hiding this comment.
Add receiving check for SCRIPT_END to prevent spurious execution.
SCRIPT_START and SCRIPT_CHUNK check the receiving flag, but SCRIPT_END does not. A stray SCRIPT_END packet could trigger execution of an empty or stale buffer.
🛡️ Proposed fix
- elif msg_type == MSG_SCRIPT_END:
+ elif msg_type == MSG_SCRIPT_END and receiving:
if payload:
script_buf.extend(payload)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| elif msg_type == MSG_SCRIPT_END: | |
| if payload: | |
| script_buf.extend(payload) | |
| code = bytes(script_buf).decode('utf-8') | |
| print('Received script ({} bytes), executing...'.format(len(code))) | |
| result = execute_script(code) | |
| print('Result ({} bytes): {}'.format(len(result), result[:100])) | |
| if peer_mac: | |
| send_result(peer_mac, result) | |
| # Reset state | |
| script_buf = bytearray() | |
| receiving = False | |
| elif msg_type == MSG_SCRIPT_END and receiving: | |
| if payload: | |
| script_buf.extend(payload) | |
| code = bytes(script_buf).decode('utf-8') | |
| print('Received script ({} bytes), executing...'.format(len(code))) | |
| result = execute_script(code) | |
| print('Result ({} bytes): {}'.format(len(result), result[:100])) | |
| if peer_mac: | |
| send_result(peer_mac, result) | |
| # Reset state | |
| script_buf = bytearray() | |
| receiving = False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/main.py` around lines 142 - 157, The MSG_SCRIPT_END branch should
guard against spurious packets by checking the receiving flag before processing;
modify the MSG_SCRIPT_END handling (where script_buf is used and
execute_script/send_result are called) to only append payload, decode, call
execute_script, send_result, and reset script_buf/receiving when receiving is
True, otherwise ignore the packet so you don't execute an empty or stale script
buffer.
Comment on lines
+42
to
+54
| ``` | ||
| Board B MAC: aa:bb:cc:dd:ee:ff | ||
| On Board A, run: set_espnow_peer aa:bb:cc:dd:ee:ff | ||
| ``` | ||
|
|
||
| ### 4. Configure Board A | ||
|
|
||
| On Board A's serial console: | ||
|
|
||
| ``` | ||
| set_espnow_peer aa:bb:cc:dd:ee:ff | ||
| restart | ||
| ``` |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks.
The code blocks at lines 42 and 51 are missing language specifiers. Adding text or console improves rendering consistency and satisfies markdown linting.
📝 Proposed fix
-```
+```text
Board B MAC: aa:bb:cc:dd:ee:ff
On Board A, run: set_espnow_peer aa:bb:cc:dd:ee:ff
```diff
-```
+```text
set_espnow_peer aa:bb:cc:dd:ee:ff
restart
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
[warning] 51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @executor/README.md around lines 42 - 54, Add language specifiers to the two
fenced code blocks containing the "Board B MAC: aa:bb:cc:dd:ee:ff" / "On Board
A, run: set_espnow_peer aa:bb:cc:dd:ee:ff" example and the "set_espnow_peer
aa:bb:cc:dd:ee:ff" / "restart" commands by changing the opening backticks from
totext (or ```console) so the blocks render consistently; update the
blocks that include the "Board B MAC" line and the block with "set_espnow_peer"
and "restart" accordingly.
</details>
<!-- fingerprinting:phantom:medusa:ocelot -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+592
to
+600
| nvs_handle_t nvs; | ||
| esp_err_t err = nvs_open(MIMI_NVS_ESPNOW, NVS_READWRITE, &nvs); | ||
| if (err != ESP_OK) { | ||
| printf("NVS open failed: %s\n", esp_err_to_name(err)); | ||
| return 1; | ||
| } | ||
| esp_err_t err = ctx->err; | ||
| vSemaphoreDelete(ctx->done); | ||
| free(input_copy); | ||
| free(ctx); | ||
| nvs_set_blob(nvs, MIMI_NVS_KEY_PEER_MAC, mac, 6); | ||
| nvs_commit(nvs); | ||
| nvs_close(nvs); |
There was a problem hiding this comment.
Check nvs_set_blob return value before commit.
The return value of nvs_set_blob is ignored. If the write fails (e.g., NVS is full), the code still commits and reports success.
🛡️ Proposed fix
nvs_handle_t nvs;
esp_err_t err = nvs_open(MIMI_NVS_ESPNOW, NVS_READWRITE, &nvs);
if (err != ESP_OK) {
printf("NVS open failed: %s\n", esp_err_to_name(err));
return 1;
}
- nvs_set_blob(nvs, MIMI_NVS_KEY_PEER_MAC, mac, 6);
- nvs_commit(nvs);
+ err = nvs_set_blob(nvs, MIMI_NVS_KEY_PEER_MAC, mac, 6);
+ if (err != ESP_OK) {
+ printf("NVS write failed: %s\n", esp_err_to_name(err));
+ nvs_close(nvs);
+ return 1;
+ }
+ err = nvs_commit(nvs);
nvs_close(nvs);
+ if (err != ESP_OK) {
+ printf("NVS commit failed: %s\n", esp_err_to_name(err));
+ return 1;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| nvs_handle_t nvs; | |
| esp_err_t err = nvs_open(MIMI_NVS_ESPNOW, NVS_READWRITE, &nvs); | |
| if (err != ESP_OK) { | |
| printf("NVS open failed: %s\n", esp_err_to_name(err)); | |
| return 1; | |
| } | |
| esp_err_t err = ctx->err; | |
| vSemaphoreDelete(ctx->done); | |
| free(input_copy); | |
| free(ctx); | |
| nvs_set_blob(nvs, MIMI_NVS_KEY_PEER_MAC, mac, 6); | |
| nvs_commit(nvs); | |
| nvs_close(nvs); | |
| nvs_handle_t nvs; | |
| esp_err_t err = nvs_open(MIMI_NVS_ESPNOW, NVS_READWRITE, &nvs); | |
| if (err != ESP_OK) { | |
| printf("NVS open failed: %s\n", esp_err_to_name(err)); | |
| return 1; | |
| } | |
| err = nvs_set_blob(nvs, MIMI_NVS_KEY_PEER_MAC, mac, 6); | |
| if (err != ESP_OK) { | |
| printf("NVS write failed: %s\n", esp_err_to_name(err)); | |
| nvs_close(nvs); | |
| return 1; | |
| } | |
| err = nvs_commit(nvs); | |
| nvs_close(nvs); | |
| if (err != ESP_OK) { | |
| printf("NVS commit failed: %s\n", esp_err_to_name(err)); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/cli/serial_cli.c` around lines 592 - 600, The code ignores the return
value of nvs_set_blob so a failed write can still be committed; change logic in
the block that opens NVS (nvs_open) to check esp_err_t from nvs_set_blob, handle
errors by logging via printf/esp_err_to_name(err) and skipping
nvs_commit/nvs_close as appropriate (or still close but return error), and only
call nvs_commit(nvs) when nvs_set_blob returns ESP_OK; use the same err variable
(esp_err_t err) for these checks and ensure nvs_close(nvs) always runs before
returning.
Comment on lines
+148
to
+174
| s_event_group = xEventGroupCreate(); | ||
| s_mutex = xSemaphoreCreateMutex(); | ||
| if (!s_event_group || !s_mutex) return ESP_ERR_NO_MEM; | ||
|
|
||
| err = esp_now_init(); | ||
| if (err != ESP_OK) { | ||
| ESP_LOGE(TAG, "esp_now_init failed: %s", esp_err_to_name(err)); | ||
| return err; | ||
| } | ||
|
|
||
| esp_now_register_send_cb(on_send_cb); | ||
| esp_now_register_recv_cb(on_recv_cb); | ||
|
|
||
| /* Add peer */ | ||
| esp_now_peer_info_t peer = { | ||
| .channel = MIMI_ESPNOW_CHANNEL, | ||
| .encrypt = false, | ||
| .ifidx = WIFI_IF_STA, | ||
| }; | ||
| memcpy(peer.peer_addr, s_peer_mac, 6); | ||
|
|
||
| err = esp_now_add_peer(&peer); | ||
| if (err != ESP_OK && err != ESP_ERR_ESPNOW_EXIST) { | ||
| ESP_LOGE(TAG, "esp_now_add_peer failed: %s", esp_err_to_name(err)); | ||
| esp_now_deinit(); | ||
| return err; | ||
| } |
There was a problem hiding this comment.
Resource leak on partial initialization failure.
If esp_now_init or esp_now_add_peer fails, the event group and mutex created at lines 148-149 are not freed. Similarly, if one of xEventGroupCreate/xSemaphoreCreateMutex fails, the other (if already created) is not cleaned up.
🛡️ Proposed fix
s_event_group = xEventGroupCreate();
s_mutex = xSemaphoreCreateMutex();
- if (!s_event_group || !s_mutex) return ESP_ERR_NO_MEM;
+ if (!s_event_group || !s_mutex) {
+ if (s_event_group) vEventGroupDelete(s_event_group);
+ if (s_mutex) vSemaphoreDelete(s_mutex);
+ s_event_group = NULL;
+ s_mutex = NULL;
+ return ESP_ERR_NO_MEM;
+ }
err = esp_now_init();
if (err != ESP_OK) {
ESP_LOGE(TAG, "esp_now_init failed: %s", esp_err_to_name(err));
+ vEventGroupDelete(s_event_group);
+ vSemaphoreDelete(s_mutex);
+ s_event_group = NULL;
+ s_mutex = NULL;
return err;
}
...
err = esp_now_add_peer(&peer);
if (err != ESP_OK && err != ESP_ERR_ESPNOW_EXIST) {
ESP_LOGE(TAG, "esp_now_add_peer failed: %s", esp_err_to_name(err));
esp_now_deinit();
+ vEventGroupDelete(s_event_group);
+ vSemaphoreDelete(s_mutex);
+ s_event_group = NULL;
+ s_mutex = NULL;
return err;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/espnow/espnow_manager.c` around lines 148 - 174, The init path leaks
FreeRTOS objects: ensure s_event_group and s_mutex are cleaned up on any
subsequent failure and that if one creation fails the other is deleted; after
calling xEventGroupCreate() and xSemaphoreCreateMutex() check each result and on
error delete the already-created object(s) and return ESP_ERR_NO_MEM, and
likewise if esp_now_init() or esp_now_add_peer() fails delete both s_event_group
and s_mutex before returning (and only call esp_now_deinit() where appropriate);
reference s_event_group, s_mutex, xEventGroupCreate, xSemaphoreCreateMutex,
esp_now_init, esp_now_add_peer and esp_now_deinit when implementing the cleanup.
Comment on lines
+14
to
+18
| if (!espnow_is_ready()) { | ||
| snprintf(output, output_size, | ||
| "Error: ESP-NOW not initialized. Configure peer with 'set espnow_peer XX:XX:XX:XX:XX:XX'"); | ||
| return ESP_ERR_INVALID_STATE; | ||
| } |
There was a problem hiding this comment.
Fix CLI command name in error message.
The error message references 'set espnow_peer' but the actual CLI command is set_espnow_peer (underscore, no space).
📝 Proposed fix
if (!espnow_is_ready()) {
snprintf(output, output_size,
- "Error: ESP-NOW not initialized. Configure peer with 'set espnow_peer XX:XX:XX:XX:XX:XX'");
+ "Error: ESP-NOW not initialized. Configure peer with 'set_espnow_peer XX:XX:XX:XX:XX:XX'");
return ESP_ERR_INVALID_STATE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!espnow_is_ready()) { | |
| snprintf(output, output_size, | |
| "Error: ESP-NOW not initialized. Configure peer with 'set espnow_peer XX:XX:XX:XX:XX:XX'"); | |
| return ESP_ERR_INVALID_STATE; | |
| } | |
| if (!espnow_is_ready()) { | |
| snprintf(output, output_size, | |
| "Error: ESP-NOW not initialized. Configure peer with 'set_espnow_peer XX:XX:XX:XX:XX:XX'"); | |
| return ESP_ERR_INVALID_STATE; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/tools/tool_run_python.c` around lines 14 - 18, The error message in
tool_run_python.c incorrectly references the CLI command 'set espnow_peer';
update the string passed to snprintf so it names the correct command
'set_espnow_peer' (underscore, no space). Locate the check using
espnow_is_ready() in the function surrounding this snippet and replace the
command text in the output buffer message to "Configure peer with
'set_espnow_peer XX:XX:XX:XX:XX:XX'".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
main/cli/serial_cli.c (1)
570-573: Avoid magic number for array iteration.The loop counter
7is hardcoded but should derive from the array size to prevent maintenance errors when namespaces are added or removed.♻️ Suggested improvement
const char *namespaces[] = { MIMI_NVS_WIFI, MIMI_NVS_TG, MIMI_NVS_FEISHU, MIMI_NVS_LLM, MIMI_NVS_PROXY, MIMI_NVS_SEARCH, MIMI_NVS_ESPNOW }; - for (int i = 0; i < 7; i++) { + for (int i = 0; i < (int)(sizeof(namespaces) / sizeof(namespaces[0])); i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/cli/serial_cli.c` around lines 570 - 573, The for-loop iterating over the namespaces array uses the hardcoded literal 7; change it to compute the array length dynamically (e.g., use sizeof(namespaces)/sizeof(namespaces[0]) or an ARRAY_SIZE(namespaces) macro) so the loop (for (int i = 0; i < ...; i++)) automatically adapts when elements are added/removed; update the loop condition where namespaces is referenced to use the computed length instead of the magic number.main/espnow/espnow_manager.c (1)
59-118: Potential data race between callback and main thread.
on_recv_cbaccesses shared state (s_result_buf,s_result_len,s_result_seq,s_result_total) without synchronization. Whileespnow_send_scriptholdss_mutexwhen setting up these variables, the callback runs asynchronously from the ESP-NOW task and doesn't acquire the mutex.This is likely safe in practice because:
- The callback only writes to the buffer that
espnow_send_scriptsets upespnow_send_scriptwaits on the event group before clearings_result_bufHowever, the pattern is fragile. Consider using
volatilequalifiers or ensuring memory barriers around the shared state to make the intent explicit and prevent compiler reordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/espnow/espnow_manager.c` around lines 59 - 118, Race on shared state: on_recv_cb reads/writes s_result_buf, s_result_len, s_result_seq, s_result_total without synchronization with espnow_send_script which uses s_mutex. Fix by protecting all accesses to those globals inside on_recv_cb with the same mutex (s_mutex): take the mutex at the start of each case branch that touches s_result_* and release it before returning (keep the critical section minimal), or alternatively snapshot needed values into local variables while holding s_mutex and do non-blocking work after release; ensure the mutex is initialized where espnow_send_script expects and keep the callback lock duration short so you don't block ESP-NOW processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/espnow/espnow_manager.c`:
- Around line 232-239: The current logic sets type = ESPNOW_MSG_SCRIPT_START
when seq == 0 even for single-chunk scripts, preventing MSG_SCRIPT_END from ever
being sent; change the selection to check if (seq == 0 && !is_last) to avoid
mis-classifying the last chunk, and update the send loop handling for the
single-chunk case (seq == 0 && is_last) to emit both a MSG_SCRIPT_START and a
MSG_SCRIPT_END for that payload (so executor/main.py still sees START to
initialize script_buf and then END to trigger execution); ensure the branch uses
the existing constants ESPNOW_MSG_SCRIPT_START, ESPNOW_MSG_SCRIPT_END, and
ESPNOW_MSG_SCRIPT_CHUNK and that the send routine reuses the same payload for
both messages.
---
Nitpick comments:
In `@main/cli/serial_cli.c`:
- Around line 570-573: The for-loop iterating over the namespaces array uses the
hardcoded literal 7; change it to compute the array length dynamically (e.g.,
use sizeof(namespaces)/sizeof(namespaces[0]) or an ARRAY_SIZE(namespaces) macro)
so the loop (for (int i = 0; i < ...; i++)) automatically adapts when elements
are added/removed; update the loop condition where namespaces is referenced to
use the computed length instead of the magic number.
In `@main/espnow/espnow_manager.c`:
- Around line 59-118: Race on shared state: on_recv_cb reads/writes
s_result_buf, s_result_len, s_result_seq, s_result_total without synchronization
with espnow_send_script which uses s_mutex. Fix by protecting all accesses to
those globals inside on_recv_cb with the same mutex (s_mutex): take the mutex at
the start of each case branch that touches s_result_* and release it before
returning (keep the critical section minimal), or alternatively snapshot needed
values into local variables while holding s_mutex and do non-blocking work after
release; ensure the mutex is initialized where espnow_send_script expects and
keep the callback lock duration short so you don't block ESP-NOW processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fc10c61-c315-45b5-965c-a0120862ae9f
📒 Files selected for processing (12)
executor/README.mdexecutor/boot.pyexecutor/main.pymain/CMakeLists.txtmain/cli/serial_cli.cmain/espnow/espnow_manager.cmain/espnow/espnow_manager.hmain/mimi.cmain/mimi_config.hmain/tools/tool_registry.cmain/tools/tool_run_python.cmain/tools/tool_run_python.h
🚧 Files skipped from review as they are similar to previous changes (2)
- main/tools/tool_registry.c
- main/tools/tool_run_python.c
Comment on lines
+232
to
+239
| uint8_t type; | ||
| if (seq == 0) { | ||
| type = ESPNOW_MSG_SCRIPT_START; | ||
| } else if (is_last) { | ||
| type = ESPNOW_MSG_SCRIPT_END; | ||
| } else { | ||
| type = ESPNOW_MSG_SCRIPT_CHUNK; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check executor/main.py for how it handles script message types and when it triggers execution
rg -n -A 10 'MSG_SCRIPT_END|MSG_SCRIPT_START' executor/main.pyRepository: memovai/mimiclaw
Length of output: 1145
Single-chunk scripts never trigger execution.
When a script fits in one chunk (seq == 0 and is_last == true), the condition if (seq == 0) evaluates true and sets the type to ESPNOW_MSG_SCRIPT_START. The else if (is_last) condition is never reached, so MSG_SCRIPT_END is never sent. The executor in executor/main.py only triggers script execution upon receiving MSG_SCRIPT_END (line 142-150), so single-chunk scripts will be buffered but never executed.
The proposed fix (adding && !is_last to the seq == 0 condition) correctly identifies that this condition should not apply to the last chunk, but implementing this fix alone creates a new issue: the executor expects MSG_SCRIPT_START to arrive first to initialize script_buf. Sending only MSG_SCRIPT_END will cause the script buffer operations to fail.
A complete solution requires either:
- Modifying the send loop to emit both
MSG_SCRIPT_STARTandMSG_SCRIPT_ENDfor single-chunk scripts, or - Modifying the executor to initialize
script_bufupon receivingMSG_SCRIPT_ENDif it hasn't been initialized yet
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/espnow/espnow_manager.c` around lines 232 - 239, The current logic sets
type = ESPNOW_MSG_SCRIPT_START when seq == 0 even for single-chunk scripts,
preventing MSG_SCRIPT_END from ever being sent; change the selection to check if
(seq == 0 && !is_last) to avoid mis-classifying the last chunk, and update the
send loop handling for the single-chunk case (seq == 0 && is_last) to emit both
a MSG_SCRIPT_START and a MSG_SCRIPT_END for that payload (so executor/main.py
still sees START to initialize script_buf and then END to trigger execution);
ensure the branch uses the existing constants ESPNOW_MSG_SCRIPT_START,
ESPNOW_MSG_SCRIPT_END, and ESPNOW_MSG_SCRIPT_CHUNK and that the send routine
reuses the same payload for both messages.
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
2 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.
Board A (MimiClaw) sends Python scripts via ESP-NOW to Board B (MicroPython executor) and receives execution results. This enables extensible skill execution without being limited by ESP32's C-only firmware environment.
Summary by CodeRabbit
Documentation
New Features