Conversation
frontend ======= - Row renderer, Module: add onFilterChange - Monitor.svelte: add clearVertices (redraw bug) backend ======= - Ethernet and WiFi settings: add psram check to disable wifi - Module: processUpdatedItem: onUpdate not for filters
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds typed frontend filter handling with a separate onFilterChange path; prevents UI-only "_filter" properties from triggering module onUpdate; unifies system hostname and network helpers across Wi‑Fi/Ethernet; emits a periodic "status" socket event for frontend telemetry; adds optional local CRGB buffering and a free fade helper for effects. Changes
Sequence Diagram(s)sequenceDiagram
participant ESP as ESP32SvelteKit
participant Socket as SocketServer
participant Client as BrowserClient
participant Telemetry as FrontendTelemetry
participant UI as UIComponents
ESP->>Socket: emit "status" {safeMode, restartNeeded, saveNeeded, hostName}
Socket->>Client: push "status" event
Client->>Telemetry: handleStatus(data) -> telemetry.setStatus(data)
Telemetry->>UI: reactive store update (status, hostName, flags)
UI->>UI: re-render status indicators / hostname
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonBase/Module.cpp`:
- Around line 339-340: The condition currently skips onUpdate for any name
containing "_filter"; change it to only skip when the key ends with the suffix
"_filter" by replacing the substring check on updatedItem.name with a suffix
check (e.g., use an endsWith/hasSuffix method or compare the trailing substring)
so onUpdate(updatedItem) is only suppressed for filter-only fields; update the
conditional around updatedItem.name and onUpdate accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c57a9cfa-f515-4ade-b1e0-c9eb128ccb8e
📒 Files selected for processing (8)
interface/src/lib/components/moonbase/RowRenderer.svelteinterface/src/routes/moonbase/module/Module.svelteinterface/src/routes/moonbase/monitor/Monitor.svelteinterface/src/routes/system/status/SystemStatus.sveltelib/framework/EthernetSettingsService.cpplib/framework/WWWData.hlib/framework/WiFiSettingsService.cppsrc/MoonBase/Module.cpp
Docs ==== - Module IO: ethernet type board default Backend - AP settingsservice: startAP also when ethernet - Module IO: ethernet type board default
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/framework/APSettingsService.cpp (1)
146-146: Same consistency concern: preferETH.connected()overETH.localIP().For the same reasons as above, using
!ETH.connected()would be more consistent with the rest of the codebase:- if (apActive && _state.provisionMode != AP_MODE_ALWAYS && WiFi.status() == WL_CONNECTED && !ETH.localIP()) // 🌙 not lingering when Ethernet is also connected + if (apActive && _state.provisionMode != AP_MODE_ALWAYS && WiFi.status() == WL_CONNECTED && !ETH.connected()) // 🌙 not lingering when Ethernet is also connected🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/APSettingsService.cpp` at line 146, The condition uses ETH.localIP() for detecting Ethernet presence but should use ETH.connected() for consistency; update the if expression that checks apActive, _state.provisionMode != AP_MODE_ALWAYS and WiFi.status() == WL_CONNECTED to replace the !ETH.localIP() check with !ETH.connected() so the test uses ETH.connected() like the rest of the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/framework/APSettingsService.cpp`:
- Line 146: The condition uses ETH.localIP() for detecting Ethernet presence but
should use ETH.connected() for consistency; update the if expression that checks
apActive, _state.provisionMode != AP_MODE_ALWAYS and WiFi.status() ==
WL_CONNECTED to replace the !ETH.localIP() check with !ETH.connected() so the
test uses ETH.connected() like the rest of the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea45ff1d-df46-4159-842c-ff8ae28a3881
📒 Files selected for processing (4)
docs/moonbase/inputoutput.mdlib/framework/APSettingsService.cpplib/framework/WWWData.hsrc/MoonBase/Modules/ModuleIO.h
…lar mappings - replace ETH.localIP() with ETH.connected() - FastLED effects: needsLocalBuf if !1:1mapping and not 3 channels to display on irregular mappings
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/MoonBase/Modules/ModuleDevices.h (1)
122-122: Optional: deduplicate the network-connected predicate.Tiny cleanup, but extracting this check avoids future drift across loop methods.
♻️ Proposed refactor
class ModuleDevices : public Module { public: + bool isNetworkConnected() const { return WiFi.isConnected() || ETH.connected(); } + NetworkUDP deviceUDP; uint16_t deviceUDPPort = 65506; bool deviceUDPConnected = false; @@ void loop20ms() override { Module::loop20ms(); - if (!WiFi.isConnected() && !ETH.connected()) return; + if (!isNetworkConnected()) return; @@ void loop10s() override { - if (!WiFi.isConnected() && !ETH.connected()) return; + if (!isNetworkConnected()) return;Also applies to: 130-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Modules/ModuleDevices.h` at line 122, Extract the repeated predicate "WiFi.isConnected() && ETH.connected()" into a single helper method (e.g., ModuleDevices::isNetworkConnected() or connectedToNetwork()) and replace both occurrences (the if guards currently using "if (!WiFi.isConnected() && !ETH.connected()) return;") with a single call using that helper (e.g., "if (!isNetworkConnected()) return;"); implement the helper to return (WiFi.isConnected() || ETH.connected()) and keep its visibility appropriate (private/static) so both loop sites can call it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Nodes/Effects/E_FastLED.h`:
- Around line 179-180: The code currently only lazy-inits canvas_buf (via if
(!canvas_buf) onSizeChanged(Coord3D())) which leaves buffer mode stale when
mapping/layout params change (e.g., oneToOneMapping, channelsPerLight) without a
size change; update the logic so that when mapping/channel/layout changes
(triggered by onUpdate()/requestMappings()/onLayout()), you force a refresh of
the buffer mode instead of only checking for null — for example, call
onSizeChanged(Coord3D()) or reinitialize/refresh canvas_buf whenever
oneToOneMapping or channelsPerLight changes (the places referencing canvas_buf
initialization and the mapping/layout update paths), ensuring the code at the
two existing init sites refreshes mode on mapping changes, not just when
canvas_buf is null.
- Around line 165-176: The code calls reallocMB2 while canvas_buf may alias
non-owned layerP.lights.channelsE (canvas_bufOwned == false), which violates
ownership; change the logic in onSizeChanged to avoid reallocMB2 when
canvas_bufOwned is false: if needsLocalBuf() and canvas_bufOwned==true use
reallocMB2 to resize, but if needsLocalBuf() and canvas_bufOwned==false allocate
a new owned buffer (use alloc/allocMB2 or equivalent) and set
canvas_bufOwned=true (do not realloc the aliased pointer); likewise when
switching to non-local buffer, free only if canvas_bufOwned is true before
assigning canvas_buf = (CRGB*)layerP.lights.channelsE and set
canvas_bufOwned=false. Apply the same ownership-aware change to the other
occurrence that currently calls reallocMB2 (the similar block around canvas_buf
usage at the other location).
---
Nitpick comments:
In `@src/MoonBase/Modules/ModuleDevices.h`:
- Line 122: Extract the repeated predicate "WiFi.isConnected() &&
ETH.connected()" into a single helper method (e.g.,
ModuleDevices::isNetworkConnected() or connectedToNetwork()) and replace both
occurrences (the if guards currently using "if (!WiFi.isConnected() &&
!ETH.connected()) return;") with a single call using that helper (e.g., "if
(!isNetworkConnected()) return;"); implement the helper to return
(WiFi.isConnected() || ETH.connected()) and keep its visibility appropriate
(private/static) so both loop sites can call it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21cfcac6-8442-44fa-b4b1-9e7496e83aa9
📒 Files selected for processing (8)
lib/framework/APSettingsService.cppsrc/MoonBase/Modules/ModuleDevices.hsrc/MoonBase/Modules/ModuleTasks.hsrc/MoonLight/Layers/VirtualLayer.cppsrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Modules/ModuleLiveScripts.hsrc/MoonLight/Nodes/Drivers/D_ArtnetIn.hsrc/MoonLight/Nodes/Effects/E_FastLED.h
frontend ======= - telemetry, models, layout, statusbar: move status from telemetry.rssi to telemetry.status backend ======= - ESP32Sveltekit: emit status every second - WiFi Settings: remove status from rssi - FastLED effects: tweaks
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
interface/src/lib/stores/telemetry.ts (1)
44-54:⚠️ Potential issue | 🟠 Major
setRSSI()can no longer represent a disconnected socket.
interface/src/routes/+layout.svelteLine 113 now callstelemetry.setRSSI({ rssi: 0, ssid: '' }), but0still passes the numeric check here, so this store recordsdisconnected: false.interface/src/routes/statusbar.sveltestill keys the offline icon off$telemetry.rssi.disconnected, so the UI shows a Wi‑Fi indicator after the websocket closes.💡 Suggested fix
import { writable } from 'svelte/store'; import type { RSSI } from '../types/models'; import type { SystemStatus } from '../types/models'; // 🌙 import type { Battery } from '../types/models'; import type { OTAStatus } from '../types/models'; import type { Ethernet } from '../types/models'; + +type RSSIState = RSSI & { disconnected?: boolean }; @@ - setRSSI: (data: RSSI) => { - if (!isNaN(Number(data.rssi))) { + setRSSI: (data: RSSIState) => { + if (data.disconnected) { + update((telemetry_data) => ({ + ...telemetry_data, + rssi: { rssi: 0, ssid: data.ssid, disconnected: true } + })); + } else if (!isNaN(Number(data.rssi))) { update((telemetry_data) => ({ ...telemetry_data, rssi: { rssi: Number(data.rssi), ssid: data.ssid, disconnected: false } }));Then update
interface/src/routes/+layout.svelteLine 113 to passdisconnected: trueon close.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/lib/stores/telemetry.ts` around lines 44 - 54, setRSSI currently treats rssi===0 as valid because it only checks Number(data.rssi), so calling telemetry.setRSSI({ rssi: 0, ssid: '' }) incorrectly marks disconnected:false; modify setRSSI to honor an explicit disconnected flag or derive disconnected from the payload by computing const numericRssi = Number(data.rssi) and const disconnected = data.disconnected === true || isNaN(numericRssi); then call update(...) with rssi: { rssi: isNaN(numericRssi) ? 0 : numericRssi, ssid: data.ssid, disconnected } so callers (e.g., +layout.svelte) can pass disconnected:true on close and the statusbar will render correctly.
♻️ Duplicate comments (1)
src/MoonLight/Nodes/Effects/E_FastLED.h (1)
180-182:⚠️ Potential issue | 🟠 MajorRefresh non-owned
canvasBufwhenchannelsEpointer changes.Line 181 and Line 1396 only refresh on null or ownership-mode flip. When
canvasBufOwned == false,canvasBufcan become stale iflayerP.lights.channelsEchanges, leading to writes into the wrong frame buffer. Please include alias and size checks in the refresh guard.🔧 Proposed fix
- // Refresh buffer if uninitialized or if mapping/channel config changed without a size change - if (!canvasBuf || canvasBufOwned != needsLocalBuf()) onSizeChanged(Coord3D()); + // Refresh buffer if uninitialized or if mapping/channel config changed without a size change + const size_t expectedLights = (size_t)layer->size.x * (size_t)layer->size.y; + const bool shouldOwn = needsLocalBuf(); + if (!canvasBuf || + canvasBufOwned != shouldOwn || + canvasBufSize != expectedLights || + (!canvasBufOwned && canvasBuf != (CRGB*)layerP.lights.channelsE)) { + onSizeChanged(Coord3D()); + } if (!canvasBuf) return;- // Refresh buffers if uninitialized or if mapping/channel config changed without a size change - if (!canvasBuf || canvasBufOwned != needsLocalBuf()) allocBuffers(); + // Refresh buffers if uninitialized or if mapping/channel config changed without a size change + const size_t expectedLights = (size_t)W * (size_t)H; + const bool shouldOwn = needsLocalBuf(); + if (!canvasBuf || + canvasBufOwned != shouldOwn || + canvasBufSize != expectedLights || + (!canvasBufOwned && canvasBuf != (CRGB*)layerP.lights.channelsE)) { + allocBuffers(); + } if (!canvasBuf || !tempBuf || !xProf || !yProf) return;Based on learnings: in
src/main.cppeffect/driver flow,channelsD/channelsEpointer swaps can happen at runtime, so cached aliases must be refreshed whenlayerP.lights.channelsEchanges.Also applies to: 1395-1397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_FastLED.h` around lines 180 - 182, The refresh guard must also detect when a non-owned buffer's alias changes: in addition to checking !canvasBuf and canvasBufOwned != needsLocalBuf(), add a condition to call onSizeChanged(Coord3D()) when canvasBuf is non-owned and the cached channels pointer/alias used for mapping differs from layerP.lights.channelsE or when the mapped size changed; after calling onSizeChanged update the cached alias/size. Specifically, update the logic around canvasBuf/canvasBufOwned and onSizeChanged to include a check like (canvasBufOwned == false && cachedChannelsPointer != layerP.lights.channelsE) and ensure you refresh the cachedChannelsPointer (and cached size) when you rebuild the buffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/routes/`+layout.svelte:
- Around line 113-114: Don't overwrite the persisted hostName on transient
socket disconnects; instead of calling telemetry.setStatus with hostName:
'MoonLight' in the disconnect handler, preserve the existing stored hostName (or
omit hostName from the payload) so the persisted title isn't cleared. Update the
disconnect logic around telemetry.setStatus(...) to read the current persisted
status/hostName (via the telemetry/status getter or existing state) and pass
that hostName through, or remove the hostName property from the call entirely so
it doesn't erase the remembered name.
In `@lib/framework/ESP32SvelteKit.cpp`:
- Around line 323-329: The current preprocessor-based selection always picks
WiFi when both FT_WIFI and FT_ETHERNET are compiled in, causing doc["hostName"]
to be wrong if Ethernet is the active uplink; replace the compile-time branches
with a runtime check that selects the active interface. Modify the block that
sets doc["hostName"] to query the interface state at runtime (for example by
calling an existing runtime indicator such as
_ethernetSettingsService.isActive() or _wifiSettingsService.isActive(), or
whatever method returns the active uplink in this codebase) and set
doc["hostName"] to _ethernetSettingsService.getHostname() when Ethernet is
active, otherwise to _wifiSettingsService.getHostname(), falling back to
"MoonLight" if neither is active. Ensure FT_WIFI/FT_ETHERNET remain guarded so
calls only occur when those services exist.
---
Outside diff comments:
In `@interface/src/lib/stores/telemetry.ts`:
- Around line 44-54: setRSSI currently treats rssi===0 as valid because it only
checks Number(data.rssi), so calling telemetry.setRSSI({ rssi: 0, ssid: '' })
incorrectly marks disconnected:false; modify setRSSI to honor an explicit
disconnected flag or derive disconnected from the payload by computing const
numericRssi = Number(data.rssi) and const disconnected = data.disconnected ===
true || isNaN(numericRssi); then call update(...) with rssi: { rssi:
isNaN(numericRssi) ? 0 : numericRssi, ssid: data.ssid, disconnected } so callers
(e.g., +layout.svelte) can pass disconnected:true on close and the statusbar
will render correctly.
---
Duplicate comments:
In `@src/MoonLight/Nodes/Effects/E_FastLED.h`:
- Around line 180-182: The refresh guard must also detect when a non-owned
buffer's alias changes: in addition to checking !canvasBuf and canvasBufOwned !=
needsLocalBuf(), add a condition to call onSizeChanged(Coord3D()) when canvasBuf
is non-owned and the cached channels pointer/alias used for mapping differs from
layerP.lights.channelsE or when the mapped size changed; after calling
onSizeChanged update the cached alias/size. Specifically, update the logic
around canvasBuf/canvasBufOwned and onSizeChanged to include a check like
(canvasBufOwned == false && cachedChannelsPointer != layerP.lights.channelsE)
and ensure you refresh the cachedChannelsPointer (and cached size) when you
rebuild the buffer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91dea5c4-265f-4230-a895-092a38e29532
📒 Files selected for processing (8)
interface/src/lib/stores/telemetry.tsinterface/src/lib/types/models.tsinterface/src/routes/+layout.svelteinterface/src/routes/statusbar.sveltelib/framework/ESP32SvelteKit.cpplib/framework/WWWData.hlib/framework/WiFiSettingsService.cppsrc/MoonLight/Nodes/Effects/E_FastLED.h
frontend ======= - layout: set status hostname default from telemetry backend ======= - ESP32SvelteKit: add getSystemHostname, use for MDNS and doc["hostName"]; - EthernetSettingsService::configureNetwork: use systemHostnameProvider - WiFiSettings: add loadState for early hostName access - Devices, Lights control: use systemHostname
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
interface/src/routes/+layout.svelte (1)
57-83:⚠️ Potential issue | 🟡 MinorUnsubscribe
handleErroron teardown.
addEventListeners()still registers'error'at Line 60, butremoveEventListeners()never removes it. Re-initializing the socket on the same layout instance will stack duplicate error handlers.♻️ Proposed fix
const removeEventListeners = () => { socket.off('analytics', handleAnalytics); socket.off('open', handleOpen); socket.off('close', handleClose); + socket.off('error', handleError); socket.off('rssi', handleNetworkStatus); socket.off('status', handleStatus); // 🌙 socket.off('notification', handleNotification); socket.off('battery', handleBattery); socket.off('otastatus', handleOTA);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/`+layout.svelte around lines 57 - 83, addEventListeners registers socket.on('error', handleError) but removeEventListeners never removes it; update removeEventListeners to call socket.off('error', handleError) (alongside the other socket.off calls) so handleError is unsubscribed on teardown, referencing the addEventListeners/removeEventListeners functions and the handleError listener.
♻️ Duplicate comments (1)
lib/framework/ESP32SvelteKit.h (1)
231-253:⚠️ Potential issue | 🟠 MajorWiFi's default hostname shadows Ethernet here.
Line 236 always wins once WiFi state has been loaded, because
WiFiSettings::update()default-initializeshostnamewithFACTORY_WIFI_HOSTNAME. On FT_WIFI + FT_ETHERNET builds, the Ethernet hostname setting can no longer affect DHCP/mDNS/HA naming even when WiFi is unused. Either add a real shared hostname setting or only prefer the WiFi value when that interface is actually active or explicitly configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/ESP32SvelteKit.h` around lines 231 - 253, getSystemHostname currently prefers the WiFi hostname unconditionally, which allows the WiFiSettings default (set in WiFiSettings::update to FACTORY_WIFI_HOSTNAME) to shadow a valid Ethernet hostname; change the selection to prefer an actually active or explicitly configured WiFi value. Modify getSystemHostname (and reference _wifiSettingsService and _ethernetSettingsService) so the WiFi hostname is only returned if the WiFi interface is up/connected or the stored hostname differs from FACTORY_WIFI_HOSTNAME (i.e., explicitly configured); otherwise fall through to check the Ethernet hostname and then the MAC-based fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/framework/ESP32SvelteKit.cpp`:
- Around line 77-79: The code currently calls WiFiSettingsService::initWiFi()
which unconditionally invokes _fsPersistence.readFromFS(), causing a second
side-effecting read/write of wifiSettings.json even though
_wifiSettingsService.loadState() already loaded state; update initWiFi() to
reuse the previously-loaded state (do not call _fsPersistence.readFromFS() if
loadState() has already populated the in-memory settings) and wrap any remaining
on-disk reads/writes behind the FT_WIFI build flag so that on FT_WIFI=0 builds
WiFi init does not touch the file; ensure getSystemHostname() still returns the
configured hostname by relying on the loaded state rather than performing a new
read.
---
Outside diff comments:
In `@interface/src/routes/`+layout.svelte:
- Around line 57-83: addEventListeners registers socket.on('error', handleError)
but removeEventListeners never removes it; update removeEventListeners to call
socket.off('error', handleError) (alongside the other socket.off calls) so
handleError is unsubscribed on teardown, referencing the
addEventListeners/removeEventListeners functions and the handleError listener.
---
Duplicate comments:
In `@lib/framework/ESP32SvelteKit.h`:
- Around line 231-253: getSystemHostname currently prefers the WiFi hostname
unconditionally, which allows the WiFiSettings default (set in
WiFiSettings::update to FACTORY_WIFI_HOSTNAME) to shadow a valid Ethernet
hostname; change the selection to prefer an actually active or explicitly
configured WiFi value. Modify getSystemHostname (and reference
_wifiSettingsService and _ethernetSettingsService) so the WiFi hostname is only
returned if the WiFi interface is up/connected or the stored hostname differs
from FACTORY_WIFI_HOSTNAME (i.e., explicitly configured); otherwise fall through
to check the Ethernet hostname and then the MAC-based fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 782ca143-9222-4dc1-98d8-5dda88f1127b
📒 Files selected for processing (9)
interface/src/routes/+layout.sveltelib/framework/ESP32SvelteKit.cpplib/framework/ESP32SvelteKit.hlib/framework/EthernetSettingsService.cpplib/framework/EthernetSettingsService.hlib/framework/WWWData.hlib/framework/WiFiSettingsService.hsrc/MoonBase/Modules/ModuleDevices.hsrc/MoonLight/Modules/ModuleLightsControl.h
Collaborator
Author
|
@coderabbitai, regarding
No fix needed. The current behavior is correct by design:
The WiFi hostname is intentionally the primary source because that's where users configure it (the WiFi settings page). The Ethernet hostname is a fallback for boards without WiFi. This is the correct design. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
frontend ======= - menu: add page.data.features.wifi - status bar: do not show WiFi not connected backend ======= - ESP32SvelteKit: use if FT_ENABLED(FT_WIFI) for Wifi functionality, add networkIsConnected and networkLocalIP helper functions, lps functions local - APSettingsService: add #if FT_ENABLED(FT_ETHERNET) - Feature service: add wifi feature - Restart service: wifi disconnect only if wifi - System status: coprocessor only on P4 with WiFi - Lights control: #if FT_ENABLED(FT_MQTT) - Devices, Tasks, LiveScripts, Artnet, Audio Sync: use the network helper functions
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Collaborator
Author
|
@coderabbitai, please review c2a20e0 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 23750
✅ Overall AssessmentThis is a well-structured commit. The goals are clearly achieved: WiFi code is properly disabled for 🔍 Findings
|
| Area | Status | Notes |
|---|---|---|
networkIsConnected() / networkLocalIP() helpers |
✅ | Clean unification |
FT_WIFI guards throughout |
✅ | Complete and consistent |
FT_MQTT guard in ModuleLightsControl |
✅ | Correct |
lps moved to ESP32SvelteKit public members |
✅ | Cleaner than via AnalyticsService |
socket.off('error') fix |
✅ | Valid bug fix |
RestartService WiFi.disconnect guard |
✅ | Prevents ESP-Hosted crash |
APSettingsService no FT_WIFI on WiFi calls |
ℹ️ | Safe by construction; no change needed |
ModuleLightsControl init-list #if pattern |
ℹ️ | Valid C++; slightly unusual style |
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-17T19:24:12.957Z
Learning: In MoonModules/MoonLight ESP32SvelteKit.h, getSystemHostname() intentionally uses WiFi hostname as the primary source (always non-empty due to FACTORY_WIFI_HOSTNAME defaulting to "#{platform}-#{unique_id}"). On WiFi+Ethernet boards both defaults resolve to the same template, so there is no shadowing. On ethernet-only builds (FT_WIFI=0) the WiFi block is compiled out and falls through to the Ethernet hostname. Hostname must NOT depend on connection state — flip-flopping would break mDNS/DHCP stability. WiFi settings page is the intended user-facing config; Ethernet hostname is a fallback only for boards without WiFi.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 151
File: lib/framework/EthernetSettingsService.cpp:137-143
Timestamp: 2026-03-16T22:55:56.751Z
Learning: In MoonModules/MoonLight EthernetSettingsService.cpp, when Ethernet drops and `_wifiDisabledByEthernet` is cleared in `updateEthernet()`, WiFi reconnection is intentionally NOT triggered immediately. The design relies on `WiFiSettingsService::manageSTA()` picking up the cleared flag on its next periodic loop iteration. The short `WIFI_RECONNECTION_DELAY` (a few seconds) is considered acceptable, and adding a direct cross-service reference to WiFiSettingsService from EthernetSettingsService was explicitly rejected as over-engineering.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 152
File: lib/framework/ESP32SvelteKit.cpp:77-79
Timestamp: 2026-03-17T19:25:51.874Z
Learning: In MoonModules/MoonLight ESP32SvelteKit.cpp begin(): The double call to _fsPersistence.readFromFS() (once via _wifiSettingsService.loadState() early in begin(), and again inside _wifiSettingsService.initWiFi()) is intentional and harmless. readFromFS() is idempotent — same JSON file, same in-memory state. initWiFi() is kept self-contained so it works regardless of whether loadState() was called first. The FT_WIFI=0 concern is already handled by the existing `#if` FT_ENABLED(FT_WIFI) guard around initWiFi(). getSystemHostname() correctly reads from in-memory state via _wifiSettingsService.getHostname(), not from disk.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService: The double initEthernet() call on boot (once at startup, once when the board preset fires readPins()) is verified as working correctly. It is a benign redundant call and adding complexity to avoid it is not warranted.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService + WiFiSettingsService: Simultaneous Ethernet + WiFi drop scenario is verified correct. When both drop: _wifiDisabledByEthernet is cleared in updateEthernet(), ETH.connected() returns false in manageSTA(), so manageSTA() proceeds normally to reconnect WiFi.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService.cpp configureNetwork(): The `else { ETH.begin(); }` fallback after the SPI and RMII branches is intentional. It acts as a safe default for future boards with native/built-in EMAC ethernet that works with default arguments. Currently all configured boards go through the explicit SPI or RMII paths, so this fallback is never reached in practice.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-08T10:36:33.812Z
Learning: In MoonModules/MoonLight D_FastLEDDriver: FastLED RMT5 (default on IDF >= 5) lacks the dual-buffer WiFi-resistance design of FastLED RMT4, causing WebSocket/WiFi throughput degradation at high LED frame rates. FastLED I2S is also incompatible with WiFi on ESP32-S3. MoonLight's D_ParallelLEDDriver (parlio.cpp) avoids this entirely by using its own DMA-streaming PARLIO implementation with zero per-frame ISR callbacks. Fix: add `-D FASTLED_RMT5=0` to platformio.ini for IDF5-based boards to force RMT4's dual-buffer design. This flag may need to be re-enabled even when a specific FastLED commit appeared stable, as RMT5 behavior varies across FastLED master commits.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-26T14:59:22.091Z
Learning: In MoonModules/MoonLight PR `#114`: ModuleDevices deadlock caused by: (1) StatefulService.h line 204-205 bug - after 500ms mutex timeout, continues WITHOUT acquiring mutex, causing race conditions and JSON corruption. (2) receiveUDP() while-loop processes own UDP broadcasts via loopback, creating circular call chain: sendUDP→broadcast→receiveUDP→updateDevices→update→handlers→sendUDP. (3) updateDevices() holds _accessMutex during expensive sort/JSON operations with 15 devices, can exceed 500ms with nested calls. Solution: Fix StatefulService to wait forever or reboot on timeout; skip own broadcasts by checking message.name == ownHostname; optimize updateDevices to sort outside mutex.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-12T18:00:40.667Z
Learning: In MoonModules/MoonLight, the QuinLED DIG-OCTA BrainBoard 32-8L (board_QuinLEDDigOctaV2) uses an original ESP32 (not S3) with an onboard LAN8720A RMII PHY. It does NOT use W5500 or SPI Ethernet. The RMII Ethernet pins are hardware-fixed: REF_CLK=GPIO17, MDIO=GPIO18, TXD0=GPIO19, TX_EN=GPIO21, TXD1=GPIO22, MDC=GPIO23, RXD0=GPIO25, RXD1=GPIO26, CRS_DV=GPIO27. GPIO17 is a clock OUTPUT from ESP32 to LAN8720A, requiring ETH_CLOCK_GPIO17_OUT build flag (the default ETH_CLOCK_GPIO0_IN is wrong for this board). ETH power is controlled by a physical jumper, not a GPIO. The board preset currently does not mark these RMII pins as pin_Ethernet, and no ETH_CLK_MODE/ETH_PHY build flags are set in firmware/esp32-d0.ini.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-08T10:36:33.812Z
Learning: In MoonModules/MoonLight D_FastLEDDriver vs D_ParallelLEDDriver: The PARLIO driver (parlio.cpp) uses `queue_nonblocking = 1` in `parlio_transmit_config_t`, making LED transmission fully fire-and-forget via hardware DMA — zero ISR callbacks during TX, CPU is free for WiFi. FastLED RMT5 (default on IDF5+) uses ISR-driven segment streaming with ISR priority capped at 1-3, which causes constant WiFi interrupt contention and WebSocket throughput degradation at high frame rates. I2S has the same ISR-driven problem. Fix: add `-D FASTLED_RMT5=0` to platformio.ini to force FastLED RMT4, which has an explicit dual-buffer WiFi-resistant design. WARNING: RMT4 and RMT5 cannot coexist in the same binary (startup abort). The affinity="PARLIO" option in FastLED Driver uses FastLED's own PARLIO wrapper, not MoonLight's custom parlio.cpp.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 136
File: src/MoonBase/Modules/ModuleDevices.h:148-148
Timestamp: 2026-03-04T10:07:41.583Z
Learning: In MoonModules/MoonLight, the `// 🌙` (MoonBase addition) and `// 💫` (MoonLight addition) comment markers are ONLY required when modifying upstream SvelteKit files (i.e., files that originate from the upstream ESP32-sveltekit project). Do NOT require or suggest these markers for MoonBase-specific or MoonLight-specific files (such as `src/MoonBase/Modules/ModuleDevices.h` or other files in `src/MoonBase/` or `src/MoonLight/`). The markers indicate where upstream files have been patched.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T14:28:21.568Z
Learning: In MoonModules/MoonLight ModuleIO.h readPins(): The assignIfValid lambda (which called GPIO_IS_VALID_GPIO() before assigning SPI ethernet pin variables) was removed in the ethernet rework commit 59f0149. SPI pin assignments (pin_SPI_SCK, pin_SPI_MISO, pin_SPI_MOSI, pin_PHY_CS, pin_PHY_IRQ) are now assigned directly without GPIO validity checking. For RMII pins this is fine (hardware-fixed), but for user-configurable W5500 SPI pins this is a regression.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-04T09:52:08.096Z
Learning: MoonLight ModuleDevices: To reduce missed packets when many devices send at once, add a 0–20 ms non-blocking jitter only to non-control (status) UDP broadcasts (post-control apply and optionally the 10s heartbeat). Implement by scheduling the send in loop20ms instead of delaying inside addUpdateHandler/httpd context; keep control packets immediate.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T14:28:21.568Z
Learning: In MoonModules/MoonLight ModuleIO.h readPins(): The ETH CLK mode derivation for ESP32 only handles ETH_CLOCK_GPIO0_OUT (gpio==0) and ETH_CLOCK_GPIO17_OUT (all other GPIOs). ETH_CLOCK_GPIO0_IN (external oscillator) and ETH_CLOCK_GPIO16_OUT modes are not handled. This is intentional for current boards but should be extended if boards with external clock input or GPIO16 clock output are added.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/framework/SystemStatus.cpp (1)
211-213:⚠️ Potential issue | 🟡 MinorWiden the coprocessor version buffer.
"v255.255.255"already needs 13 bytes including the terminator, sochar coprocessor[10]truncates multi-digit versions.🧩 Suggested change
- char coprocessor[10]; + char coprocessor[16];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/SystemStatus.cpp` around lines 211 - 213, The coprocessor buffer is too small — change the fixed char coprocessor[10] used with snprintf("v%d.%d.%d", c6_fw_version.major1, c6_fw_version.minor1, c6_fw_version.patch1) so multi-digit fields won't be truncated; replace it with a larger buffer (e.g. char coprocessor[16]) or use a std::string and format into it before assigning to root["coprocessor"], keeping the snprintf/format call targeting the new buffer size and preserving the c6_fw_version.major1/minor1/patch1 references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/framework/APSettingsService.cpp`:
- Around line 75-80: The AP stay-on condition is inverted: when
_state.provisionMode == AP_MODE_DISCONNECTED you want the AP to remain only if
both WiFi is not connected and Ethernet is not connected; update the expression
that currently uses "WiFi.status() != WL_CONNECTED || ETH.connected()" to
require no Ethernet (i.e., use "WiFi.status() != WL_CONNECTED &&
!ETH.connected()") so that ETH.connected() being true will not keep the AP
active; ensure this change is applied within the same conditional that also
checks _recoveryMode and preserves grouping/precedence around
(_state.provisionMode == AP_MODE_DISCONNECTED ... ) || _recoveryMode.
In `@lib/framework/SystemStatus.cpp`:
- Line 204: Replace the live counter usage in SystemStatus.cpp (root["lps_all"]
= esp32sveltekit.lps_all) with the latched snapshot field provided by
ESP32SvelteKit (use the last-computed/latched member such as
esp32sveltekit.lps_all_last or esp32sveltekit.lps_all_latched as declared in
ESP32SvelteKit) so /systemStatus reports the rolled LPS value rather than the
ephemeral live counter; update the assignment to read that latched member
instead of esp32sveltekit.lps_all.
---
Outside diff comments:
In `@lib/framework/SystemStatus.cpp`:
- Around line 211-213: The coprocessor buffer is too small — change the fixed
char coprocessor[10] used with snprintf("v%d.%d.%d", c6_fw_version.major1,
c6_fw_version.minor1, c6_fw_version.patch1) so multi-digit fields won't be
truncated; replace it with a larger buffer (e.g. char coprocessor[16]) or use a
std::string and format into it before assigning to root["coprocessor"], keeping
the snprintf/format call targeting the new buffer size and preserving the
c6_fw_version.major1/minor1/patch1 references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d9ec06d-5ddf-44e0-951d-cbd7fe43b991
📒 Files selected for processing (23)
interface/src/routes/+layout.svelteinterface/src/routes/menu.svelteinterface/src/routes/statusbar.sveltelib/framework/APSettingsService.cpplib/framework/ESP32SvelteKit.cpplib/framework/ESP32SvelteKit.hlib/framework/EthernetSettingsService.cpplib/framework/EthernetSettingsService.hlib/framework/FeaturesService.cpplib/framework/RestartService.hlib/framework/SystemStatus.cpplib/framework/WWWData.hlib/framework/WiFiSettingsService.hsrc/MoonBase/Modules/ModuleDevices.hsrc/MoonBase/Modules/ModuleIO.hsrc/MoonBase/Modules/ModuleTasks.hsrc/MoonLight/Modules/ModuleEffects.hsrc/MoonLight/Modules/ModuleLightsControl.hsrc/MoonLight/Modules/ModuleLiveScripts.hsrc/MoonLight/Nodes/Drivers/D_ArtnetIn.hsrc/MoonLight/Nodes/Drivers/D_ArtnetOut.hsrc/MoonLight/Nodes/Drivers/D_AudioSync.hsrc/MoonLight/Nodes/Effects/E_MoonLight.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/MoonBase/Modules/ModuleDevices.h
- interface/src/routes/+layout.svelte
- src/MoonBase/Modules/ModuleTasks.h
- lib/framework/EthernetSettingsService.cpp
- src/MoonLight/Nodes/Drivers/D_ArtnetIn.h
- src/MoonLight/Modules/ModuleLiveScripts.h
- lib/framework/WiFiSettingsService.h
Comment on lines
+75
to
+80
| (_state.provisionMode == AP_MODE_DISCONNECTED && (WiFi.status() != WL_CONNECTED | ||
| #if FT_ENABLED(FT_ETHERNET) | ||
| || ETH.connected() | ||
| #endif | ||
| )) || // 🌙 keep AP when WiFi disconnected or Ethernet is the active connection | ||
| _recoveryMode) |
There was a problem hiding this comment.
Potential logic inversion in AP management condition.
The condition on line 77 adds || ETH.connected() but this appears inverted. Let me trace the logic:
AP_MODE_DISCONNECTEDmeans "provision AP when device is disconnected from external network"- Original condition:
WiFi.status() != WL_CONNECTED→ AP stays on when WiFi is disconnected - New condition:
WiFi.status() != WL_CONNECTED || ETH.connected()→ AP stays on when WiFi is disconnected OR Ethernet is connected
This means the AP will remain active when Ethernet is connected, which seems counterintuitive—if Ethernet is the active connection, the device IS connected to the network, so the AP should turn off (not stay on).
Should this be && !ETH.connected() instead to mean "WiFi disconnected AND Ethernet not connected"?
Suggested fix
if (_state.provisionMode == AP_MODE_ALWAYS ||
(_state.provisionMode == AP_MODE_DISCONNECTED && (WiFi.status() != WL_CONNECTED
`#if` FT_ENABLED(FT_ETHERNET)
- || ETH.connected()
+ && !ETH.connected()
`#endif`
- )) || // 🌙 keep AP when WiFi disconnected or Ethernet is the active connection
+ )) || // 🌙 keep AP only when both WiFi and Ethernet are disconnected
_recoveryMode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/framework/APSettingsService.cpp` around lines 75 - 80, The AP stay-on
condition is inverted: when _state.provisionMode == AP_MODE_DISCONNECTED you
want the AP to remain only if both WiFi is not connected and Ethernet is not
connected; update the expression that currently uses "WiFi.status() !=
WL_CONNECTED || ETH.connected()" to require no Ethernet (i.e., use
"WiFi.status() != WL_CONNECTED && !ETH.connected()") so that ETH.connected()
being true will not keep the AP active; ensure this change is applied within the
same conditional that also checks _recoveryMode and preserves
grouping/precedence around (_state.provisionMode == AP_MODE_DISCONNECTED ... )
|| _recoveryMode.
Collaborator
Author
|
@coderabbitai, I am about to merge this PR, please do an in-depth review of all the commits made in this PR, make a summary, a recommendation to merge and a list of possible future actions including a prompt for AI agents to look at the most urgent items before merge |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17104
📋 Full PR Review —
|
| # | SHA | Headline | Assessment |
|---|---|---|---|
| 1 | a888475 |
Ethernet vs WiFi fixes, filter fixes, Monitor fix | ✅ Solid foundation |
| 2 | 5294e85 |
Ethernet and AP | ✅ Correct guards |
| 3 | a85803d |
ETH.localIP()→ETH.connected(), FastLED local buffers | ✅ with follow-up items |
| 4 | 8d6f7a1 |
Status split from RSSI → telemetry.status | ✅ Clean refactor |
| 5 | b51cafc |
System hostname unification | ✅ Intentional design confirmed |
| 6 | c2a20e0 |
Disable WiFi code if no WiFi, network helpers | ✅ Reviewed separately |
Commit 1 — a888475: Filter fixes + Monitor fix
- RowRenderer filter routing:
onFilterChangecorrectly decoupled fromonChange;changeOnInput=falseon filter fields prevents premature live-update triggers. ✅ - Module.cpp
_filterskip:contains("_filter")is a pragmatic choice given ArduinoJson'sJsonStringhas noendsWith. Risk of false positives is low as long as property names avoid that substring by convention. ✅ (accepted) - Monitor clearVertices(): Clears stale WebGL vertex positions on scene rebuild — correct fix. ✅
Commit 2 — 5294e85: Ethernet and AP
- APSettingsService
ETH.connected(): The|| ETH.connected()addition toAP_MODE_DISCONNECTEDcorrectly suppresses recovery AP when Ethernet is the active link. ✅ psramFound()guard in EthernetSettingsService: Prevents disabling WiFi on PSRAM boards (e.g. ESP32-P4 with ESP-Hosted coprocessor). Correct and necessary. ✅
Commit 3 — a85803d: FastLED local buffer + ETH.localIP() cleanup
ETH.localIP()→ETH.connected(): More semantically correct — IP validity is not the right connectivity test, link state is. ✅FixedPointCanvasDemoEffect/ColorTrailsEffectlocal CRGB buffers: ThecanvasBufOwnedownership model correctly handles the two cases (aliasingchannelsEvs owning a separate allocation). Destructors clean up owned buffers. ✅fastled_fadeToBlackByfree function: Clean non-member API for raw CRGB arrays. ✅- Log message
!oneToOne mapping !: Minor improvement. ✅
Commit 4 — 8d6f7a1: Telemetry status split
SystemStatustype +setStatus(): Correctly separates device status (safeMode, restartNeeded, saveNeeded, hostName) from RSSI.hostNameis persisted to localStorage so it survives page reload. ✅socket.on('status'): New dedicated socket event is cleaner than piggybacking on RSSI. ✅+layout.svelteclose handling: Retains$telemetry.status.hostNameon disconnect rather than resetting to'MoonLight'— good UX, last known hostname survives. ✅- statusbar.svelte: Reads from
$telemetry.status.*correctly; WiFiOff icon removal simplifies the disconnect indicator. ✅
Commit 5 — b51cafc: System hostname
getSystemHostname(): WiFi-first, Ethernet fallback, MAC-derived last-resort. Intentional and correct per the design discussion. ✅ (learning saved)WiFiSettingsService::loadState(): Enables early hostname availability before WiFi is fully started. ✅EthernetSettingsService::systemHostnameProvider: Clean functional injection pattern. ✅ModuleDevices.h+ModuleLightsControl.h: All hostname references unified togetSystemHostname(). ✅
Commit 6 — c2a20e0: Network helpers + FT_WIFI guards
- Already reviewed in detail — all ✅. See prior comment.
CI Status ✅
All 38 checks passing across every platform target, including the critical esp32-p4-eth and esp32-p4-nano builds that were the primary motivation for much of this work.
| Check | Result |
|---|---|
| Frontend ESLint + Prettier | ✅ |
| IWYU | ✅ |
| Backend cppcheck + cpp_lint | ✅ |
| Backend Unit Tests (doctest) | ✅ |
| All PlatformIO builds (15+ targets) | ✅ |
✅ Recommendation: Merge
This PR is well-scoped, all design decisions have been explained and are intentional, all review threads are resolved, and every CI check passes including the previously broken esp32-p4-eth target. The code improvements (hostname unification, network helpers, FT_WIFI gating, telemetry status split) are a clear net positive for the codebase quality and ESP32-P4 compatibility.
🔮 Post-Merge Follow-Up Actions
The items below are not blockers but should be tracked for future PRs:
🔴 Urgent (before next ethernet-facing PR)
-
E_FastLED.h— buffer invalidation on mapping change (flagged as 🟠 Major in review, thread resolved but unclear if code was updated):needsLocalBuf()/onSizeChanged()is only triggered when buffer is null. IfoneToOneMappingorchannelsPerLightchanges at runtime while a buffer already exists, it may not be re-evaluated. Should add anonSizeChanged/onMappingChangedhook that clearscanvasBufwhen ownership mode needs to flip. -
lps_allinSystemStatus.cpp—esp32sveltekit.lps_allis the live incrementing counter. Verify the SystemStatus JSON emission path is reading the latched/computed LPS value (set inESP32SvelteKit::loop) and not the raw counter mid-cycle. -
SPI pin GPIO validity check regression (from learning ID
1fdaf05a):readPins()assignspin_SPI_SCK,pin_SPI_MISO,pin_SPI_MOSI,pin_PHY_CS,pin_PHY_IRQwithoutGPIO_IS_VALID_GPIO()validation. Invalid pin values from a corrupt board preset could crashETH.begin(). Restore a validity guard for SPI/W5500 pins.
🟡 Lower Priority
-
QuinLED DIG-OCTA (board_QuinLEDDigOctaV2): Board preset still missing
ETH_CLOCK_GPIO17_OUTbuild flag and RMII pins are not markedpin_Ethernet. Not broken by this PR but remains an open item. -
_filternaming convention: Document (comment or wiki) that property names must not contain the substring"_filter"unless they are filter-only UI properties, sinceModule::processUpdatedItemusescontains("_filter")to skiponUpdate. -
APSettingsService— noFT_WIFIguard onWiFi.getMode()/WiFi.status(): Safe today becauseAPSettingsServiceis only instantiated inside#if FT_ENABLED(FT_WIFI). IfAPSettingsServiceever moves outside that guard, these bare WiFi calls will fail to compile onFT_WIFI=0builds. Consider adding guards proactively.
🤖 AI Agent Prompt — Urgent Pre/Post-Merge Checks
Context: MoonModules/MoonLight, just merged PR
#152("Ethernet tweaks"). The repo uses ESP-IDF / Arduino ESP32 withFT_WIFI,FT_ETHERNET,FT_MQTTfeature flags. Key files:src/MoonLight/Nodes/Effects/E_FastLED.h,lib/framework/ESP32SvelteKit.h,lib/framework/SystemStatus.cpp,src/MoonBase/Modules/ModuleIO.h.Task 1 —
E_FastLED.hbuffer lifetime: InFixedPointCanvasDemoEffectandColorTrailsEffect, check whetherneedsLocalBuf()is re-evaluated after the initial null-check. Specifically: ifoneToOneMappingorchannelsPerLightchanges at runtime whilecanvasBufis already allocated, doesonSizeChanged()correctly free and reallocate with the new ownership mode? If not, propose a fix that clearscanvasBuf(freeing if owned) whenever the mode decision would change.Task 2 —
lps_allread inSystemStatus.cpp: Trace the value ofesp32sveltekit.lps_allfrom where it is incremented, to where it is reset/latched inESP32SvelteKit::loop(), to whereSystemStatus::createJSON()reads it. Confirm thatSystemStatusalways reads the latched snapshot (post-reset) rather than a mid-cycle counter. If it reads the live counter, propose moving the latch or snapshotting the value before reset.Task 3 — SPI pin validity in
ModuleIO.h::readPins(): Find the five SPI ethernet pin assignments (pin_SPI_SCK,pin_SPI_MISO,pin_SPI_MOSI,pin_PHY_CS,pin_PHY_IRQ) and check whether any GPIO validity check is performed before assignment. If not, add a guard usingGPIO_IS_VALID_GPIO()(orrtc_gpio_is_valid_gpio()for RTC-capable pins) matching the pattern used elsewhere in the file, to prevent invalid pins from reachingETH.begin().
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 152
File: lib/framework/ESP32SvelteKit.cpp:77-79
Timestamp: 2026-03-17T19:25:51.874Z
Learning: In MoonModules/MoonLight ESP32SvelteKit.cpp begin(): The double call to _fsPersistence.readFromFS() (once via _wifiSettingsService.loadState() early in begin(), and again inside _wifiSettingsService.initWiFi()) is intentional and harmless. readFromFS() is idempotent — same JSON file, same in-memory state. initWiFi() is kept self-contained so it works regardless of whether loadState() was called first. The FT_WIFI=0 concern is already handled by the existing `#if` FT_ENABLED(FT_WIFI) guard around initWiFi(). getSystemHostname() correctly reads from in-memory state via _wifiSettingsService.getHostname(), not from disk.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-26T14:59:22.091Z
Learning: In MoonModules/MoonLight PR `#114`: ModuleDevices deadlock caused by: (1) StatefulService.h line 204-205 bug - after 500ms mutex timeout, continues WITHOUT acquiring mutex, causing race conditions and JSON corruption. (2) receiveUDP() while-loop processes own UDP broadcasts via loopback, creating circular call chain: sendUDP→broadcast→receiveUDP→updateDevices→update→handlers→sendUDP. (3) updateDevices() holds _accessMutex during expensive sort/JSON operations with 15 devices, can exceed 500ms with nested calls. Solution: Fix StatefulService to wait forever or reboot on timeout; skip own broadcasts by checking message.name == ownHostname; optimize updateDevices to sort outside mutex.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-17T19:24:12.957Z
Learning: In MoonModules/MoonLight ESP32SvelteKit.h, getSystemHostname() intentionally uses WiFi hostname as the primary source (always non-empty due to FACTORY_WIFI_HOSTNAME defaulting to "#{platform}-#{unique_id}"). On WiFi+Ethernet boards both defaults resolve to the same template, so there is no shadowing. On ethernet-only builds (FT_WIFI=0) the WiFi block is compiled out and falls through to the Ethernet hostname. Hostname must NOT depend on connection state — flip-flopping would break mDNS/DHCP stability. WiFi settings page is the intended user-facing config; Ethernet hostname is a fallback only for boards without WiFi.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 151
File: lib/framework/EthernetSettingsService.cpp:137-143
Timestamp: 2026-03-16T22:55:56.751Z
Learning: In MoonModules/MoonLight EthernetSettingsService.cpp, when Ethernet drops and `_wifiDisabledByEthernet` is cleared in `updateEthernet()`, WiFi reconnection is intentionally NOT triggered immediately. The design relies on `WiFiSettingsService::manageSTA()` picking up the cleared flag on its next periodic loop iteration. The short `WIFI_RECONNECTION_DELAY` (a few seconds) is considered acceptable, and adding a direct cross-service reference to WiFiSettingsService from EthernetSettingsService was explicitly rejected as over-engineering.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-08T10:36:33.812Z
Learning: In MoonModules/MoonLight D_FastLEDDriver: FastLED RMT5 (default on IDF >= 5) lacks the dual-buffer WiFi-resistance design of FastLED RMT4, causing WebSocket/WiFi throughput degradation at high LED frame rates. FastLED I2S is also incompatible with WiFi on ESP32-S3. MoonLight's D_ParallelLEDDriver (parlio.cpp) avoids this entirely by using its own DMA-streaming PARLIO implementation with zero per-frame ISR callbacks. Fix: add `-D FASTLED_RMT5=0` to platformio.ini for IDF5-based boards to force RMT4's dual-buffer design. This flag may need to be re-enabled even when a specific FastLED commit appeared stable, as RMT5 behavior varies across FastLED master commits.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService: The double initEthernet() call on boot (once at startup, once when the board preset fires readPins()) is verified as working correctly. It is a benign redundant call and adding complexity to avoid it is not warranted.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService + WiFiSettingsService: Simultaneous Ethernet + WiFi drop scenario is verified correct. When both drop: _wifiDisabledByEthernet is cleared in updateEthernet(), ETH.connected() returns false in manageSTA(), so manageSTA() proceeds normally to reconnect WiFi.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService.cpp configureNetwork(): The `else { ETH.begin(); }` fallback after the SPI and RMII branches is intentional. It acts as a safe default for future boards with native/built-in EMAC ethernet that works with default arguments. Currently all configured boards go through the explicit SPI or RMII paths, so this fallback is never reached in practice.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-12T18:00:40.667Z
Learning: In MoonModules/MoonLight, the QuinLED DIG-OCTA BrainBoard 32-8L (board_QuinLEDDigOctaV2) uses an original ESP32 (not S3) with an onboard LAN8720A RMII PHY. It does NOT use W5500 or SPI Ethernet. The RMII Ethernet pins are hardware-fixed: REF_CLK=GPIO17, MDIO=GPIO18, TXD0=GPIO19, TX_EN=GPIO21, TXD1=GPIO22, MDC=GPIO23, RXD0=GPIO25, RXD1=GPIO26, CRS_DV=GPIO27. GPIO17 is a clock OUTPUT from ESP32 to LAN8720A, requiring ETH_CLOCK_GPIO17_OUT build flag (the default ETH_CLOCK_GPIO0_IN is wrong for this board). ETH power is controlled by a physical jumper, not a GPIO. The board preset currently does not mark these RMII pins as pin_Ethernet, and no ETH_CLK_MODE/ETH_PHY build flags are set in firmware/esp32-d0.ini.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T14:28:21.568Z
Learning: In MoonModules/MoonLight ModuleIO.h readPins(): The assignIfValid lambda (which called GPIO_IS_VALID_GPIO() before assigning SPI ethernet pin variables) was removed in the ethernet rework commit 59f0149. SPI pin assignments (pin_SPI_SCK, pin_SPI_MISO, pin_SPI_MOSI, pin_PHY_CS, pin_PHY_IRQ) are now assigned directly without GPIO validity checking. For RMII pins this is fine (hardware-fixed), but for user-configurable W5500 SPI pins this is a regression.
Collaborator
Author
|
@coderabbitai, make a prompt for AI agents to update documentation (/docs) based on all changes in this PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 160541
🤖 AI Agent Prompt — Update
|
| Property | Type | Description |
|---|---|---|
$telemetry.rssi.rssi |
Number |
WiFi RSSI signal strength in dBm |
$telemetry.rssi.ssid |
String |
Name of the connected WiFi station |
$telemetry.rssi.disconnected |
Boolean |
true when WiFi is not connected |
$telemetry.status.hostName |
String |
System hostname (persisted in localStorage, survives page reload) |
$telemetry.status.safeMode |
Boolean |
Device is in Safe Mode |
$telemetry.status.restartNeeded |
Boolean |
A restart is required |
$telemetry.status.saveNeeded |
Boolean |
Unsaved changes exist |
Note that hostName, safeMode, restartNeeded, and saveNeeded were moved out of rssi into the new status container in this release. Update any code referencing $telemetry.rssi.hostName (etc.) to use $telemetry.status.* instead.
The status data is delivered via a new dedicated 'status' WebSocket event (emitted every second when socket clients are connected), separate from the 'rssi' event. The hostName in status is persisted in localStorage so the last known device name survives page reloads and disconnections.
6. docs/buildprocess.md
Update the Feature Flags table to add:
| Flag | Description |
|---|---|
FT_WIFI |
Controls whether the WiFi stack is compiled in. Disable on ethernet-only boards (e.g. esp32-p4-eth). When disabled, WiFi Station and Access Point menu items are hidden, and the Ethernet hostname is used as the system hostname. |
Add a note under the existing table:
Ethernet-only builds: Setting
FT_WIFI=0removes all WiFi code from the binary, which is appropriate for boards like theesp32-p4-eththat connect exclusively over Ethernet. The UI adapts automatically — WiFi Station and Access Point pages are hidden. The system hostname falls back to the Ethernet hostname or a MAC-derived default.
7. docs/moonbase/inputoutput.md
Verify the following changes are present (they were already made in this PR — just confirm and adjust if anything was missed):
eth_Noneoption is now calledBoard Default(renamed fromNone).Board Defaultdescription should state: uses compile-time Ethernet pin definitions frompins_arduino.h— no runtime pin assignment is needed; suitable for boards with fixed onboard Ethernet (e.g. QuinLED Dig-Octa with LAN8720 RMII,esp32-p4-eth). Has no effect if no Ethernet pins are defined for the target board.- The Ethernet Type table should list:
Board Default,W5500 (SPI),LAN8720 (RMII).
8. docs/moonbase/devices.md
Add a note under the Name section:
- The device name shown and used for group matching is the system hostname (configured in
Network → WiFi Stationon WiFi-capable boards, or derived from the Ethernet hostname on ethernet-only boards). - On boards without WiFi (
FT_WIFI=0), the Ethernet hostname is used for UDP device discovery and group membership.
9. docs/moonbase/overview.md
Update the Status Bar section:
hostNamein the status bar header now reflects the system hostname, which is the WiFi hostname on WiFi-capable boards and the Ethernet hostname (or MAC-derived fallback) on ethernet-only boards.- The Save button in the status bar is now accompanied by a Cancel button that restores the last saved state, when there are unsaved changes.
10. docs/develop/modules.md
Add a _filter property convention note:
When defining properties in setupDefinition, property names ending or containing "_filter" are treated as UI-only filter properties:
- They are persisted to the filesystem normally.
- They do not trigger
onUpdate()inModule::processUpdatedItem. - Use this convention for search/filter controls in row arrays that should not cause side effects when changed.
⚠️ Avoid using the substring"_filter"in regular property names, as it will suppress theironUpdatehandler.
Add an onFilterChange section:
RowRenderer.svelte now supports a separate onFilterChange callback prop (in addition to onChange). Module.svelte wires this to inputChanged with the property name suffixed by "_filter". This allows filter input fields to update a separate filter state key without triggering the main onChange handler. Filter inputs also have changeOnInput=false to avoid triggering on every keystroke.
11. docs/develop/nodes.md (or docs/moonlight/drivers.md)
Add to the VirtualLayer / Effects developer section:
A new free function fastled_fadeToBlackBy(CRGB* leds, uint16_t num_leds, uint8_t fadeBy) is available from VirtualLayer.h. It operates on a raw CRGB array directly, without requiring a VirtualLayer instance. Use this in effects that maintain their own local CRGB buffer (e.g. FixedPointCanvasDemoEffect, ColorTrailsEffect).
12. docs/develop/architecture.md
Add a note under the ESP32SvelteKit task section about the new status socket event:
The ESP32SvelteKit loop now emits a 'status' WebSocket event every second (when clients are connected), containing: safeMode, restartNeeded, saveNeeded, and hostName. This is separate from the 'rssi' event which only carries rssi, ssid, and disconnected.
Additional Instructions for the AI Agent
- Cross-link pages wherever relevant: e.g.
docs/network/ethernet.md↔docs/network/sta.md↔docs/moonbase/inputoutput.md↔docs/buildprocess.md. - Preserve all existing content that is still accurate. Do not remove working screenshots, tables, or links unless they are directly obsoleted by the PR changes.
- Follow the existing MkDocs style: use
!!! info,!!! tip,!!! warningadmonition blocks where appropriate; keep table formatting consistent with the rest of the file. - Do not modify
docs/network/mqtt.md,docs/network/ntp.md,docs/system/coredump.md,docs/system/update.md,docs/moonlight/effects.md,docs/moonlight/layouts.md,docs/moonlight/modifiers.md, or any unrelated docs — the PR did not change the functionality these cover. - Commit message suggestion:
docs: update for PREthernet WiFi and host-name #152ethernet tweaks — system hostname, FT_WIFI, telemetry.status, eth_BoardDefault, network helpers
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-17T19:24:12.957Z
Learning: In MoonModules/MoonLight ESP32SvelteKit.h, getSystemHostname() intentionally uses WiFi hostname as the primary source (always non-empty due to FACTORY_WIFI_HOSTNAME defaulting to "#{platform}-#{unique_id}"). On WiFi+Ethernet boards both defaults resolve to the same template, so there is no shadowing. On ethernet-only builds (FT_WIFI=0) the WiFi block is compiled out and falls through to the Ethernet hostname. Hostname must NOT depend on connection state — flip-flopping would break mDNS/DHCP stability. WiFi settings page is the intended user-facing config; Ethernet hostname is a fallback only for boards without WiFi.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService: The double initEthernet() call on boot (once at startup, once when the board preset fires readPins()) is verified as working correctly. It is a benign redundant call and adding complexity to avoid it is not warranted.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService + WiFiSettingsService: Simultaneous Ethernet + WiFi drop scenario is verified correct. When both drop: _wifiDisabledByEthernet is cleared in updateEthernet(), ETH.connected() returns false in manageSTA(), so manageSTA() proceeds normally to reconnect WiFi.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T23:12:59.624Z
Learning: In MoonModules/MoonLight EthernetSettingsService.cpp configureNetwork(): The `else { ETH.begin(); }` fallback after the SPI and RMII branches is intentional. It acts as a safe default for future boards with native/built-in EMAC ethernet that works with default arguments. Currently all configured boards go through the explicit SPI or RMII paths, so this fallback is never reached in practice.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 152
File: lib/framework/ESP32SvelteKit.cpp:77-79
Timestamp: 2026-03-17T19:25:51.874Z
Learning: In MoonModules/MoonLight ESP32SvelteKit.cpp begin(): The double call to _fsPersistence.readFromFS() (once via _wifiSettingsService.loadState() early in begin(), and again inside _wifiSettingsService.initWiFi()) is intentional and harmless. readFromFS() is idempotent — same JSON file, same in-memory state. initWiFi() is kept self-contained so it works regardless of whether loadState() was called first. The FT_WIFI=0 concern is already handled by the existing `#if` FT_ENABLED(FT_WIFI) guard around initWiFi(). getSystemHostname() correctly reads from in-memory state via _wifiSettingsService.getHostname(), not from disk.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 151
File: lib/framework/EthernetSettingsService.cpp:137-143
Timestamp: 2026-03-16T22:55:56.751Z
Learning: In MoonModules/MoonLight EthernetSettingsService.cpp, when Ethernet drops and `_wifiDisabledByEthernet` is cleared in `updateEthernet()`, WiFi reconnection is intentionally NOT triggered immediately. The design relies on `WiFiSettingsService::manageSTA()` picking up the cleared flag on its next periodic loop iteration. The short `WIFI_RECONNECTION_DELAY` (a few seconds) is considered acceptable, and adding a direct cross-service reference to WiFiSettingsService from EthernetSettingsService was explicitly rejected as over-engineering.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-05T07:10:15.113Z
Learning: In MoonModules/MoonLight (lib/PsychicHttp/src/PsychicWebSocket.cpp): `PsychicWebSocketClient::sendMessage()` must guard with `httpd_ws_get_fd_info()` before calling `httpd_ws_send_frame_async()`. Without the guard, when a client disconnects and reconnects (reusing the same socket fd) while a browser tab is hidden, repeated failed sends degrade the lwIP netconn state until `lwip_netconn_do_write` hard-asserts `(state!)` instead of returning ESP_FAIL, causing a reboot. Fix: check `httpd_ws_get_fd_info(server, socket) == HTTPD_WS_CLIENT_WEBSOCKET` first and return ESP_FAIL early if not. Also, in EventSocket.cpp, erase stale clients from subscriptions on ESP_FAIL instead of keeping them.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-08T10:36:33.812Z
Learning: In MoonModules/MoonLight D_FastLEDDriver: FastLED RMT5 (default on IDF >= 5) lacks the dual-buffer WiFi-resistance design of FastLED RMT4, causing WebSocket/WiFi throughput degradation at high LED frame rates. FastLED I2S is also incompatible with WiFi on ESP32-S3. MoonLight's D_ParallelLEDDriver (parlio.cpp) avoids this entirely by using its own DMA-streaming PARLIO implementation with zero per-frame ISR callbacks. Fix: add `-D FASTLED_RMT5=0` to platformio.ini for IDF5-based boards to force RMT4's dual-buffer design. This flag may need to be re-enabled even when a specific FastLED commit appeared stable, as RMT5 behavior varies across FastLED master commits.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-26T14:59:22.091Z
Learning: In MoonModules/MoonLight PR `#114`: ModuleDevices deadlock caused by: (1) StatefulService.h line 204-205 bug - after 500ms mutex timeout, continues WITHOUT acquiring mutex, causing race conditions and JSON corruption. (2) receiveUDP() while-loop processes own UDP broadcasts via loopback, creating circular call chain: sendUDP→broadcast→receiveUDP→updateDevices→update→handlers→sendUDP. (3) updateDevices() holds _accessMutex during expensive sort/JSON operations with 15 devices, can exceed 500ms with nested calls. Solution: Fix StatefulService to wait forever or reboot on timeout; skip own broadcasts by checking message.name == ownHostname; optimize updateDevices to sort outside mutex.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-12T18:00:40.667Z
Learning: In MoonModules/MoonLight, the QuinLED DIG-OCTA BrainBoard 32-8L (board_QuinLEDDigOctaV2) uses an original ESP32 (not S3) with an onboard LAN8720A RMII PHY. It does NOT use W5500 or SPI Ethernet. The RMII Ethernet pins are hardware-fixed: REF_CLK=GPIO17, MDIO=GPIO18, TXD0=GPIO19, TX_EN=GPIO21, TXD1=GPIO22, MDC=GPIO23, RXD0=GPIO25, RXD1=GPIO26, CRS_DV=GPIO27. GPIO17 is a clock OUTPUT from ESP32 to LAN8720A, requiring ETH_CLOCK_GPIO17_OUT build flag (the default ETH_CLOCK_GPIO0_IN is wrong for this board). ETH power is controlled by a physical jumper, not a GPIO. The board preset currently does not mark these RMII pins as pin_Ethernet, and no ETH_CLK_MODE/ETH_PHY build flags are set in firmware/esp32-d0.ini.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-28T18:06:23.868Z
Learning: ESP32-P4 observed lower headroom for httpd task stack in MoonLight: NodeManager rename (create new node + rebuild controls + remove old) can overflow httpd stack when onUpdate runs in httpd. Mitigation: defer only NodeManager rename heavy work to SvelteKit loop via a small pendingRenames queue; keep identity short-circuit in compareRecursive and equality-guarded controls updates. Increasing httpd stack_size for P4 builds is an optional, broader fix.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-03-16T14:28:21.568Z
Learning: In MoonModules/MoonLight ModuleIO.h readPins(): The assignIfValid lambda (which called GPIO_IS_VALID_GPIO() before assigning SPI ethernet pin variables) was removed in the ethernet rework commit 59f0149. SPI pin assignments (pin_SPI_SCK, pin_SPI_MISO, pin_SPI_MOSI, pin_PHY_CS, pin_PHY_IRQ) are now assigned directly without GPIO validity checking. For RMII pins this is fine (hardware-fixed), but for user-configurable W5500 SPI pins this is a regression.
Collaborator
Author
|
@coderabbitai, regarding
this is a lot!!! Please note the documentation is end user documentation so end users don't have to know anything about the internals. So please do it again to guide AI agents to make compact and user friendly updates only describing usage implications of the changes made |
|
Tip For best results, initiate chat on the files or code changes.
🤖 AI Agent Prompt — User-Facing
|
Docs ==== - update with changes from this PR - SvelteKit, SystemStatus: add lps_all_snapshot - module IO: pin_SPI: check GPIO_IS_VALID_GPIO
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/framework/ESP32SvelteKit.h (1)
261-263: Clarify the hostname comment to avoid “active interface” ambiguity.The implementation is intentionally stable-priority (WiFi → Ethernet), not connection-state based; rewording this comment would prevent future regressions from misinterpretation.
Based on learnings: “Hostname must NOT depend on connection state — flip-flopping would break mDNS/DHCP stability.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/framework/ESP32SvelteKit.h` around lines 261 - 263, Reword the comment above getSystemHostname() to explicitly state that hostname selection is based on a fixed priority order (WiFi hostname first, then Ethernet hostname, then "ML"+last4 MAC, then "MoonLight") and does NOT depend on real-time connection state, to avoid ambiguity and prevent flip-flopping; update the text to mention stability for mDNS/DHCP and that the function returns a deterministic hostname by priority rather than the currently active interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/develop/standardsguidelines.md`:
- Line 47: Replace the unhyphenated compound adjectives in the documentation
line: change both occurrences of "end user documentation" to "end-user
documentation" and change "user friendly updates" to "user-friendly updates" in
docs/develop/standardsguidelines.md (search for those exact phrases and update
them wherever they appear to maintain grammatical consistency).
In `@docs/network/ethernet.md`:
- Line 7: The doc sentence states Wi‑Fi and AP are always suspended when
Ethernet is connected, but runtime actually checks psramFound() before
suspending STA/AP and allows AP in disconnected provision mode; update the text
to reflect the conditional behavior: mention that suspension of Wi‑Fi Station
and Access Point occurs only when !psramFound(), that AP may still be
started/kept as a fallback in disconnected provision mode, and note that STA
reconnection after Ethernet loss is handled on the next
WiFiSettingsService::manageSTA() cycle rather than immediately.
In `@src/MoonBase/Modules/ModuleIO.h`:
- Around line 817-822: The SPI output pins are currently validated with
GPIO_IS_VALID_GPIO which allows input-only pins; update the validators so
output-capable pins are enforced: replace the checks for usage == pin_SPI_SCK,
usage == pin_SPI_MOSI, and usage == pin_PHY_CS to use GPIO_IS_VALID_OUTPUT_GPIO
before assigning to ess->v_ETH_SPI_SCK, ess->v_ETH_SPI_MOSI, and
ess->v_ETH_PHY_CS respectively, while leaving pin_SPI_MISO and pin_PHY_IRQ
checks as GPIO_IS_VALID_GPIO.
---
Nitpick comments:
In `@lib/framework/ESP32SvelteKit.h`:
- Around line 261-263: Reword the comment above getSystemHostname() to
explicitly state that hostname selection is based on a fixed priority order
(WiFi hostname first, then Ethernet hostname, then "ML"+last4 MAC, then
"MoonLight") and does NOT depend on real-time connection state, to avoid
ambiguity and prevent flip-flopping; update the text to mention stability for
mDNS/DHCP and that the function returns a deterministic hostname by priority
rather than the currently active interface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f6f60ff-07c3-4cbb-bfe8-67012697924d
📒 Files selected for processing (11)
docs/develop/standardsguidelines.mddocs/moonbase/overview.mddocs/network/ethernet.mddocs/network/sta.mddocs/system/status.mdlib/framework/ESP32SvelteKit.cpplib/framework/ESP32SvelteKit.hlib/framework/SystemStatus.cppplatformio.inisrc/MoonBase/Modules/ModuleIO.hsrc/MoonLight/Modules/ModuleEffects.h
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/framework/SystemStatus.cpp
- src/MoonLight/Modules/ModuleEffects.h
docs/develop/standardsguidelines.md
Outdated
| Before merging a PR back into main ask the 🐰 the following: | ||
|
|
||
| @coderabbitai, I am about to merge this PR, please do an in-depth review of all the commits made in this PR, make a summary, a recommendation to merge and a list of possible future actions. | ||
| @coderabbitai, I am about to merge this PR, please do an in-depth review of all the commits made in this PR, make a summary, a recommendation to merge and a list of possible future actions including a prompt for AI agents to look at the most urgent items before merge. Also make a prompt for AI agents to update end user documentation (/docs) based on all changes in this PR. Please note the documentation is end user documentation so end users don't have to know anything about the internals so make compact and user friendly updates only describing usage implications of the changes made. |
There was a problem hiding this comment.
Fix compound adjective hyphenation.
The line contains compound adjectives that should be hyphenated per standard English grammar conventions:
- "end user documentation" → "end-user documentation" (appears twice)
- "user friendly updates" → "user-friendly updates"
These are flagged by LanguageTool and improve documentation clarity.
📝 Proposed grammar fix
-@coderabbitai, I am about to merge this PR, please do an in-depth review of all the commits made in this PR, make a summary, a recommendation to merge and a list of possible future actions including a prompt for AI agents to look at the most urgent items before merge. Also make a prompt for AI agents to update end user documentation (/docs) based on all changes in this PR. Please note the documentation is end user documentation so end users don't have to know anything about the internals so make compact and user friendly updates only describing usage implications of the changes made.
+@coderabbitai, I am about to merge this PR, please do an in-depth review of all the commits made in this PR, make a summary, a recommendation to merge and a list of possible future actions including a prompt for AI agents to look at the most urgent items before merge. Also make a prompt for AI agents to update end-user documentation (/docs) based on all changes in this PR. Please note the documentation is end-user documentation so end users don't have to know anything about the internals so make compact and user-friendly updates only describing usage implications of the changes made.📝 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
| @coderabbitai, I am about to merge this PR, please do an in-depth review of all the commits made in this PR, make a summary, a recommendation to merge and a list of possible future actions including a prompt for AI agents to look at the most urgent items before merge. Also make a prompt for AI agents to update end user documentation (/docs) based on all changes in this PR. Please note the documentation is end user documentation so end users don't have to know anything about the internals so make compact and user friendly updates only describing usage implications of the changes made. | |
| `@coderabbitai`, I am about to merge this PR, please do an in-depth review of all the commits made in this PR, make a summary, a recommendation to merge and a list of possible future actions including a prompt for AI agents to look at the most urgent items before merge. Also make a prompt for AI agents to update end-user documentation (/docs) based on all changes in this PR. Please note the documentation is end-user documentation so end users don't have to know anything about the internals so make compact and user-friendly updates only describing usage implications of the changes made. |
🧰 Tools
🪛 LanguageTool
[grammar] ~47-~47: Use a hyphen to join words.
Context: ...ake a prompt for AI agents to update end user documentation (/docs) based on all ...
(QB_NEW_EN_HYPHEN)
[grammar] ~47-~47: Use a hyphen to join words.
Context: ...PR. Please note the documentation is end user documentation so end users don't ha...
(QB_NEW_EN_HYPHEN)
[grammar] ~47-~47: Use a hyphen to join words.
Context: ...t the internals so make compact and user friendly updates only describing usage i...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/develop/standardsguidelines.md` at line 47, Replace the unhyphenated
compound adjectives in the documentation line: change both occurrences of "end
user documentation" to "end-user documentation" and change "user friendly
updates" to "user-friendly updates" in docs/develop/standardsguidelines.md
(search for those exact phrases and update them wherever they appear to maintain
grammatical consistency).
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.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
UI Updates
Documentation