Conversation
Build ArtifactsFirmware built from this pull request's code:
Notice
|
|
Runtime data also written to LittleFS if WebUI 'Device Reboot' is used. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a thread-safe RuntimeClass singleton (RuntimeData) that persists runtime metadata to LittleFS as JSON with minute scheduling, read/write APIs, and accessors; integrates writes into OTA and reboot flows; exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Main as Main (setup)
participant Scheduler as Scheduler
participant RD as RuntimeData
participant FS as LittleFS
participant API as Web API
participant UI as Webapp UI
Main->>RD: init(scheduler)
RD->>Scheduler: register periodic loop (1 min)
Main->>RD: read()
RD->>FS: read `/runtime.json`
FS-->>RD: data / error
RD-->>Main: read() result
Note over Scheduler,RD: Scheduled loop checks daily trigger window (00:05–00:10) and explicit _writeNow flag
User->>API: complete firmware upload
API->>RD: write(60)
RD->>FS: write `/runtime.json`
FS-->>RD: result
RD-->>API: write() result
User->>API: reboot command
API->>RD: write(60)
RD->>FS: write `/runtime.json`
UI->>API: GET /api/system/status
API->>RD: getWriteCountAndTimeString()
RD-->>API: "count / time" or "no time"
API-->>UI: JSON includes `runtime_savecount`
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
include/RuntimeData.h (1)
33-40: Document atomic flags individuallyBoth
_readOKand_writeOKshare the same comment (“true if the last read/write operation was successful”), which makes it unclear which flag refers to which operation. Please update the comments (or rename the members) so each atomic’s responsibility is explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
include/RuntimeData.h(1 hunks)src/RuntimeData.cpp(1 hunks)src/WebApi_firmware.cpp(2 hunks)src/WebApi_maintenance.cpp(2 hunks)src/WebApi_sysstatus.cpp(2 hunks)src/main.cpp(2 hunks)webapp/src/components/FirmwareInfo.vue(1 hunks)webapp/src/locales/de.json(1 hunks)webapp/src/locales/en.json(1 hunks)webapp/src/types/SystemStatus.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/WebApi_maintenance.cppsrc/WebApi_firmware.cppinclude/RuntimeData.hsrc/RuntimeData.cppsrc/WebApi_sysstatus.cppsrc/main.cpp
webapp/src/**/*.{js,jsx,ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
webapp/src/**/*.{js,jsx,ts,tsx,vue}: Webapp source must pass ESLint (yarn lint)
Webapp source must be Prettier-formatted (yarn prettier --check src/)
Files:
webapp/src/components/FirmwareInfo.vuewebapp/src/types/SystemStatus.ts
🧬 Code graph analysis (1)
include/RuntimeData.h (1)
src/RuntimeData.cpp (16)
init(37-44)init(37-37)read(116-150)read(116-116)write(60-110)write(60-60)getWriteCount(156-160)getWriteCount(156-156)getWriteEpochTime(166-170)getWriteEpochTime(166-166)getWriteCountAndTimeString(178-191)getWriteCountAndTimeString(178-178)loop(50-54)loop(50-50)getWriteTrigger(197-211)getWriteTrigger(197-197)
⏰ 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). (4)
- GitHub Check: Build Environments (generic_esp32s3_usb)
- GitHub Check: Build Environments (generic_esp32s3)
- GitHub Check: Build Environments (generic_esp32_4mb_no_ota)
- GitHub Check: Build Environments (generic_esp32_8mb)
🔇 Additional comments (7)
webapp/src/types/SystemStatus.ts (1)
34-34: LGTM!The new
runtime_savecount: stringfield is correctly positioned and follows the existing naming conventions. The string type is appropriate for the combined "count / time" format exposed by the backend.src/WebApi_sysstatus.cpp (1)
17-17: LGTM!The addition of
RuntimeData.hinclude and the use ofRuntimeData.getWriteCountAndTimeString()to populate theruntime_savecountfield is straightforward and correct. This is a read-only operation with no side effects, appropriate for a status endpoint.Also applies to: 81-81
webapp/src/locales/en.json (1)
285-285: LGTM!The localization entry for
RuntimeSaveCountis clear, descriptive, and follows the existing pattern. The label "Runtime data save count / time" effectively communicates the combined information displayed.webapp/src/components/FirmwareInfo.vue (1)
86-89: LGTM with a minor observation.The new table row for
RuntimeSaveCountis correctly implemented and follows the existing pattern. The positioning betweenConfigSaveCountandUptimeis logical.One minor observation: Unlike
cfgsavecount(line 84) which uses$n(systemStatus.cfgsavecount, 'decimal')for number formatting,runtime_savecountis displayed directly. This appears intentional sinceruntime_savecountis a string containing both count and time information (as indicated by the localization "Runtime data save count / time"). No action needed unless the backend format changes.webapp/src/locales/de.json (1)
285-285: LGTM!The German localization entry for
RuntimeSaveCountis properly translated and follows the existing pattern. The label "Laufzeitdatenspeicherungen Anzahl / Zeit" accurately mirrors the English version.src/main.cpp (1)
157-159: Verify non-blocking read and clarify initialization order.
- Confirm that RuntimeData.read() completes quickly or handles missing/corrupt files without blocking startup.
- Add error handling or at least log the result of read() for visibility.
- Clarify the comment to explain why RuntimeData must be initialized last (e.g., it references other components or requires the scheduler to be fully configured).
src/WebApi_maintenance.cpp (1)
47-47: Handle write() return and assess blocking behavior
RuntimeData.write()returns a bool but isn’t checked here; wrap it in anifand log or act on failures.- LittleFS writes are synchronous, so verify that this call completes within acceptable time (<100 ms) or move it off the reboot path (e.g. perform the write before sending the response or offload it).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/RuntimeData.cpp (1)
197-207: Guard_lastTriggerwith the class mutex
getWriteTrigger()flips_lastTriggerwithout taking_mutex, so concurrent callers race the write, violating the class’s thread-safety guarantee. Please take_mutexbefore touching_lastTriggerso all state transitions stay atomic.bool RuntimeClass::getWriteTrigger(void) { struct tm actTime; if (getLocalTime(&actTime, 5)) { - if ((actTime.tm_hour == 0) && (actTime.tm_min >= 5) && (actTime.tm_min <= 10)) { - if (_lastTrigger == false) { - _lastTrigger = true; - return true; - } - } else { - _lastTrigger = false; - } + std::lock_guard<std::mutex> lock(_mutex); + if ((actTime.tm_hour == 0) && (actTime.tm_min >= 5) && (actTime.tm_min <= 10)) { + if (_lastTrigger == false) { + _lastTrigger = true; + return true; + } + } else { + _lastTrigger = false; + } } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/RuntimeData.h(1 hunks)src/RuntimeData.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/RuntimeData.h
🧰 Additional context used
📓 Path-based instructions (1)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/RuntimeData.cpp
⏰ 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). (4)
- GitHub Check: Build Environments (generic_esp32s3_usb)
- GitHub Check: Build Environments (generic_esp32_8mb)
- GitHub Check: Build Environments (generic_esp32s3)
- GitHub Check: Build Environments (generic_esp32_4mb_no_ota)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/RuntimeData.cpp (1)
206-219: Race condition:_lastTriggeraccessed without mutex protection.The
_lastTriggermember variable is read and modified (lines 210-211, 215) without mutex protection, creating a race condition. SincegetWriteTrigger()is called from the scheduled loop task and potentially from other contexts, concurrent access can corrupt the trigger state, leading to missed or duplicate writes.Apply this diff to add mutex protection:
bool RuntimeClass::getWriteTrigger(void) { + std::lock_guard<std::mutex> lock(_mutex); struct tm actTime; if (getLocalTime(&actTime, 5)) { if ((actTime.tm_hour == 0) && (actTime.tm_min >= 5) && (actTime.tm_min <= 10)) { if (_lastTrigger == false) { _lastTrigger = true; return true; } } else { _lastTrigger = false; } } return false; }
🧹 Nitpick comments (1)
src/RuntimeData.cpp (1)
192-193: Consider clarifying the time availability check.Line 192 calls
getLocalTime(&time, 5)but immediately overwritestimewithlocaltime_r(&_writeEpoch, &time)on line 193. ThegetLocalTimecall is only used for its boolean return to verify the time service is available, which makes the code somewhat confusing.Consider extracting a helper method to check time service availability, or add a comment explaining that
getLocalTimeis used only as an availability check:+ // Check if time service is available before converting epoch to local time if ((_writeEpoch != 0) && (getLocalTime(&time, 5))) { localtime_r(&_writeEpoch, &time);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RuntimeData.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/RuntimeData.cpp
⏰ 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). (4)
- GitHub Check: Build Environments (generic_esp32s3_usb)
- GitHub Check: Build Environments (generic_esp32s3)
- GitHub Check: Build Environments (generic_esp32_4mb_no_ota)
- GitHub Check: Build Environments (generic_esp32_8mb)
🔇 Additional comments (1)
src/RuntimeData.cpp (1)
77-116: LGTM! Write state updates deferred until success.The implementation correctly addresses the previous review feedback. The write counter and epoch are now only committed after a successful file write (lines 111-116), preventing the rate limiter from being incorrectly updated when writes fail.
These contradict. The first is reasonable. The second should be something like "runtime data is also saved periodically, every hour". Is this still a shell? I don't see what data is actually saved. |
this depends on the actual data, doesn't it? i don't see why data like "battery last fully charged" or "calibration factors" should be written on an hourly basis. as they will change rarely. IMO, for such slow changing data, a single write a day and planned writes on reboot or update should be enough. and also there i think it would be smart to only write, when data was update/changed. i don't know if the filesystem handles this on its own to reduce unnecessary writecycles. |
|
Hello @schlimmchen and @spcqike
Yes, but I'm already testing the first function (saving the last SoC calibration time, 100% SoC).
I share @spcqike opinion. It depends on the information we want to save. And how critical it would be if we lose it. But, we also can do it more frequently or ... and that is the option I would prefer .. On demand. Would that be okay with you? 🤔 |
shouldn't the RuntimeClass::loop() handle this? Check, whether or not data was changed and save it accordingly? the loop() may run every minute or faster, but only write when a change is detected and when the last write was > 1h ago, or so. |
No, I think that is the wrong place.
This might be easy to decide with Boolean data. But when do we store changing values like the battery's internal resistance? However, so far we don't have any important data that needs to be saved to the runtime file so often. Normally during WebApp 'OTA firmware update' and 'Reset' is enough. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/RuntimeData.cpp (2)
197-198: Minor optimization: redundant getLocalTime call.Line 197 calls
getLocalTime(&time, 5)solely to check if the time service is available, but the resultingtimestruct is immediately overwritten bylocaltime_r(&_writeEpoch, &time)on line 198. While not incorrect, this is slightly wasteful.Consider checking time service availability without populating a struct that will be discarded:
- if ((_writeEpoch != 0) && (getLocalTime(&time, 5))) { + struct tm tempCheck; + if ((_writeEpoch != 0) && (getLocalTime(&tempCheck, 5))) { localtime_r(&_writeEpoch, &time);Or simply rely on
_writeEpoch != 0if time service availability is already implied by a non-zero epoch.
212-225: Consider protecting_lastTriggerwith mutex for design robustness.The comment on line 210 acknowledges that
_lastTriggeris accessed without mutex protection, relying on the assumption thatgetWriteTrigger()is only called from the single-threadedloop(). While this may be safe in the current implementation, it creates a fragile design:
- Member variables with underscore prefix conventionally require synchronization in multi-threaded contexts.
- If future code changes call
getWriteTrigger()from another thread (e.g., for debugging or manual trigger), a data race will occur.- The edge-detection logic (lines 216-221) reads and writes
_lastTriggerwithout atomic guarantees.Consider protecting
_lastTriggerwith_mutexto make the design more robust:bool RuntimeClass::getWriteTrigger(void) { struct tm actTime; if (getLocalTime(&actTime, 5)) { if ((actTime.tm_hour == 0) && (actTime.tm_min >= 5) && (actTime.tm_min <= 10)) { + std::lock_guard<std::mutex> lock(_mutex); if (_lastTrigger == false) { _lastTrigger = true; return true; } } else { + std::lock_guard<std::mutex> lock(_mutex); _lastTrigger = false; } } return false; }This adds minimal overhead (mutex is uncontended in single-threaded use) while preventing potential future data races.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/RuntimeData.h(1 hunks)src/RuntimeData.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- include/RuntimeData.h
🧰 Additional context used
📓 Path-based instructions (1)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/RuntimeData.cpp
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/RuntimeData.cpp (2)
63-122: Excellent write implementation with proper state management.The write method correctly addresses the previous review concern by staging the next state values and only committing them after successful file I/O. Rate limiting, mutex usage, and error handling are all properly implemented.
Consider making the rate-limit error message more specific:
- return cleanExit(false, "Time interval between 2 write operations too short, skipping write"); + return cleanExit(false, "Time interval between 2 write operations too short (requires %u minutes), skipping write", freezeMinutes);
190-206: Consider clarifying the time-checking logic.The intent of checking
getLocalTime()before converting_writeEpochis to ensure the time system is initialized. However, the code pattern of populatingtimeand then immediately overwriting it is confusing.Consider this clearer implementation:
String RuntimeClass::getWriteCountAndTimeString(void) const { std::lock_guard<std::mutex> lock(_mutex); char buf[32] = ""; struct tm time; + struct tm timeCheck; - if ((_writeEpoch != 0) && (getLocalTime(&time, 5))) { + if ((_writeEpoch != 0) && (getLocalTime(&timeCheck, 5))) { localtime_r(&_writeEpoch, &time); strftime(buf, sizeof(buf), " / %d-%h %R", &time); } else { snprintf(buf, sizeof(buf), " / no time"); } String ctString = String(_writeCount) + String(buf); return ctString; }This makes it explicit that
getLocalTime()is only used to verify time system availability, whiletimeis independently populated from_writeEpoch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RuntimeData.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/RuntimeData.cpp
🧬 Code graph analysis (1)
src/RuntimeData.cpp (2)
src/WebApi_maintenance.cpp (2)
init(13-18)init(13-13)src/WebApi_firmware.cpp (2)
init(15-29)init(15-15)
🔇 Additional comments (5)
src/RuntimeData.cpp (5)
1-33: LGTM! Clear documentation and proper setup.The file header provides comprehensive documentation of the RuntimeData functionality. The TAG constant is appropriately set to "runtimedata", constants are well-defined, and the singleton pattern is correctly implemented.
39-46: LGTM! Proper scheduler task initialization.The init method correctly sets up the periodic loop task with appropriate interval and callback binding.
52-56: LGTM! Clean loop implementation.The loop method correctly checks the daily trigger condition and invokes write with a 60-minute debounce interval.
128-162: LGTM! Robust read implementation with proper error handling.The read method gracefully handles missing files and deserialization errors, applies appropriate defaults, and correctly uses mutex protection for state updates.
212-226: LGTM! Trigger logic correctly uses instance state.The getWriteTrigger method properly uses
_lastTriggeras a class member (addressing the previous review concern about static state) and correctly implements debounce logic for the daily trigger window with appropriate mutex protection.
|
Hallo @AndreasBoehm , 
|
|
@SW-Niko Das geht leider nicht, da deine RuntimeData branch nicht im hoylabs repo sondern in deinem Fork liegt. Du kannst |
|
@AndreasBoehm Danke für die Info. Das erklärt auch warum der Battery Gard in der Liste angeboten wird. 👍 |
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 (10)
include/RuntimeData.h(1 hunks)src/RuntimeData.cpp(1 hunks)src/WebApi_firmware.cpp(2 hunks)src/WebApi_maintenance.cpp(2 hunks)src/WebApi_sysstatus.cpp(2 hunks)src/main.cpp(2 hunks)webapp/src/components/FirmwareInfo.vue(1 hunks)webapp/src/locales/de.json(1 hunks)webapp/src/locales/en.json(1 hunks)webapp/src/types/SystemStatus.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- webapp/src/components/FirmwareInfo.vue
- src/WebApi_sysstatus.cpp
- webapp/src/locales/en.json
- webapp/src/types/SystemStatus.ts
🧰 Additional context used
📓 Path-based instructions (1)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/WebApi_maintenance.cppsrc/WebApi_firmware.cppinclude/RuntimeData.hsrc/RuntimeData.cppsrc/main.cpp
🧬 Code graph analysis (2)
include/RuntimeData.h (3)
src/RuntimeData.cpp (16)
init(39-46)init(39-39)read(128-162)read(128-128)write(63-122)write(63-63)getWriteCount(168-172)getWriteCount(168-168)getWriteEpochTime(178-182)getWriteEpochTime(178-178)getWriteCountAndTimeString(190-206)getWriteCountAndTimeString(190-190)loop(52-56)loop(52-52)getWriteTrigger(212-226)getWriteTrigger(212-212)src/WebApi_firmware.cpp (2)
init(15-29)init(15-15)src/WebApi_maintenance.cpp (2)
init(13-18)init(13-13)
src/RuntimeData.cpp (2)
src/WebApi_firmware.cpp (2)
init(15-29)init(15-15)src/WebApi_maintenance.cpp (2)
init(13-18)init(13-13)
🔇 Additional comments (17)
webapp/src/locales/de.json (1)
285-285: LGTM!The German translation for
RuntimeSaveCountis accurate and follows the existing localization pattern in the file.src/WebApi_firmware.cpp (2)
13-13: LGTM!The include is properly added for RuntimeData usage.
51-53: LGTM!The RuntimeData.write(60) call is correctly placed after the HTTP response is sent but before the restart is triggered. This ensures runtime data is persisted during OTA updates without blocking the upload handler. The return value can be safely ignored here since the device will restart immediately.
src/main.cpp (1)
39-39: LGTM!The include is properly added for RuntimeData usage.
src/WebApi_maintenance.cpp (2)
11-11: LGTM!The include is properly added for RuntimeData usage.
47-49: LGTM!The RuntimeData.write(60) call follows the same pattern as in
src/WebApi_firmware.cpp, correctly placed after the HTTP response is sent but before the restart is triggered. This ensures runtime data is persisted during device reboots without blocking the request handler.src/RuntimeData.cpp (7)
1-31: LGTM!The file header provides clear documentation of the module's purpose, behavior, and constraints. The constants and TAG are well-defined.
39-46: LGTM!The
init()method correctly sets up the scheduler task with appropriate configuration:
- Runs every minute
- Properly binds the loop callback
- Infinite iterations (TASK_FOREVER)
52-56: LGTM!The
loop()method implements the daily trigger mechanism cleanly by checking for the trigger condition and invoking write with the appropriate throttle parameter.
63-122: LGTM!The
write()method is well-implemented with proper error handling and thread safety:
- Correctly validates local time availability before writing
- Implements throttling to prevent excessive writes
- Uses staging variables (nextCount, nextEpoch) and only commits them to class state after successful write
- Proper mutex protection for shared state
- Clear error messages via cleanExit lambda
- The past review concern about deferring updates until write succeeds has been properly addressed (lines 114-119)
128-162: LGTM!The
read()method is well-implemented with proper defaults and thread safety:
- Gracefully handles file not found or deserialization errors by using default values
- Proper mutex protection when updating shared state
- Uses
Utils::skipBom()for BOM handling- Returns success status while always ensuring valid defaults
- Clear logging of outcomes
168-206: LGTM!The accessor methods (
getWriteCount(),getWriteEpochTime(),getWriteCountAndTimeString()) are properly implemented with:
- Consistent mutex protection for thread-safe access
getWriteCountAndTimeString()correctly handles cases where time is not yet available- Appropriate time formatting for UI display
212-226: LGTM!The
getWriteTrigger()method correctly implements the daily trigger window (00:05-00:10) with proper edge detection using_lastTriggerto prevent multiple triggers on the same day. The past review concern about moving this from a static local to a class member has been properly addressed.include/RuntimeData.h (4)
1-8: LGTM!The includes are appropriate for the class design:
- ArduinoJson for serialization
- TaskSchedulerDeclarations for task management
- mutex and atomic for thread safety
10-27: LGTM!The public interface is well-designed:
- Explicit constructor prevents implicit conversions
- Copy and move operations properly deleted for singleton pattern
init()for scheduler setupread()andwrite()for persistence with throttling support- Clean accessor methods for querying state
getWriteState()andgetReadState()provide operation status feedbackgetWriteCountAndTimeString()provides formatted data for UI display
29-41: LGTM!The private implementation details are well-structured:
- Task for periodic execution
- Atomics for operation status (thread-safe without mutex overhead for simple flags)
- Mutable mutex for protecting shared data in const methods
- State tracking for trigger detection and versioning
- Clear separation between public API and internal implementation
43-43: LGTM!The extern declaration properly exposes the global singleton instance that is defined in
src/RuntimeData.cpp.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/RuntimeData.cpp (1)
69-129: LGTM! Write method correctly implements transactional semantics.The implementation properly addresses the past critical issue: values are staged locally (line 95), I/O is performed, and state is committed only after a successful write (lines 122-124). The throttle logic correctly allows bypassing when
freezeMinutes=0. The cleanExit lambda provides consistent logging and state management.Minor clarity suggestion: The comment on line 103 says "protect the shared data with a local mutex" but the mutex is already held at this point. Consider rewording to "ensure any additional shared data added here remains under the existing mutex protection."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/RuntimeData.h(1 hunks)src/RuntimeData.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/RuntimeData.cppinclude/RuntimeData.h
🧠 Learnings (1)
📚 Learning: 2025-10-17T08:29:40.184Z
Learnt from: SW-Niko
PR: hoylabs/OpenDTU-OnBattery#2262
File: src/main.cpp:149-159
Timestamp: 2025-10-17T08:29:40.184Z
Learning: In the OpenDTU-OnBattery project (src/main.cpp), RuntimeData must be initialized after SolarCharger, PowerMeter, PowerLimiter, GridCharger, and Battery components have been initialized, but before the main loop. RuntimeData acts as a persistence service that these components access during their runtime operations (not during initialization).
Applied to files:
src/RuntimeData.cpp
🧬 Code graph analysis (2)
src/RuntimeData.cpp (2)
src/WebApi_firmware.cpp (2)
init(15-29)init(15-15)src/WebApi_maintenance.cpp (2)
init(13-18)init(13-13)
include/RuntimeData.h (1)
src/RuntimeData.cpp (16)
init(41-48)init(41-41)read(135-169)read(135-135)write(69-129)write(69-69)getWriteCount(175-179)getWriteCount(175-175)getWriteEpochTime(185-189)getWriteEpochTime(185-185)getWriteCountAndTimeString(197-213)getWriteCountAndTimeString(197-197)loop(54-62)loop(54-54)getWriteTrigger(219-236)getWriteTrigger(219-219)
🪛 Clang (14.0.6)
src/RuntimeData.cpp
[error] 20-20: 'Utils.h' file not found
(clang-diagnostic-error)
include/RuntimeData.h
[error] 4-4: 'ArduinoJson.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (8)
include/RuntimeData.h (2)
10-43: LGTM! Thread-safe singleton design is solid.The class correctly implements a non-copyable/non-movable singleton pattern. The synchronization strategy properly separates atomic flags (
_readOK,_writeOK,_writeNow) from mutex-protected shared state (_writeVersion,_writeCount,_writeEpoch,_lastTrigger). The mutable mutex appropriately enables const methods to acquire locks.
19-28: LGTM! Public interface is clean and well-designed.The API provides clear methods for initialization, persistence operations, and state queries. The default 10-minute throttle in
write()and the inlinerequestWriteOnNextTaskLoop()setter are sensible design choices. Const correctness is properly maintained throughout.src/RuntimeData.cpp (6)
1-35: LGTM! Clean module setup with clear documentation.The file header provides excellent guidance for future maintainers, including deadlock avoidance advice. The TAG is now correctly set to "runtimedata" (addressing past feedback), and the constants and singleton initialization are properly defined.
41-62: LGTM! Task initialization and loop logic are correct.The minute-based task is properly configured, and the loop logic correctly triggers writes either on explicit request (
_writeNow) or during the daily maintenance window. Callingwrite(0)bypasses the throttle for these scheduled events, which is the intended behavior.
135-169: LGTM! Read method properly handles missing or corrupt data.The implementation gracefully falls back to defaults when the file is missing or JSON deserialization fails. Mutex protection correctly guards shared state updates (lines 152-156), and the default operator
|is the idiomatic way to provide fallback values with ArduinoJson.(Same optional comment clarity note as in the write method applies to line 158.)
175-189: LGTM! Accessors properly protect shared state.Both getters correctly acquire the mutex before returning the protected values.
197-213: LGTM! Time formatting handles unavailable time server gracefully.The method correctly verifies time server availability (line 205) before converting the epoch to local time (line 206). The fallback to "no time" is appropriate for early boot scenarios.
219-236: LGTM! Daily trigger window is correctly implemented.The method properly detects the 00:05–00:10 window and uses
_lastTriggerfor edge detection to prevent multiple fires. This addresses the past review concern about the static flag—_lastTriggeris now a proper instance member protected by the mutex.
|
Hello @schlimmchen or @AndreasBoehm ,
But I need this PR and #2298 first to avoid triple nested PRs. 😀 |
AndreasBoehm
force-pushed
the
RuntimeData
branch
from
97ffc77 to
d724657
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
include/RuntimeData.h (1)
1-7: Consider forward declarations or moving ArduinoJson include to implementation.
ArduinoJson.his included in the header butJsonDocumentis only used internally in the.cppfile. This increases compilation coupling unnecessarily. Consider moving the include toRuntimeData.cppsince the public API doesn't expose any ArduinoJson types.♻️ Suggested change
// SPDX-License-Identifier: GPL-2.0-or-later `#pragma` once -#include <ArduinoJson.h> `#include` <TaskSchedulerDeclarations.h> `#include` <mutex> `#include` <atomic>The
ArduinoJson.hinclude is already present inRuntimeData.cpp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/RuntimeData.h` around lines 1 - 7, RuntimeData.h currently includes ArduinoJson.h even though ArduinoJson types (e.g., JsonDocument) are only used in the implementation; remove the ArduinoJson.h include from RuntimeData.h to reduce compile coupling, ensure any references in the header use only standard types or forward declarations (no ArduinoJson types are exposed), and keep ArduinoJson.h included in RuntimeData.cpp where JsonDocument or other ArduinoJson symbols are actually used (verify functions/methods in class RuntimeData that manipulate JSON are implemented in RuntimeData.cpp and include ArduinoJson.h there).
🤖 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/RuntimeData.cpp`:
- Around line 81-92: Update the misleading comment and stop treating expected
throttling as an error: change the comment near Utils::getEpoch(&nextEpoch, 5)
to say it returns UTC epoch (not local time), and modify the throttling return
path that checks (_writeEpoch != 0) && (difftime(nextEpoch, _writeEpoch) < 60 *
freezeMinutes) so it does not call cleanExit(false, ...) which logs an error;
either return success (cleanExit(true, "...")) or add a non-error early return
path that returns true/no-error when writes are intentionally skipped due to
freezeMinutes, keeping cleanExit(false, ...) reserved for real failures.
- Around line 54-62: The check-reset of the _writeNow flag in RuntimeClass::loop
has a race where a concurrent requestWriteOnNextTaskLoop() can set it between
the if-check and the reset; make _writeNow an std::atomic<bool> (or protect with
a mutex) and perform an atomic test-and-clear (e.g. use
_writeNow.exchange(false)) so the check-and-clear is atomic, then keep the
getWriteTrigger() check and call write(0) if either condition held; update
requestWriteOnNextTaskLoop() to set the atomic and include <atomic> as needed.
---
Nitpick comments:
In `@include/RuntimeData.h`:
- Around line 1-7: RuntimeData.h currently includes ArduinoJson.h even though
ArduinoJson types (e.g., JsonDocument) are only used in the implementation;
remove the ArduinoJson.h include from RuntimeData.h to reduce compile coupling,
ensure any references in the header use only standard types or forward
declarations (no ArduinoJson types are exposed), and keep ArduinoJson.h included
in RuntimeData.cpp where JsonDocument or other ArduinoJson symbols are actually
used (verify functions/methods in class RuntimeData that manipulate JSON are
implemented in RuntimeData.cpp and include ArduinoJson.h there).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0fe60b2d-7006-4d69-b74a-b302eabc7246
📒 Files selected for processing (10)
include/RuntimeData.hsrc/RuntimeData.cppsrc/WebApi_firmware.cppsrc/WebApi_maintenance.cppsrc/WebApi_sysstatus.cppsrc/main.cppwebapp/src/components/FirmwareInfo.vuewebapp/src/locales/de.jsonwebapp/src/locales/en.jsonwebapp/src/types/SystemStatus.ts
✅ Files skipped from review due to trivial changes (6)
- src/WebApi_sysstatus.cpp
- webapp/src/components/FirmwareInfo.vue
- src/WebApi_maintenance.cpp
- webapp/src/locales/en.json
- webapp/src/types/SystemStatus.ts
- webapp/src/locales/de.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/WebApi_firmware.cpp
|
Hello @AndreasBoehm, |
|
@SW-Niko yes, sure :) |
|
@coderabbitai really no findings? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me do a thorough review of the actual code first. 🏁 Script executed: Length of output: 32619 Let me check for other locale files before finalising. 🏁 Script executed: Length of output: 5716
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | 🐛 Bug | write() |
RUNTIME_VERSION constant used instead of _writeVersion member |
| 2 | 🐛 Bug | write() |
Mutex held during full file I/O — blocks all getters for ~tens of ms |
| 3 | write() |
_writeOK = true when write is suppressed by freeze interval |
|
| 4 | write() |
File truncated before serializeJson — corruption on failure |
|
| 5 | getWriteCountAndTimeString() |
getLocalTime() called while mutex is held |
|
| 6 | WebApi_firmware/maintenance.cpp |
Blocking file I/O on async TCP callback thread | |
| 7 | ℹ️ Info | read() |
ReadMode branches are empty stubs — document intent |
| 8 | ℹ️ Info | RuntimeData.h |
_writeCount is uint16_t — wraps at 65,535 |
| 9 | 🐛 Bug | fr.json |
Missing RuntimeSaveCount translation key |
|
Hello @AndreasBoehm , You will find a description of the feature on this wiki page: Link |
AndreasBoehm
force-pushed
the
RuntimeData
branch
from
ae3a69b to
31f96c1
Compare
- save and load runtime data persistent in a file - read mode ON_DEMAND or ON_STARTUP - the data is stored in JSON format - the data is written during WebApp 'OTA firmware upgrade' and during Webapp 'Reboot' - for security reasons such as 'unexpected power cycles' or 'physical resets', data is also written once a day at 00:05
- into update, reset - WebUI
- mutex no longer held during the entire file I/O - _writeOK indicates successful write operation
AndreasBoehm
force-pushed
the
RuntimeData
branch
from
31f96c1 to
3b8a5cb
Compare
| if (mode == ReadMode::START_UP) { | ||
| ; // todo: deserialize additional runtime data that can be initialized during startup | ||
| } else { | ||
| ; // todo: deserialize additional runtime data that can not be initialized during startup |
There was a problem hiding this comment.
What data to you have in mind that can not be read/initialized during startup?
There was a problem hiding this comment.
Data that uses allocated memory. Like auto upInv = PowerLimiterInverter::create(invConfig);
So far I have only encountered one case... The correction list for inverter transmission in the case of the 24-VDC problem.
There was a problem hiding this comment.
Maybe i am not getting how this will work in the end.
If i trigger read() what will happen?
RuntimeClass will access e.g. PowerLimiterInverter and set a value on it?
There was a problem hiding this comment.
I found a example in a different PR of yours: https://github.com/hoylabs/OpenDTU-OnBattery/pull/2372/changes#diff-783e5c69ea25ae8ae20e5bd1f5faf7895f7ad15abebdb4ead79be611ed91ea23R174
I don't like the idea of injecting stored data into other objects.
Other objects should read from RuntimeData when they need the data.
Other objects will be tightly coupled to the RuntimeData class/object with the current approach.
There was a problem hiding this comment.
I talke to claude about it and this is the result.
It violates both Single Responsibility (the class handles I/O and orchestrates restoration) and Open/Closed (not open for extension without modification).
inversion of control via an interface/protocol
Each subsystem should own its own serialization logic and register itself with a generic runtime-data manager. The manager only knows about the interface, not the concrete classes.
// Interface each subsystem implements
class IRuntimeDataProvider {
public:
virtual std::string getId() const = 0;
virtual JsonObject serialize() const = 0;
virtual void deserialize(const JsonObject& data) = 0;
virtual ~IRuntimeDataProvider() = default;
};
// Generic manager — knows nothing about PowerLimiter etc.
class RuntimeDataManager {
public:
void registerProvider(IRuntimeDataProvider* provider) {
_providers[provider->getId()] = provider;
}
void saveAll(const std::string& path) {
JsonDocument doc;
for (auto& [id, provider] : _providers)
doc[id] = provider->serialize();
writeJsonToDisk(doc, path);
}
void loadAll(const std::string& path) {
JsonDocument doc = readJsonFromDisk(path);
for (auto& [id, provider] : _providers)
if (doc.containsKey(id))
provider->deserialize(doc[id]);
}
private:
std::map<std::string, IRuntimeDataProvider*> _providers;
};Then each subsystem is self-contained:
class PowerLimiter : public IRuntimeDataProvider {
public:
std::string getId() const override { return "powerlimiter"; }
JsonObject serialize() const override { /* pack own state */ }
void deserialize(const JsonObject& data) override { /* restore own state */ }
};And wiring it up happens at the composition root (e.g. main or your app init), not inside the manager:
manager.registerProvider(&powerLimiter);
manager.registerProvider(&battery);
manager.registerProvider(&solar);What you gain:
RuntimeDataManagernever changes when you add a subsystem- Each subsystem owns its own data schema — no leakage
- Trivially testable in isolation
- Ordering, optional providers, missing keys on load all handled generically
This pattern fits well with embedded/OpenDTU-style architectures where subsystems are long-lived singletons — registration in the constructor or an init() call works cleanly.
There was a problem hiding this comment.
I understand your concerns.
RuntimeDataManager never changes when you add a subsystem
I fully agree. (I just reused the pattern from the config)
Each subsystem owns its own data schema — no leakage
This is already the way of working. Hmmm ... maybe claude did not get it because this PR is only the shell?
Trivially testable in isolation
Ok
Ordering, optional providers, missing keys on load all handled generically
Ok, understood:
I will need some time to implement the new concept. 🤔
There was a problem hiding this comment.
Let me think about this for some more time before you change something
4 participants
Read and write runtime data persistent on LittleFS
I should also mention the following points:
See issue #1676
To quickly test the runtime functionality on your system, follow these steps: