d0 tuning #63
d0 tuning #63
Conversation
WalkthroughThis PR adjusts ESP32 firmware configurations, updates Ethernet settings file path, implements conditional asynchronous callback execution for a Moonlight feature behind the FT_MOONLIGHT macro, and modifies FreeRTOS task stack sizes with explicit core affinity assignments. Changes
Sequence DiagramsequenceDiagram
participant Module as Module
participant TaskQueue as TaskQueue<br/>(runInAppTask)
participant AppTask as AppTask<br/>(FreeRTOS)
rect rgb(230, 245, 255)
Note over Module,AppTask: FT_MOONLIGHT Enabled
Module->>TaskQueue: Enqueue callback<br/>(onReOrderSwap/onUpdate)
TaskQueue->>AppTask: Process queued callback<br/>when idle
AppTask->>AppTask: Execute callback
end
rect rgb(255, 245, 230)
Note over Module,AppTask: FT_MOONLIGHT Disabled
Module->>Module: Execute callback directly<br/>(synchronous)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
firmware/esp32-d0.ini(2 hunks)firmware/esp32-p4.ini(0 hunks)firmware/esp32-s3.ini(0 hunks)lib/framework/EthernetSettingsService.h(1 hunks)src/MoonBase/Module.cpp(2 hunks)src/MoonBase/Utilities.cpp(1 hunks)src/MoonBase/Utilities.h(1 hunks)src/main.cpp(2 hunks)
💤 Files with no reviewable changes (2)
- firmware/esp32-p4.ini
- firmware/esp32-s3.ini
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonBase/Module.cpp (1)
src/MoonBase/NodeManager.h (2)
onReOrderSwap(287-298)onUpdate(134-285)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
firmware/esp32-d0.ini (1)
89-89: LGTM: Consistent HTTPD stack sizing across D0 environments.The HTTPD_STACK_SIZE of 6144 bytes is now consistent across all esp32-d0 environments. The reasoning that MoonBase-only builds need more HTTPD stack due to lack of app tasks is documented and sound.
Also applies to: 107-107
src/main.cpp (2)
299-299: Good: Explicit core affinity improves predictability.Both tasks are now explicitly pinned to core 1, which makes the threading behavior more predictable and easier to reason about.
Also applies to: 308-308
304-304: Stack reallocation between tasks needs verification.The driverTask stack size for non-PSRAM boards increased from 3KB to 4KB, which mirrors the 1KB reduction in effectTask. While this maintains total stack allocation, ensure this redistribution aligns with actual runtime requirements of each task.
lib/framework/EthernetSettingsService.h (1)
36-36: Directory creation is handled automatically; confirm breaking change intent.The
/.config/directory is created automatically byFSPersistence::mkdirs()during the first write operation (lib/framework/FSPersistence.h:160–170), so explicit directory creation logic is not required.However, migration logic is not present: there are no references to the old
/config/ethernetSettings.jsonpath in the codebase. This represents an intentional breaking change—existing deployments using/config/ethernetSettings.jsonwill lose their settings and start fresh with the new path.All code references correctly use the
ETHERNET_SETTINGS_FILEmacro (EthernetSettingsService.cpp:24), consistent with other services (WiFi, MQTT, NTP, Security, AP).Verify that this breaking change is acceptable for your deployment scenario.
src/MoonBase/Utilities.h (1)
383-387: LGTM - Conditional compilation of task queue globals.The extern declarations are now properly guarded behind
FT_MOONLIGHT, making these globals optional. This aligns with the changes in Utilities.cpp where the actual definitions are similarly guarded.src/MoonBase/Utilities.cpp (1)
166-170: LGTM - Conditional compilation matches header declarations.The definitions are properly guarded behind
FT_MOONLIGHT, consistent with the extern declarations in Utilities.h.src/MoonBase/Module.cpp (2)
138-148: Same lifetime and macro concerns as checkReOrderSwap.The same two concerns apply here:
Macro inconsistency: Uses
#if FT_ENABLED(FT_MOONLIGHT)instead of#if FT_MOONLIGHTused in Utilities files.Lambda lifetime: Captures
thisby reference whileupdatedItemis captured by value. Themutablekeyword is appropriate sinceonUpdatemay modify the captured copy, but the reference tothiscreates a potential use-after-free if the Module is destroyed before task execution.Note: The comment on line 142 states "mutable as updatedItem is called by reference (&)" which is slightly misleading—
mutableis needed becauseonUpdatetakes a non-const reference and may modify the captured copy.
107-115: Lambda lifetime concern is invalid; Module objects are global with guaranteed lifetime.The review comment's concerns do not hold upon verification:
Lambda lifetime is safe: Module subclass instances (ModuleDevices, ModuleIO, ModuleEffects, etc.) are created as global objects in src/main.cpp (lines 93–113), not dynamically allocated. Global objects persist for the entire program execution. Since runInAppTask lambdas are processed in the continuous loopTask, the Module objects are guaranteed to exist when lambdas execute. There is no use-after-free risk.
Macro consistency is a non-issue:
FT_ENABLED(FT_MOONLIGHT)expands toFT_MOONLIGHT(FT_ENABLED is a passthrough macro defined in lib/framework/Features.h:18). Both forms are functionally identical. While the codebase predominantly uses#if FT_MOONLIGHTdirectly, the wrapper form in Module.cpp is merely a style variation, not a correctness problem.Likely an incorrect or invalid review comment.
| xTaskCreateUniversal(effectTask, // task function | ||
| "AppEffectTask", // name | ||
| (psramFound() ? 6 : 4) * 1024, // d0-tuning... stack size (without livescripts we can do with 12...). updated from 4 to 6 to support preset loop | ||
| (psramFound() ? 6 : 3) * 1024, // d0-tuning... stack size (without livescripts we can do with 12...). updated from 4 to 6 to support preset loop |
There was a problem hiding this comment.
🧩 Analysis chain
Verify stack sufficiency and update misleading comment.
The effectTask stack size for non-PSRAM boards has been reduced from 4KB to 3KB (25% reduction). The inline comment states "updated from 4 to 6 to support preset loop," which appears to contradict the actual change.
Concerns:
- The 1KB reduction could lead to stack overflow if the task requires the full 4KB
- The comment is confusing and doesn't accurately describe the change
Please verify that 3KB is sufficient for effectTask on non-PSRAM ESP32-D0 boards, and update the comment to accurately reflect the stack size changes.
🏁 Script executed:
#!/bin/bash
# Search for any stack overflow detection or watermark checks
rg -nP --type=cpp 'uxTaskGetStackHighWaterMark|Stack.*overflow|stack.*remain' -A 2Length of output: 2731
🏁 Script executed:
#!/bin/bash
# Get context around line 295 in src/main.cpp to see task creation
sed -n '280,310p' src/main.cppLength of output: 1610
🏁 Script executed:
#!/bin/bash
# Search for effectTask definition and its operations
rg -nP --type=cpp 'effectTask|void.*effect' -A 5 | head -100Length of output: 2173
🏁 Script executed:
#!/bin/bash
# Look for any stack-related warnings or past issues with this task
rg -nP --type=cpp 'effect.*stack|effectTask.*size' -B 2 -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get full effectTask implementation
sed -n '121,145p' src/main.cppLength of output: 753
🏁 Script executed:
#!/bin/bash
# Search for layerP.step() to understand call depth
rg -nP --type=cpp 'layerP\.step|void step\(' -A 3 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any comments mentioning stack tuning or the "4 to 6" or "4 to 3" change
rg -nP --type=cpp 'tuning|preset.*loop|stack.*size' -B 1 -A 1Length of output: 1185
🏁 Script executed:
#!/bin/bash
# Find layerP.loop implementation - likely in layer-related files
find src -name "*.h" -o -name "*.cpp" | xargs grep -l "layerP" | head -5Length of output: 146
🏁 Script executed:
#!/bin/bash
# Search for loop() method definition in layer files
rg -nP --type=cpp 'void loop\(\)|void loop20ms\(\)' -B 2 -A 8 | head -100Length of output: 7897
🏁 Script executed:
#!/bin/bash
# Check git log/blame for this line to see what was actually changed
git log -p --all -S "updated from 4 to" -- src/main.cpp 2>/dev/null | head -100Length of output: 59
Fix misleading comment and verify 3KB stack is sufficient.
The effectTask stack size was reduced from 4KB to 3KB for non-PSRAM boards (25% reduction). The inline comment stating "updated from 4 to 6" is incorrect—the actual change is 4→3 for non-PSRAM (PSRAM remains 6KB unchanged).
Concerns:
- Comment contradicts the code change and must be corrected
- The 1KB reduction on non-PSRAM requires validation that 3KB suffices for effectTask's layerP.loop() call chain
Update the comment to accurately reflect: "reduced from 4 to 3 for non-PSRAM to..." and verify the stack is adequate or document the tradeoffs made.
🤖 Prompt for AI Agents
In src/main.cpp around line 295, the inline comment next to the effectTask stack
size is incorrect and misleading (it currently says "updated from 4 to 6" while
the code uses 6KB for PSRAM and 3KB for non-PSRAM); update the comment to
accurately state that non-PSRAM boards were reduced from 4KB to 3KB while PSRAM
remains 6KB, and explain the reason/tradeoff (e.g., to save RAM while accepting
tighter stack headroom). Then verify 3KB is sufficient by exercising
effectTask's call chain (specifically layerP.loop() and its callees) under
worst-case conditions (deep recursion, large local buffers, debug builds) using
an instrumented run or stack high-water mark check; if stack overflows or margin
is too small, either increase the non-PSRAM stack back to 4KB or refactor to
reduce stack usage, and document the chosen outcome in the comment.
Back end
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.