Open
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 MicroPython integration (runner, stubs, UART hooks, CLI commands), SPIRAM-aware allocations with fallbacks, TLS certificate-bundle compatibility and weak stubs, SNTP time-sync task and wait utility, and build/config updates including sdkconfig defaults and a micropython-helper dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Serial CLI User
participant CLI as serial_cli (mpy_run/mpy_exec)
participant Resolver as mpy_resolve_path
participant Task as FreeRTOS Task
participant Exec as mpy_exec_task (worker)
participant SPIFFS as SPIFFS
participant Runner as MicroPython Runner
User->>CLI: mpy_run script.py
CLI->>Resolver: validate & resolve path
alt invalid
Resolver-->>CLI: error
CLI-->>User: report invalid path
else valid
CLI->>Task: create worker with path
Task-->>CLI: queued
Exec->>SPIFFS: read script file
SPIFFS-->>Exec: file contents
Exec->>Runner: mpy_exec_file(path)
Runner->>Runner: acquire mutex
Runner->>Runner: allocate heap (SPIRAM -> fallback)
Runner->>Runner: setup VM & execute
Runner->>Runner: teardown VM & free heap
Runner-->>Exec: result
Exec->>CLI: log status
end
sequenceDiagram
participant App as Application
participant WiFi as WiFi Manager
participant SNTP as SNTP Task
participant Clock as System Clock
participant Runner as MicroPython Runner
App->>Runner: mpy_runner_init()
Runner-->>App: mutex created
App->>WiFi: connect
WiFi->>WiFi: obtain IP
WiFi->>SNTP: start sntp_sync_task (if not started)
SNTP->>Clock: poll time periodically
alt time >= 2020
Clock-->>SNTP: valid time
SNTP-->>WiFi: synced (log)
SNTP-->>SNTP: delete task
else timeout
SNTP-->>WiFi: warn unsynced
SNTP-->>SNTP: delete task
end
App->>App: wait_for_time_sync(timeout)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
IRONICBo
force-pushed
the
feat/micropython
branch
2 times, most recently
from
0be6774 to
4f29f84
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: 7
🧹 Nitpick comments (3)
main/micropython/mpy_runner.c (1)
100-105: Minor: Consider logging on file read inconsistency.If
freadreturns fewer bytes thanst.st_size, execution proceeds silently with truncated content. This could happen with filesystem errors.🔧 Optional: Add a warning for partial reads
size_t n = fread(buf, 1, size, f); fclose(f); + if (n < size) { + ESP_LOGW(TAG, "Partial read: %u of %u bytes from %s", (unsigned)n, (unsigned)size, path); + } buf[n] = '\0';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/micropython/mpy_runner.c` around lines 100 - 105, When reading the file size (st.st_size) and calling fread, check the actual bytes returned and handle partial reads: after calling fread into the buffer, compare the returned bytes_read against size (cast from st.st_size); if bytes_read < size log a warning (include path, expected size, and actual bytes_read) using the project logging facility and decide whether to treat it as an error or continue, and ensure resources (f) are still closed on error paths; reference the variables/functions st.st_size, size, fread, f and the existing ESP_ERR_INVALID_SIZE return for guidance on where to add the check and log.main/mimi.c (1)
172-174: Consider the timing relationship with SNTP initialization.The
wait_for_time_synccall happens immediately afterwifi_manager_wait_connectedreturns, butsntp_sync_taskis spawned asynchronously at that same moment (seewifi_manager.c:107). There's a brief window where SNTP hasn't initialized yet.In practice, the generous 15-second timeout should accommodate the SNTP startup delay, but if you encounter sporadic "Time not synced yet" warnings despite SNTP succeeding shortly after, this race is the likely cause.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/mimi.c` around lines 172 - 174, The warning can race with SNTP startup because wait_for_time_sync(15000) is called immediately after wifi_manager_wait_connected while sntp_sync_task is spawned asynchronously; ensure the SNTP synchronizer is actually started before waiting: either invoke the SNTP init/start routine (or call/start sntp_sync_task) synchronously prior to calling wait_for_time_sync, or add a short loop that polls for SNTP being started (or extend the timeout) so wait_for_time_sync only begins once sntp_sync_task is confirmed running; look for wifi_manager_wait_connected, sntp_sync_task, and wait_for_time_sync to implement the change.main/llm/llm_proxy.c (1)
107-120: Verifyheap_caps_reallocbehavior when SPIRAM is exhausted.When
use_spiramis true andheap_caps_reallocis called withMALLOC_CAP_SPIRAM, if SPIRAM is exhausted, the call will return NULL rather than falling back to internal RAM. This is correct behavior for maintaining theuse_spiraminvariant, but consider whether a fallback strategy might be beneficial for resilience.The current implementation is functionally correct—just flagging for awareness that large LLM responses could fail if SPIRAM becomes fragmented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/llm/llm_proxy.c` around lines 107 - 120, The resp_buf_append function currently calls heap_caps_realloc(..., MALLOC_CAP_SPIRAM) when rb->use_spiram is true which will return NULL if SPIRAM is exhausted; update resp_buf_append to detect a NULL return from heap_caps_realloc and implement a safe fallback (e.g., attempt a plain realloc to internal RAM) while preserving behavior and logging when fallback occurs; reference resp_buf_append, rb->use_spiram, heap_caps_realloc and realloc so you can add the conditional retry and appropriate error/log handling without changing the rest of the resizing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 24-29: The current SDKCONFIG_DEFAULTS list always includes
"sdkconfig.micropython.override", which unintentionally disables SPIRAM for the
S3 build; remove "sdkconfig.micropython.override" from the default
SDKCONFIG_DEFAULTS list and instead make it opt-in via a CMake option (e.g., a
boolean cache variable like USE_MICROPYTHON_OVERRIDE). Update the CMake logic
that constructs SDKCONFIG_DEFAULTS to conditionally append
"sdkconfig.micropython.override" only when that option is true, preserving the
existing defaults including "sdkconfig.defaults.esp32s3"; this prevents the
override from breaking PSRAM-first allocations in main/agent/agent_loop.c and
main/micropython/mpy_runner.c unless explicitly requested.
- Around line 5-15: The include of the helper's mpy.cmake happens too early
because MICROPY_HELPER_DIR / managed_components/mr9you__micropython-helper may
not exist before ESP-IDF resolves Component Manager dependencies; move or defer
that include so it runs after dependencies are resolved (e.g., move the
EXISTS/include block to after the project() call in CMakeLists.txt or remove it
here and instead add a project_include.cmake inside the
mr9you__micropython-helper component that contains the mpy.cmake logic;
alternatively, resolve the component at build time using
idf_component_get_property(helper_dir mr9you__micropython-helper COMPONENT_DIR)
and then include "${helper_dir}/mpy.cmake"), ensuring references to
MICROPY_HELPER_DIR and the mpy.cmake include are updated/removed accordingly.
In `@main/agent/agent_loop.c`:
- Around line 180-192: If any of the three allocations (system_prompt,
history_json, tool_output via heap_caps_calloc) fails, free any buffers that
were successfully allocated before calling vTaskDelete; specifically check each
pointer (system_prompt, history_json, tool_output) and call the appropriate free
(heap_caps_free or free consistent with heap_caps_calloc) to release memory,
then log the error with ESP_LOGE and call vTaskDelete(NULL). Update the failure
branch around the allocation block in agent_loop.c so no partial allocations are
leaked on startup abort.
In `@main/cli/serial_cli.c`:
- Around line 192-194: The fwrite call that writes const char *content must
check its return value to detect partial writes or errors: after calling
fwrite(content, 1, strlen(content), f) verify the returned size equals
strlen(content); if not, handle the failure (log an error, attempt to write
remaining bytes in a loop or treat as fatal, ensure fclose(f) is still called
and remove/truncate the incomplete file or return an error code). Update the
write logic around the fwrite call and the file pointer f so partial writes are
retried or properly reported and resources are cleaned up.
- Around line 118-130: The mpy_resolve_path function currently uses strstr(name,
"..") and only checks prefix for absolute paths, which can be bypassed or allow
escape; update mpy_resolve_path to split/normalize the input path into
components and reject any path segment equal to ".." (and "." as appropriate),
then rebuild a canonical path into out; for absolute inputs, after normalization
ensure the resulting path begins with MIMI_SPIFFS_BASE and does not escape it,
and for relative inputs prepend MIMI_SPIFFS_MPY_DIR and normalize so no ".." can
climb out; also ensure snprintf/out_size bounds are respected when writing the
final resolved path.
In `@main/wifi/wifi_manager.c`:
- Around line 20-52: The s_sntp_started flag has a race between the event
handler and sntp_sync_task; to fix it, set s_sntp_started = true in the event
handler immediately before creating/spawning sntp_sync_task (so the handler
short-circuits further spawns) and remove the assignment from inside
sntp_sync_task; locate references to s_sntp_started and the task creation call
(where vTaskCreate/xTaskCreate or similar is used) and move the flag write into
that event handler path, leaving sntp_sync_task to only perform SNTP setup and
synchronization.
In `@sdkconfig.micropython.override`:
- Around line 2-3: The override file hardcodes developer-specific absolute paths
for CONFIG_PARTITION_TABLE_CUSTOM_FILENAME and CONFIG_PARTITION_TABLE_FILENAME
which break builds for other machines and CI; change both to use
repository-relative paths (or just the partition filename) instead of
/Users/asklv/… so the build can locate partitions.csv across environments—update
the entries for CONFIG_PARTITION_TABLE_CUSTOM_FILENAME and
CONFIG_PARTITION_TABLE_FILENAME to a relative path (e.g., partitions.csv or a
repo-relative path) and commit the corrected sdkconfig.micropython.override.
---
Nitpick comments:
In `@main/llm/llm_proxy.c`:
- Around line 107-120: The resp_buf_append function currently calls
heap_caps_realloc(..., MALLOC_CAP_SPIRAM) when rb->use_spiram is true which will
return NULL if SPIRAM is exhausted; update resp_buf_append to detect a NULL
return from heap_caps_realloc and implement a safe fallback (e.g., attempt a
plain realloc to internal RAM) while preserving behavior and logging when
fallback occurs; reference resp_buf_append, rb->use_spiram, heap_caps_realloc
and realloc so you can add the conditional retry and appropriate error/log
handling without changing the rest of the resizing logic.
In `@main/micropython/mpy_runner.c`:
- Around line 100-105: When reading the file size (st.st_size) and calling
fread, check the actual bytes returned and handle partial reads: after calling
fread into the buffer, compare the returned bytes_read against size (cast from
st.st_size); if bytes_read < size log a warning (include path, expected size,
and actual bytes_read) using the project logging facility and decide whether to
treat it as an error or continue, and ensure resources (f) are still closed on
error paths; reference the variables/functions st.st_size, size, fread, f and
the existing ESP_ERR_INVALID_SIZE return for guidance on where to add the check
and log.
In `@main/mimi.c`:
- Around line 172-174: The warning can race with SNTP startup because
wait_for_time_sync(15000) is called immediately after
wifi_manager_wait_connected while sntp_sync_task is spawned asynchronously;
ensure the SNTP synchronizer is actually started before waiting: either invoke
the SNTP init/start routine (or call/start sntp_sync_task) synchronously prior
to calling wait_for_time_sync, or add a short loop that polls for SNTP being
started (or extend the timeout) so wait_for_time_sync only begins once
sntp_sync_task is confirmed running; look for wifi_manager_wait_connected,
sntp_sync_task, and wait_for_time_sync to implement the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93dbb2ac-fc2e-4fe1-b754-ecbdc682c1c3
📒 Files selected for processing (23)
CMakeLists.txtmain/CMakeLists.txtmain/agent/agent_loop.cmain/channels/feishu/feishu_bot.cmain/channels/telegram/telegram_bot.cmain/cli/serial_cli.cmain/compat/esp_tls_stubs.cmain/compat/mbedtls_compat.hmain/idf_component.ymlmain/llm/llm_proxy.cmain/micropython/mpy_runner.cmain/micropython/mpy_runner.hmain/micropython/mpy_stubs.cmain/micropython/mpy_uart_stubs.cmain/mimi.cmain/mimi_config.hmain/ota/ota_manager.cmain/proxy/http_proxy.cmain/tools/tool_get_time.cmain/tools/tool_web_search.cmain/wifi/wifi_manager.csdkconfig.defaults.esp32s3sdkconfig.micropython.override
Comment on lines
+180
to
192
| if (!system_prompt) { | ||
| system_prompt = heap_caps_calloc(1, MIMI_CONTEXT_BUF_SIZE, MALLOC_CAP_8BIT); | ||
| } | ||
| if (!history_json) { | ||
| history_json = heap_caps_calloc(1, MIMI_LLM_STREAM_BUF_SIZE, MALLOC_CAP_8BIT); | ||
| } | ||
| if (!tool_output) { | ||
| tool_output = heap_caps_calloc(1, TOOL_OUTPUT_SIZE, MALLOC_CAP_8BIT); | ||
| } | ||
|
|
||
| if (!system_prompt || !history_json || !tool_output) { | ||
| ESP_LOGE(TAG, "Failed to allocate PSRAM buffers"); | ||
| ESP_LOGE(TAG, "Failed to allocate agent buffers"); | ||
| vTaskDelete(NULL); |
There was a problem hiding this comment.
Free successful allocations before aborting startup.
If one buffer still fails after fallback, Lines 190-192 delete the task without releasing the buffers that already succeeded. On a low-memory boot that leaks tens of KiB permanently and makes recovery harder.
Proposed fix
if (!system_prompt || !history_json || !tool_output) {
ESP_LOGE(TAG, "Failed to allocate agent buffers");
+ free(system_prompt);
+ free(history_json);
+ free(tool_output);
vTaskDelete(NULL);
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/agent/agent_loop.c` around lines 180 - 192, If any of the three
allocations (system_prompt, history_json, tool_output via heap_caps_calloc)
fails, free any buffers that were successfully allocated before calling
vTaskDelete; specifically check each pointer (system_prompt, history_json,
tool_output) and call the appropriate free (heap_caps_free or free consistent
with heap_caps_calloc) to release memory, then log the error with ESP_LOGE and
call vTaskDelete(NULL). Update the failure branch around the allocation block in
agent_loop.c so no partial allocations are leaked on startup abort.
Comment on lines
+192
to
+194
| const char *content = mpy_write_args.content->sval[0]; | ||
| fwrite(content, 1, strlen(content), f); | ||
| fclose(f); |
There was a problem hiding this comment.
Check fwrite return value to detect partial writes.
If the filesystem is full or an I/O error occurs, fwrite may write fewer bytes than requested. The current code doesn't detect this, potentially leaving a corrupted script file.
🛡️ Proposed fix
const char *content = mpy_write_args.content->sval[0];
- fwrite(content, 1, strlen(content), f);
+ size_t len = strlen(content);
+ size_t written = fwrite(content, 1, len, f);
fclose(f);
- printf("mpy_write status: OK (%s)\n", path);
+ if (written != len) {
+ printf("mpy_write status: partial write (%zu/%zu bytes)\n", written, len);
+ return 1;
+ }
+ printf("mpy_write status: OK (%s, %zu bytes)\n", path, len);
return 0;🤖 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 192 - 194, The fwrite call that writes
const char *content must check its return value to detect partial writes or
errors: after calling fwrite(content, 1, strlen(content), f) verify the returned
size equals strlen(content); if not, handle the failure (log an error, attempt
to write remaining bytes in a loop or treat as fatal, ensure fclose(f) is still
called and remove/truncate the incomplete file or return an error code). Update
the write logic around the fwrite call and the file pointer f so partial writes
are retried or properly reported and resources are cleaned up.
Comment on lines
+20
to
+52
| static bool s_sntp_started = false; | ||
|
|
||
| static void sntp_sync_task(void *arg) | ||
| { | ||
| (void)arg; | ||
| if (!s_sntp_started) { | ||
| sntp_setoperatingmode(SNTP_OPMODE_POLL); | ||
| sntp_setservername(0, "pool.ntp.org"); | ||
| sntp_setservername(1, "time.nist.gov"); | ||
| sntp_init(); | ||
| s_sntp_started = true; | ||
| } | ||
|
|
||
| time_t now = 0; | ||
| struct tm timeinfo = {0}; | ||
| int retry = 0; | ||
| const int retry_max = 10; | ||
| while (retry < retry_max) { | ||
| time(&now); | ||
| localtime_r(&now, &timeinfo); | ||
| if (timeinfo.tm_year >= (2020 - 1900)) { | ||
| ESP_LOGI(TAG, "Time synced: %04d-%02d-%02d %02d:%02d:%02d", | ||
| timeinfo.tm_year + 1900, timeinfo.tm_mon + 1, timeinfo.tm_mday, | ||
| timeinfo.tm_hour, timeinfo.tm_min, timeinfo.tm_sec); | ||
| vTaskDelete(NULL); | ||
| return; | ||
| } | ||
| vTaskDelay(pdMS_TO_TICKS(2000)); | ||
| retry++; | ||
| } | ||
| ESP_LOGW(TAG, "SNTP sync timeout; TLS may fail until time is set"); | ||
| vTaskDelete(NULL); | ||
| } |
There was a problem hiding this comment.
Potential race condition on s_sntp_started flag.
The s_sntp_started flag is read on line 25 within the task and set on line 30, but it's also checked on line 106 in the event handler. Since these run in different contexts (event handler vs. task), there's a small race window where:
- Event handler checks
s_sntp_started(false), spawns task - Before task sets
s_sntp_started = true, another IP event could spawn a second task
While unlikely in practice (WiFi reconnects are spaced out), consider setting the flag before spawning the task for clarity.
🛡️ Proposed fix
xEventGroupSetBits(s_wifi_event_group, WIFI_CONNECTED_BIT);
if (!s_sntp_started) {
+ s_sntp_started = true; /* Set before spawn to prevent race */
xTaskCreate(sntp_sync_task, "sntp_sync", 4096, NULL, 5, NULL);
}And remove the flag setting from inside the task:
static void sntp_sync_task(void *arg)
{
(void)arg;
- if (!s_sntp_started) {
- sntp_setoperatingmode(SNTP_OPMODE_POLL);
- sntp_setservername(0, "pool.ntp.org");
- sntp_setservername(1, "time.nist.gov");
- sntp_init();
- s_sntp_started = true;
- }
+ sntp_setoperatingmode(SNTP_OPMODE_POLL);
+ sntp_setservername(0, "pool.ntp.org");
+ sntp_setservername(1, "time.nist.gov");
+ sntp_init();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/wifi/wifi_manager.c` around lines 20 - 52, The s_sntp_started flag has a
race between the event handler and sntp_sync_task; to fix it, set s_sntp_started
= true in the event handler immediately before creating/spawning sntp_sync_task
(so the handler short-circuits further spawns) and remove the assignment from
inside sntp_sync_task; locate references to s_sntp_started and the task creation
call (where vTaskCreate/xTaskCreate or similar is used) and move the flag write
into that event handler path, leaving sntp_sync_task to only perform SNTP setup
and synchronization.
48c17b1 to
8f8791c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
main/agent/agent_loop.c (1)
190-193:⚠️ Potential issue | 🟠 MajorFree partial allocations before aborting startup.
If Line 190 is hit after one or two buffers succeeded, this path leaks those allocations permanently because the task exits without releasing them. On a low-memory boot that makes recovery harder.
Proposed fix
if (!system_prompt || !history_json || !tool_output) { ESP_LOGE(TAG, "Failed to allocate agent buffers"); + free(system_prompt); + free(history_json); + free(tool_output); vTaskDelete(NULL); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/agent/agent_loop.c` around lines 190 - 193, The failure branch that logs "Failed to allocate agent buffers" can leak any buffers that were already allocated (system_prompt, history_json, tool_output); before calling ESP_LOGE(TAG, ...) and vTaskDelete(NULL) in the allocation check, free each buffer that is non-NULL (e.g., call the appropriate free()/heap_free()/agent_buffer_free() for system_prompt, history_json and tool_output) and set pointers to NULL so partial allocations are released cleanly prior to aborting startup.main/wifi/wifi_manager.c (1)
25-30:⚠️ Potential issue | 🟡 MinorSet
s_sntp_startedbefore spawning the task.The handler checks the flag, but the task flips it later. Two quick
IP_EVENT_STA_GOT_IPdeliveries can still create the task twice before Line 30 runs. Move the write into the handler immediately beforexTaskCreate()and drop the guard insidesntp_sync_task.Proposed fix
static void sntp_sync_task(void *arg) { (void)arg; - if (!s_sntp_started) { - sntp_setoperatingmode(SNTP_OPMODE_POLL); - sntp_setservername(0, "pool.ntp.org"); - sntp_setservername(1, "time.nist.gov"); - sntp_init(); - s_sntp_started = true; - } + sntp_setoperatingmode(SNTP_OPMODE_POLL); + sntp_setservername(0, "pool.ntp.org"); + sntp_setservername(1, "time.nist.gov"); + sntp_init(); @@ - if (!s_sntp_started) { + if (!s_sntp_started) { + s_sntp_started = true; xTaskCreate(sntp_sync_task, "sntp_sync", 4096, NULL, 5, NULL); }Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/wifi/wifi_manager.c` around lines 25 - 30, The s_sntp_started flag is set too late allowing a race where two IP_EVENT_STA_GOT_IP handlers spawn the same task; set s_sntp_started = true immediately in the IP_EVENT_STA_GOT_IP handler just before calling xTaskCreate() to reserve the task slot, remove the redundant guard/set inside sntp_sync_task, and apply the same change to the other occurrence that initializes SNTP (the block around sntp_setoperatingmode/sntp_init and its corresponding s_sntp_started write).CMakeLists.txt (2)
5-10:⚠️ Potential issue | 🟠 MajorDon't include
mpy.cmakefrommanaged_components/beforeproject().This still depends on
managed_components/mr9you__micropython-helperexisting before ESP-IDF resolves Component Manager dependencies, so a clean configure can skip the helper entirely. Move this integration to a supported post-resolution hook instead of probingmanaged_components/from the top-level preamble.In ESP-IDF 5.5, are Component Manager dependencies under managed_components guaranteed to exist before top-level CMakeLists.txt code runs before include($ENV{IDF_PATH}/tools/cmake/project.cmake) and project()? What is the supported way to include a CMake file from a managed component on a clean configure?Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 5 - 10, The top-level CMakeLists currently probes "${CMAKE_CURRENT_LIST_DIR}/managed_components/mr9you__micropython-helper/mpy.cmake" (using MICROPY_HELPER_DIR and setting MICROPY_BOARD) before project(), which can fail on a clean configure; remove that pre-project EXISTS/include logic and instead move the integration into a post-resolution hook that runs after project() (e.g., create a CMake include that is added after calling project() or use ESP-IDF's supported post-resolution/project include mechanism) and perform the MICROPY_HELPER_DIR detection and include of mpy.cmake there so the managed_components are guaranteed to be resolved first.
48-56:⚠️ Potential issue | 🟠 MajorMake the MicroPython override opt-in instead of unconditional.
Appending
sdkconfig.micropython.overridehere forcesCONFIG_SPIRAM=ninto the default S3 build. That conflicts with the PSRAM-first allocations inmain/agent/agent_loop.c:193-221andmain/llm/llm_proxy.c:93-125,616-619, which then fall back to internal RAM. Gate this file behind a CMake option instead of loading it for every build.Proposed fix
set(_mimi_sdkconfig_defaults "sdkconfig.defaults" "${CMAKE_CURRENT_LIST_DIR}/sdkconfig.defaults.esp32s3" - "${CMAKE_CURRENT_LIST_DIR}/sdkconfig.micropython.override" ) +option(MIMI_USE_MICROPYTHON_OVERRIDE "Enable MicroPython-specific sdkconfig overrides" OFF) +if(MIMI_USE_MICROPYTHON_OVERRIDE) + list(APPEND _mimi_sdkconfig_defaults "${CMAKE_CURRENT_LIST_DIR}/sdkconfig.micropython.override") +endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 48 - 56, The CMakeLists currently always appends "sdkconfig.micropython.override" into the _mimi_sdkconfig_defaults list (via set(_mimi_sdkconfig_defaults ...)) which forces CONFIG_SPIRAM=n; change this to be opt-in by adding a CMake option (e.g., option(USE_MICROPYTHON_SDKCONFIG "Enable micropython sdkconfig override" OFF)) and only insert/append "${CMAKE_CURRENT_LIST_DIR}/sdkconfig.micropython.override" into _mimi_sdkconfig_defaults when that option is ON; keep the existing conditional for "${CMAKE_BINARY_DIR}/sdkconfig.combined" and leave the final set(SDKCONFIG_DEFAULTS ...) unchanged so behavior is identical unless the new option is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 11-23: The current regex-only removal in the string(REGEX REPLACE)
step for _mimi_mpy_common_src targets target_link_libraries(...) lines but
misses generator expressions like $<TARGET_FILE:micropy_extmod_btree>; update
the post-read transformations to also remove or neutralize generator expressions
referencing micropy_extmod_btree (e.g., "$<TARGET_FILE:micropy_extmod_btree>"
and "$<LINK_ONLY:micropy_extmod_btree>") from the _mimi_mpy_common_src string
after reading _mimi_mpy_common so the offending generator expressions are
eliminated before writing the file back; keep references to the existing
variables/functions (_mimi_mpy_common, _mimi_mpy_common_src, string(REGEX
REPLACE), micropy_extmod_btree) when implementing the fix.
In `@main/compat/mbedtls_compat.h`:
- Around line 5-10: The macro MIMI_CRT_BUNDLE_ATTACH currently expands to NULL
when CONFIG_MBEDTLS_CERTIFICATE_BUNDLE is disabled, removing the only visible CA
source and breaking TLS/HTTPS initialization; fix this by providing a real
fallback instead of NULL: either set MIMI_CRT_BUNDLE_ATTACH to a function that
attaches an alternative CA source (e.g., a project-provided PEM loader) or
ensure callers explicitly pass a valid CA configuration
(cert_pem/use_global_ca_store/cacert_pem) before initializing mbedTLS;
alternatively, gate the features that require the bundle behind the
CONFIG_MBEDTLS_CERTIFICATE_BUNDLE flag so they are not compiled when no CA
source exists. Ensure changes reference the MIMI_CRT_BUNDLE_ATTACH macro and
update all TLS initialization call sites to use the fallback or be conditionally
compiled.
In `@main/micropython/mpy_uart_stubs.c`:
- Around line 1-13: The file includes a non-existent header "uart.h" which will
break build; either remove the include and keep the stub implementations of
uart_stdout_init and uart_stdout_tx_strn (they only use stdio), or add a new
uart.h declaring void uart_stdout_init(void); and int uart_stdout_tx_strn(const
char *str, size_t len); and place it where headers are expected so the compiler
can find it; update include in mpy_uart_stubs.c to reference the new header if
you choose option 2.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 5-10: The top-level CMakeLists currently probes
"${CMAKE_CURRENT_LIST_DIR}/managed_components/mr9you__micropython-helper/mpy.cmake"
(using MICROPY_HELPER_DIR and setting MICROPY_BOARD) before project(), which can
fail on a clean configure; remove that pre-project EXISTS/include logic and
instead move the integration into a post-resolution hook that runs after
project() (e.g., create a CMake include that is added after calling project() or
use ESP-IDF's supported post-resolution/project include mechanism) and perform
the MICROPY_HELPER_DIR detection and include of mpy.cmake there so the
managed_components are guaranteed to be resolved first.
- Around line 48-56: The CMakeLists currently always appends
"sdkconfig.micropython.override" into the _mimi_sdkconfig_defaults list (via
set(_mimi_sdkconfig_defaults ...)) which forces CONFIG_SPIRAM=n; change this to
be opt-in by adding a CMake option (e.g., option(USE_MICROPYTHON_SDKCONFIG
"Enable micropython sdkconfig override" OFF)) and only insert/append
"${CMAKE_CURRENT_LIST_DIR}/sdkconfig.micropython.override" into
_mimi_sdkconfig_defaults when that option is ON; keep the existing conditional
for "${CMAKE_BINARY_DIR}/sdkconfig.combined" and leave the final
set(SDKCONFIG_DEFAULTS ...) unchanged so behavior is identical unless the new
option is enabled.
In `@main/agent/agent_loop.c`:
- Around line 190-193: The failure branch that logs "Failed to allocate agent
buffers" can leak any buffers that were already allocated (system_prompt,
history_json, tool_output); before calling ESP_LOGE(TAG, ...) and
vTaskDelete(NULL) in the allocation check, free each buffer that is non-NULL
(e.g., call the appropriate free()/heap_free()/agent_buffer_free() for
system_prompt, history_json and tool_output) and set pointers to NULL so partial
allocations are released cleanly prior to aborting startup.
In `@main/wifi/wifi_manager.c`:
- Around line 25-30: The s_sntp_started flag is set too late allowing a race
where two IP_EVENT_STA_GOT_IP handlers spawn the same task; set s_sntp_started =
true immediately in the IP_EVENT_STA_GOT_IP handler just before calling
xTaskCreate() to reserve the task slot, remove the redundant guard/set inside
sntp_sync_task, and apply the same change to the other occurrence that
initializes SNTP (the block around sntp_setoperatingmode/sntp_init and its
corresponding s_sntp_started write).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 174e63dc-948d-42f9-978a-259d8b4d2567
📒 Files selected for processing (23)
CMakeLists.txtmain/CMakeLists.txtmain/agent/agent_loop.cmain/channels/feishu/feishu_bot.cmain/channels/telegram/telegram_bot.cmain/cli/serial_cli.cmain/compat/esp_tls_stubs.cmain/compat/mbedtls_compat.hmain/idf_component.ymlmain/llm/llm_proxy.cmain/micropython/mpy_runner.cmain/micropython/mpy_runner.hmain/micropython/mpy_stubs.cmain/micropython/mpy_uart_stubs.cmain/mimi.cmain/mimi_config.hmain/ota/ota_manager.cmain/proxy/http_proxy.cmain/tools/tool_get_time.cmain/tools/tool_web_search.cmain/wifi/wifi_manager.csdkconfig.defaults.esp32s3sdkconfig.micropython.override
🚧 Files skipped from review as they are similar to previous changes (12)
- main/channels/feishu/feishu_bot.c
- main/proxy/http_proxy.c
- main/tools/tool_get_time.c
- main/CMakeLists.txt
- main/llm/llm_proxy.c
- main/channels/telegram/telegram_bot.c
- main/micropython/mpy_runner.c
- main/ota/ota_manager.c
- main/idf_component.yml
- main/mimi.c
- main/mimi_config.h
- main/tools/tool_web_search.c
Comment on lines
+11
to
+23
| # Work around IDF 5.5 + micropython-helper 1.22.1: linking micropy_extmod_btree | ||
| # (OBJECT_LIBRARY) breaks ldgen when TARGET_FILE is expanded. | ||
| set(_mimi_mpy_common "${MICROPY_HELPER_DIR}/micropython/ports/esp32/esp32_common.cmake") | ||
| if(EXISTS "${_mimi_mpy_common}") | ||
| file(READ "${_mimi_mpy_common}" _mimi_mpy_common_src) | ||
| string(REGEX REPLACE | ||
| "[^\n]*target_link_libraries\\([^\n]*micropy_extmod_btree[^\n]*\\)\n" | ||
| "" | ||
| _mimi_mpy_common_src | ||
| "${_mimi_mpy_common_src}" | ||
| ) | ||
| file(WRITE "${_mimi_mpy_common}" "${_mimi_mpy_common_src}") | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any remaining references to the broken target or generator expression.
rg -n -C2 'micropy_extmod_btree|\$<TARGET_FILE:micropy_extmod_btree>' CMakeLists.txt managed_components || trueRepository: memovai/mimiclaw
Length of output: 1203
🏁 Script executed:
#!/bin/bash
# Search entire repo for micropy_extmod_btree references
rg 'micropy_extmod_btree' --no-heading -nRepository: memovai/mimiclaw
Length of output: 370
🏁 Script executed:
#!/bin/bash
# Check the structure and find any related cmake files
find . -name "*.cmake" -o -name "CMakeLists.txt" | head -20Repository: memovai/mimiclaw
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Look at full CMakeLists.txt to understand the complete workaround
wc -l CMakeLists.txtRepository: memovai/mimiclaw
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Read full CMakeLists.txt to understand the workaround context
cat -n CMakeLists.txtRepository: memovai/mimiclaw
Length of output: 3383
🏁 Script executed:
#!/bin/bash
# Check if MICROPY_HELPER_DIR is set and what esp32_common.cmake contains
# Also check if there are other cmake files that might reference the target
find . -type f -name "esp32_common.cmake" -o -name "*esp32*.cmake" 2>/dev/nullRepository: memovai/mimiclaw
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Verify the generator expression pattern - check if TARGET_FILE is mentioned elsewhere
rg '\$<TARGET_FILE' --no-heading -C2Repository: memovai/mimiclaw
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check for any generator expressions in the codebase
rg '\$<' --no-heading | head -20Repository: memovai/mimiclaw
Length of output: 173
The $<TARGET_FILE:micropy_extmod_btree> generator expression is not being removed by the current workaround.
The workaround removes $<LINK_ONLY:micropy_extmod_btree> at line 32 and attempts regex removal from source files at line 17, but the error indicates $<TARGET_FILE:micropy_extmod_btree> is still being generated. These are different generator expression patterns—the TARGET_FILE variant is not caught by the current mitigation. This requires either a patched version of micropython-helper that doesn't generate the TARGET_FILE expression, or a deterministic component-level fix after the target graph exists, before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 11 - 23, The current regex-only removal in the
string(REGEX REPLACE) step for _mimi_mpy_common_src targets
target_link_libraries(...) lines but misses generator expressions like
$<TARGET_FILE:micropy_extmod_btree>; update the post-read transformations to
also remove or neutralize generator expressions referencing micropy_extmod_btree
(e.g., "$<TARGET_FILE:micropy_extmod_btree>" and
"$<LINK_ONLY:micropy_extmod_btree>") from the _mimi_mpy_common_src string after
reading _mimi_mpy_common so the offending generator expressions are eliminated
before writing the file back; keep references to the existing
variables/functions (_mimi_mpy_common, _mimi_mpy_common_src, string(REGEX
REPLACE), micropy_extmod_btree) when implementing the fix.
Comment on lines
+5
to
+10
| #if CONFIG_MBEDTLS_CERTIFICATE_BUNDLE | ||
| #include "esp_crt_bundle.h" | ||
| #define MIMI_CRT_BUNDLE_ATTACH esp_crt_bundle_attach | ||
| #else | ||
| #define MIMI_CRT_BUNDLE_ATTACH NULL | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether every MIMI_CRT_BUNDLE_ATTACH caller has an alternate CA source.
rg -n -C3 'crt_bundle_attach\s*=\s*MIMI_CRT_BUNDLE_ATTACH|cert_pem|use_global_ca_store|cacert_buf|cacert_pem' mainRepository: memovai/mimiclaw
Length of output: 5144
🏁 Script executed:
#!/bin/bash
# Check if CONFIG_MBEDTLS_CERTIFICATE_BUNDLE is actually configured/tested
rg -n 'CONFIG_MBEDTLS_CERTIFICATE_BUNDLE' mainRepository: memovai/mimiclaw
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Check for any global CA store setup or alternative CA configurations
rg -n 'use_global_ca_store|esp_tls_init_global_ca_store|cacert' main --type cRepository: memovai/mimiclaw
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Count all unique files with MIMI_CRT_BUNDLE_ATTACH usage
rg -l 'MIMI_CRT_BUNDLE_ATTACH' mainRepository: memovai/mimiclaw
Length of output: 284
MIMI_CRT_BUNDLE_ATTACH removes the only visible CA source for 7+ HTTPS/TLS endpoints in bundle-disabled builds.
When CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=n, this macro becomes NULL. The codebase has no visible fallback CA sources (cert_pem, use_global_ca_store, cacert_pem, etc.) at any call site. This affects HTTPS/TLS initialization in:
main/ota/ota_manager.c(line 19)main/proxy/http_proxy.c(line 294)main/llm/llm_proxy.c(line 271)main/tools/tool_web_search.c(lines 224, 317)main/tools/tool_get_time.c(line 142)main/channels/telegram/telegram_bot.c(line 218)main/channels/feishu/feishu_bot.c(lines 330, 390, 467, 623)
If bundle-disabled builds must work, configure a real CA source globally (or per-endpoint) instead of returning NULL. Otherwise, explicitly compile these features out when the bundle is disabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/compat/mbedtls_compat.h` around lines 5 - 10, The macro
MIMI_CRT_BUNDLE_ATTACH currently expands to NULL when
CONFIG_MBEDTLS_CERTIFICATE_BUNDLE is disabled, removing the only visible CA
source and breaking TLS/HTTPS initialization; fix this by providing a real
fallback instead of NULL: either set MIMI_CRT_BUNDLE_ATTACH to a function that
attaches an alternative CA source (e.g., a project-provided PEM loader) or
ensure callers explicitly pass a valid CA configuration
(cert_pem/use_global_ca_store/cacert_pem) before initializing mbedTLS;
alternatively, gate the features that require the bundle behind the
CONFIG_MBEDTLS_CERTIFICATE_BUNDLE flag so they are not compiled when no CA
source exists. Ensure changes reference the MIMI_CRT_BUNDLE_ATTACH macro and
update all TLS initialization call sites to use the fallback or be conditionally
compiled.
Comment on lines
+1
to
+13
| #include <stdio.h> | ||
| #include "uart.h" | ||
|
|
||
| void uart_stdout_init(void) | ||
| { | ||
| /* No-op: UART REPL disabled in embedded mode. */ | ||
| } | ||
|
|
||
| int uart_stdout_tx_strn(const char *str, size_t len) | ||
| { | ||
| if (!str || len == 0) return 0; | ||
| return (int)fwrite(str, 1, len, stdout); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that uart.h header exists in the MicroPython port or ESP-IDF components
fd -t f 'uart.h' --exec echo "Found: {}"Repository: memovai/mimiclaw
Length of output: 42
🏁 Script executed:
# Search for all header files in the main/micropython directory
fd -t f '\.(h|hpp)$' main/micropython --exec echo "{}"Repository: memovai/mimiclaw
Length of output: 89
🏁 Script executed:
# Check for references to "uart.h" in the codebase (build files, source files, etc.)
rg 'uart\.h' --type c --type cmake --type makefileRepository: memovai/mimiclaw
Length of output: 92
🏁 Script executed:
# Look at the directory structure of main/micropython
ls -la main/micropython/ 2>/dev/null || echo "Directory not found"Repository: memovai/mimiclaw
Length of output: 428
🏁 Script executed:
# Check if there's any build configuration that might explain header resolution
fd -t f '(CMakeLists|Makefile|idf_component)' main/micropythonRepository: memovai/mimiclaw
Length of output: 42
🏁 Script executed:
# Search entire repository for uart.h
fd -t f 'uart\.h'Repository: memovai/mimiclaw
Length of output: 42
🏁 Script executed:
# Check what the other stub file (mpy_stubs.c) includes
cat -n main/micropython/mpy_stubs.c | head -20Repository: memovai/mimiclaw
Length of output: 746
🏁 Script executed:
# Check what mpy_uart_stubs.c includes and what it needs
cat -n main/micropython/mpy_uart_stubs.cRepository: memovai/mimiclaw
Length of output: 413
🏁 Script executed:
# Search for any idf_component or build configuration files
fd -t f 'idf_component'Repository: memovai/mimiclaw
Length of output: 82
🏁 Script executed:
# Check if uart.h is referenced anywhere in build or config files
rg 'uart\.h' --type cpp --type c --type yamlRepository: memovai/mimiclaw
Length of output: 111
🏁 Script executed:
# Check the idf_component.yml to understand component dependencies
cat main/idf_component.ymlRepository: memovai/mimiclaw
Length of output: 705
🏁 Script executed:
# Look at the entire main directory to understand structure
ls -la main/ | head -30Repository: memovai/mimiclaw
Length of output: 1404
🏁 Script executed:
# Search for any references to uart or stdio in the entire codebase that might hint at where uart.h comes from
rg 'UART|uart' --type c --type yaml --max-count=20Repository: memovai/mimiclaw
Length of output: 650
Missing required header file — uart.h does not exist.
The file includes "uart.h" at line 2, but this header file is not present in the repository. A search of the entire codebase confirms no uart.h exists locally, and it is not available from external dependencies listed in idf_component.yml. This will cause a compilation failure.
Either:
- Create the missing
uart.hheader to declareuart_stdout_init()anduart_stdout_tx_strn(), or - Remove the include if these functions are meant to be internal to this stub file.
Note: The functions in this file only use stdio.h (fwrite, stdout), so uart.h may not be necessary for the implementation itself.
🧰 Tools
🪛 Clang (14.0.6)
[error] 1-1: 'stdio.h' file not found
(clang-diagnostic-error)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/micropython/mpy_uart_stubs.c` around lines 1 - 13, The file includes a
non-existent header "uart.h" which will break build; either remove the include
and keep the stub implementations of uart_stdout_init and uart_stdout_tx_strn
(they only use stdio), or add a new uart.h declaring void
uart_stdout_init(void); and int uart_stdout_tx_strn(const char *str, size_t
len); and place it where headers are expected so the compiler can find it;
update include in mpy_uart_stubs.c to reference the new header if you choose
option 2.
a2f1af5 to
1a982b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CMakeLists.txt (2)
74-82:⚠️ Potential issue | 🟠 MajorMicroPython override unconditionally included; may conflict with SPIRAM settings.
sdkconfig.micropython.overrideis always appended last in the defaults list, meaning it overrides all preceding configs. If this file disables or modifies SPIRAM settings, it will conflict with PSRAM-first allocations used elsewhere in the codebase (e.g.,main/agent/agent_loop.c,main/micropython/mpy_runner.c).Consider making the override opt-in via a CMake option:
Suggested change
+option(MIMI_USE_MICROPYTHON_OVERRIDE "Include sdkconfig.micropython.override" OFF) + set(_mimi_sdkconfig_defaults "sdkconfig.defaults" "${CMAKE_CURRENT_LIST_DIR}/sdkconfig.defaults.esp32s3" - "${CMAKE_CURRENT_LIST_DIR}/sdkconfig.micropython.override" ) +if(MIMI_USE_MICROPYTHON_OVERRIDE) + list(APPEND _mimi_sdkconfig_defaults "${CMAKE_CURRENT_LIST_DIR}/sdkconfig.micropython.override") +endif(),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 74 - 82, The current CMakeLists adds "sdkconfig.micropython.override" unconditionally to the _mimi_sdkconfig_defaults list which allows that file to override SPIRAM/PSRAM settings; change this to be opt-in by introducing a CMake option (e.g., a boolean CACHE variable like USE_MICROPY_OVERRIDE defaulting to OFF) and only append or insert "sdkconfig.micropython.override" into _mimi_sdkconfig_defaults when that option is ON; update references to _mimi_sdkconfig_defaults and the final set(SDKCONFIG_DEFAULTS ...) so behavior is unchanged unless the new option is enabled.
10-35:⚠️ Potential issue | 🔴 CriticalBuild fails on clean configure and
$<TARGET_FILE:micropy_extmod_btree>is not removed.Two critical problems remain:
Clean configure failure: The
EXISTScheck at line 10 runs before the Component Manager downloads dependencies (beforeproject()at line 85). On a clean build,managed_components/doesn't exist, so this entire block is skipped and the workarounds never apply.Pipeline failure persists: The regex at lines 27-32 only strips
target_link_libraries(...)lines containingmicropy_extmod_btree. The$<TARGET_FILE:micropy_extmod_btree>generator expression used elsewhere in the helper is not caught, causing the ldgen error shown in CI.To fix both issues, move this logic to a component-level
CMakeLists.txtthat runs after dependency resolution, or request the upstreammr9you/micropython-helperto ship aproject_include.cmakewith the necessary patches. Alternatively, add an additional string replacement to catch$<TARGET_FILE:micropy_extmod_btree>:Suggested additional patch for TARGET_FILE (still requires fixing timing)
string(REGEX REPLACE "[^\n]*target_link_libraries\\([^\n]*micropy_extmod_btree[^\n]*\\)\n" "" _mimi_mpy_common_src "${_mimi_mpy_common_src}" ) + string(REPLACE + "$<TARGET_FILE:micropy_extmod_btree>" + "" + _mimi_mpy_common_src + "${_mimi_mpy_common_src}" + ) file(WRITE "${_mimi_mpy_common}" "${_mimi_mpy_common_src}"),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 10 - 35, The workaround block that modifies files under MICROPY_HELPER_DIR runs too early (before project()/dependency fetch) and its regex only removes target_link_libraries lines referencing micropy_extmod_btree, leaving $<TARGET_FILE:micropy_extmod_btree> generator expressions that break ldgen; move this logic from the top-level CMakeLists into a component-level CMakeLists.txt that is processed after dependency resolution (so EXISTS checks on _mimi_extmod_cmake and _mimi_mpy_common succeed), and also add an additional string replacement targeting the literal "$<TARGET_FILE:micropy_extmod_btree>" (or a regex matching generator expressions) when editing _mimi_extmod_cmake/_mimi_mpy_common (the symbols to modify are _mimi_extmod_cmake, _mimi_extmod_src, _mimi_mpy_common, _mimi_mpy_common_src, micropy_extmod_btree, and the target_link_libraries lines).
🧹 Nitpick comments (1)
CMakeLists.txt (1)
66-73: Redundant MBEDTLS patching; settings already insdkconfig.micropython.override.Per the context snippet,
sdkconfig.micropython.override(line 77) already defines:CONFIG_MBEDTLS_SSL_ALPN=y CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=ySince
sdkconfig.micropython.overrideappears last inSDKCONFIG_DEFAULTS, its values take precedence in ESP-IDF's configuration merge. The replace-and-append logic here is redundant and adds clutter tosdkconfig.combined. Consider removing lines 66-73 entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 66 - 73, Remove the redundant manual patching block that reads/writes "${CMAKE_BINARY_DIR}/sdkconfig.combined" (the if(EXISTS ... ) block replacing and appending CONFIG_MBEDTLS_SSL_ALPN and CONFIG_MBEDTLS_CERTIFICATE_BUNDLE) because these settings are already provided in sdkconfig.micropython.override and handled via SDKCONFIG_DEFAULTS; delete that entire conditional block so the override file controls those config values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 48-64: The loop that tries to clean micropy_extmod_btree links
uses get_property(... GLOBAL PROPERTY TARGETS) which is invalid and runs too
early; change to query directory-level targets via
get_property(_mimi_all_targets DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" PROPERTY
BUILDSYSTEM_TARGETS) and move the entire cleanup block so it executes after
project() / after ESP-IDF has created its component targets (e.g., place it in a
project_include.cmake or a component CMakeLists.txt that is parsed
post-project()); keep the rest of the logic (list REMOVE_ITEM for
micropy_extmod_btree and micropy_extmod_btree_static and the set_property calls)
unchanged but ensure the new location runs when BUILD system targets exist.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Around line 74-82: The current CMakeLists adds
"sdkconfig.micropython.override" unconditionally to the _mimi_sdkconfig_defaults
list which allows that file to override SPIRAM/PSRAM settings; change this to be
opt-in by introducing a CMake option (e.g., a boolean CACHE variable like
USE_MICROPY_OVERRIDE defaulting to OFF) and only append or insert
"sdkconfig.micropython.override" into _mimi_sdkconfig_defaults when that option
is ON; update references to _mimi_sdkconfig_defaults and the final
set(SDKCONFIG_DEFAULTS ...) so behavior is unchanged unless the new option is
enabled.
- Around line 10-35: The workaround block that modifies files under
MICROPY_HELPER_DIR runs too early (before project()/dependency fetch) and its
regex only removes target_link_libraries lines referencing micropy_extmod_btree,
leaving $<TARGET_FILE:micropy_extmod_btree> generator expressions that break
ldgen; move this logic from the top-level CMakeLists into a component-level
CMakeLists.txt that is processed after dependency resolution (so EXISTS checks
on _mimi_extmod_cmake and _mimi_mpy_common succeed), and also add an additional
string replacement targeting the literal "$<TARGET_FILE:micropy_extmod_btree>"
(or a regex matching generator expressions) when editing
_mimi_extmod_cmake/_mimi_mpy_common (the symbols to modify are
_mimi_extmod_cmake, _mimi_extmod_src, _mimi_mpy_common, _mimi_mpy_common_src,
micropy_extmod_btree, and the target_link_libraries lines).
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 66-73: Remove the redundant manual patching block that
reads/writes "${CMAKE_BINARY_DIR}/sdkconfig.combined" (the if(EXISTS ... ) block
replacing and appending CONFIG_MBEDTLS_SSL_ALPN and
CONFIG_MBEDTLS_CERTIFICATE_BUNDLE) because these settings are already provided
in sdkconfig.micropython.override and handled via SDKCONFIG_DEFAULTS; delete
that entire conditional block so the override file controls those config values.
Comment on lines
+48
to
+64
| get_property(_mimi_all_targets GLOBAL PROPERTY TARGETS) | ||
| foreach(_mimi_tgt IN LISTS _mimi_all_targets) | ||
| foreach(_mimi_prop IN ITEMS LINK_LIBRARIES INTERFACE_LINK_LIBRARIES) | ||
| get_target_property(_mimi_links ${_mimi_tgt} ${_mimi_prop}) | ||
| if(_mimi_links) | ||
| set(_mimi_links_new "${_mimi_links}") | ||
| list(REMOVE_ITEM _mimi_links_new micropy_extmod_btree "$<LINK_ONLY:micropy_extmod_btree>") | ||
| if(TARGET micropy_extmod_btree_static) | ||
| list(REMOVE_ITEM _mimi_links_new micropy_extmod_btree_static "$<LINK_ONLY:micropy_extmod_btree_static>") | ||
| list(APPEND _mimi_links_new micropy_extmod_btree_static) | ||
| endif() | ||
| if(NOT _mimi_links_new STREQUAL _mimi_links) | ||
| set_property(TARGET ${_mimi_tgt} PROPERTY ${_mimi_prop} "${_mimi_links_new}") | ||
| endif() | ||
| endif() | ||
| endforeach() | ||
| endforeach() |
There was a problem hiding this comment.
GLOBAL PROPERTY TARGETS is not valid; target cleanup loop is ineffective.
TARGETS is not a standard CMake global property. You likely intended BUILDSYSTEM_TARGETS on a directory scope:
get_property(_mimi_all_targets DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" PROPERTY BUILDSYSTEM_TARGETS)However, even with this fix, the loop runs before project() is invoked, so ESP-IDF component targets haven't been created yet and won't be cleaned. This cleanup logic needs to execute after the ESP-IDF build system has defined all targets (e.g., via a project_include.cmake or in a component's CMakeLists.txt).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 48 - 64, The loop that tries to clean
micropy_extmod_btree links uses get_property(... GLOBAL PROPERTY TARGETS) which
is invalid and runs too early; change to query directory-level targets via
get_property(_mimi_all_targets DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" PROPERTY
BUILDSYSTEM_TARGETS) and move the entire cleanup block so it executes after
project() / after ESP-IDF has created its component targets (e.g., place it in a
project_include.cmake or a component CMakeLists.txt that is parsed
post-project()); keep the rest of the logic (list REMOVE_ITEM for
micropy_extmod_btree and micropy_extmod_btree_static and the set_property calls)
unchanged but ensure the new location runs when BUILD system targets exist.
1a982b2 to
7dfd632
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 5-17: The CI git-clone creates the
managed_components/mr9you__micropython-helper tree before ESP-IDF's Component
Manager runs, which can conflict with idf_component.yml; change the logic so CI
does not write into MICROPY_HELPER_DIR directly: either remove the
execute_process fallback and let Component Manager populate MICROPY_HELPER_DIR,
or clone into a separate temp path (not managed_components) and set
MICROPY_HELPER_DIR to that temp only for CI; update references to
MICROPY_HELPER_DIR and keep the find_package(Git) + execute_process call gated
behind the new separate directory or removed entirely, and ensure the check that
currently runs before project() is preserved or moved so local configures behave
the same.
- Around line 77-84: The current block that reads and then appends
CONFIG_MBEDTLS_SSL_ALPN and CONFIG_MBEDTLS_CERTIFICATE_BUNDLE is redundant and
can race with ESP-IDF generating sdkconfig.combined; remove the read/replace +
append hack and instead inject the settings reliably via ESP-IDF's
SDKCONFIG_DEFAULTS mechanism or a post-configure hook. Concretely: delete the
file(READ)/string(REPLACE)/file(APPEND) sequence operating on
"${CMAKE_BINARY_DIR}/sdkconfig.combined", add a small sdkconfig.defaults
fragment containing CONFIG_MBEDTLS_SSL_ALPN=y and
CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=y, and set/export SDKCONFIG_DEFAULTS (or
register that fragment with the build) so the settings are picked up at
configure time; alternatively, if you must patch the generated file, perform the
modification from a custom CMake post-configure/custom command that runs after
ESP-IDF produces sdkconfig.combined to avoid the timing issue.
Comment on lines
+24
to
+45
| set(_mimi_extmod_cmake "${MICROPY_HELPER_DIR}/micropython/extmod/extmod.cmake") | ||
| if(EXISTS "${_mimi_extmod_cmake}") | ||
| file(READ "${_mimi_extmod_cmake}" _mimi_extmod_src) | ||
| string(REPLACE | ||
| "add_library(micropy_extmod_btree OBJECT" | ||
| "add_library(micropy_extmod_btree STATIC" | ||
| _mimi_extmod_src | ||
| "${_mimi_extmod_src}" | ||
| ) | ||
| file(WRITE "${_mimi_extmod_cmake}" "${_mimi_extmod_src}") | ||
| endif() | ||
| set(_mimi_mpy_common "${MICROPY_HELPER_DIR}/micropython/ports/esp32/esp32_common.cmake") | ||
| if(EXISTS "${_mimi_mpy_common}") | ||
| file(READ "${_mimi_mpy_common}" _mimi_mpy_common_src) | ||
| string(REGEX REPLACE | ||
| "[^\n]*target_link_libraries\\([^\n]*micropy_extmod_btree[^\n]*\\)\n" | ||
| "" | ||
| _mimi_mpy_common_src | ||
| "${_mimi_mpy_common_src}" | ||
| ) | ||
| file(WRITE "${_mimi_mpy_common}" "${_mimi_mpy_common_src}") | ||
| endif() |
There was a problem hiding this comment.
Modifying files in managed_components/ breaks reproducibility.
Writing to extmod.cmake (Line 33) and esp32_common.cmake (Line 44) mutates files managed by the Component Manager. This causes:
- Dirty working tree warnings in CI
- Potential conflicts on subsequent
idf.py reconfigurewhen Component Manager updates - Non-reproducible builds if patches silently fail
Prefer a downstream fix: after include(mpy.cmake), adjust target properties rather than patching upstream source files. If upstream patching is unavoidable, consider versioning the patch or contributing the fix upstream.
Comment on lines
+77
to
+84
| if(EXISTS "${CMAKE_BINARY_DIR}/sdkconfig.combined") | ||
| file(READ "${CMAKE_BINARY_DIR}/sdkconfig.combined" _sc) | ||
| string(REPLACE "CONFIG_MBEDTLS_SSL_ALPN=n" "CONFIG_MBEDTLS_SSL_ALPN=y" _sc "${_sc}") | ||
| string(REPLACE "CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=n" "CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=y" _sc "${_sc}") | ||
| file(WRITE "${CMAKE_BINARY_DIR}/sdkconfig.combined" "${_sc}") | ||
| file(APPEND "${CMAKE_BINARY_DIR}/sdkconfig.combined" | ||
| "\nCONFIG_MBEDTLS_SSL_ALPN=y\nCONFIG_MBEDTLS_CERTIFICATE_BUNDLE=y\n") | ||
| endif() |
There was a problem hiding this comment.
Redundant patching and timing issue with sdkconfig.combined.
-
Redundancy: Lines 79-80 replace
=nwith=y, then Lines 82-83 append the same settings. If the replace succeeded, the append creates duplicates; if the keys didn't exist, the replace was a no-op anyway. -
Timing: On a clean configure,
${CMAKE_BINARY_DIR}/sdkconfig.combinedtypically doesn't exist until ESP-IDF's build system generates it. This block will be skipped, and the intended config won't apply.
Consider moving this logic to a post-configure hook or using SDKCONFIG_DEFAULTS to inject a fragment file with the required settings instead of patching generated files.
Proposed simplification (remove redundant append)
if(EXISTS "${CMAKE_BINARY_DIR}/sdkconfig.combined")
file(READ "${CMAKE_BINARY_DIR}/sdkconfig.combined" _sc)
string(REPLACE "CONFIG_MBEDTLS_SSL_ALPN=n" "CONFIG_MBEDTLS_SSL_ALPN=y" _sc "${_sc}")
string(REPLACE "CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=n" "CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=y" _sc "${_sc}")
file(WRITE "${CMAKE_BINARY_DIR}/sdkconfig.combined" "${_sc}")
- file(APPEND "${CMAKE_BINARY_DIR}/sdkconfig.combined"
- "\nCONFIG_MBEDTLS_SSL_ALPN=y\nCONFIG_MBEDTLS_CERTIFICATE_BUNDLE=y\n")
endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakeLists.txt` around lines 77 - 84, The current block that reads and then
appends CONFIG_MBEDTLS_SSL_ALPN and CONFIG_MBEDTLS_CERTIFICATE_BUNDLE is
redundant and can race with ESP-IDF generating sdkconfig.combined; remove the
read/replace + append hack and instead inject the settings reliably via
ESP-IDF's SDKCONFIG_DEFAULTS mechanism or a post-configure hook. Concretely:
delete the file(READ)/string(REPLACE)/file(APPEND) sequence operating on
"${CMAKE_BINARY_DIR}/sdkconfig.combined", add a small sdkconfig.defaults
fragment containing CONFIG_MBEDTLS_SSL_ALPN=y and
CONFIG_MBEDTLS_CERTIFICATE_BUNDLE=y, and set/export SDKCONFIG_DEFAULTS (or
register that fragment with the build) so the settings are picked up at
configure time; alternatively, if you must patch the generated file, perform the
modification from a custom CMake post-configure/custom command that runs after
ESP-IDF produces sdkconfig.combined to avoid the timing issue.
76c666c to
3f765e2
Compare
IRONICBo
force-pushed
the
feat/micropython
branch
from
3f765e2 to
5cf61e6
Compare
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
1 participant
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.
Add MicroPython integration scaffolding, compatibility stubs, and build configuration updates for ESP32-S3.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes