Enable full power range of battery-powered HM-xxx inverter on 24VDC#2372
Enable full power range of battery-powered HM-xxx inverter on 24VDC#2372SW-Niko wants to merge 26 commits intohoylabs:developmentfrom
Conversation
read() must be done after all other components have been initialized
Refactored getBatteryPower() to use a state machine approach for better clarity and maintainability. - Now we have 4 different battery states: STOP, NO_DISCHARGE, DISCHARGE_ALLOWED and DISCHARGE_NIGHT - Renamed some variables for better understanding - Improved comments and documentation - State stored in runtime file
Add option AllowStandby to battery powered inverter
Build ArtifactsFirmware built from this pull request's code:
Notice
|
WalkthroughThis PR introduces an Adaptive Transfer Function (ATF) system for inverter power correction, external inverter meter support via multiple protocols (MQTT, HTTP JSON/SML, Serial SDM, UDP Victron), a battery state machine for discharge management, and runtime data persistence to LittleFS with corresponding web API and UI updates. Changes
Sequence DiagramssequenceDiagram
participant Config
participant PowerLimiter
participant InverterATF
participant PowerLimiterInv as PowerLimiterInverter
participant ATFState as ATF State
Config->>PowerLimiter: init() with inverter config
note over PowerLimiter: _checkATF = true
rect rgb(220, 240, 255)
note over PowerLimiter,ATFState: ATF Activation Phase
PowerLimiter->>PowerLimiter: loop() detects _checkATF
PowerLimiter->>PowerLimiter: initATF()
PowerLimiter->>PowerLimiterInv: activateATF(nomPower)
PowerLimiterInv->>InverterATF: activateATF(nomPower)
InverterATF->>ATFState: Initialize correction table<br/>allocate _realPower array
InverterATF-->>PowerLimiterInv: return success
PowerLimiterInv-->>PowerLimiter: ATF active
end
rect rgb(240, 255, 240)
note over PowerLimiter,ATFState: Runtime Power Limiting
PowerLimiter->>PowerLimiterInv: update(target_watts)
PowerLimiterInv->>InverterATF: setATFData(power, limit)
InverterATF->>ATFState: Validate & update<br/>correction table
PowerLimeterInv->>InverterATF: getATFLimit(power)
InverterATF->>ATFState: Interpolate correction
InverterATF-->>PowerLimeterInv: newLimitPercent
PowerLimeterInv-->>PowerLimiter: updated limit
end
rect rgb(255, 240, 240)
note over PowerLimiter,ATFState: Persistence Phase
PowerLimiter->>PowerLimiter: loop() serializes
PowerLimiter->>InverterATF: serializeATFData(json)
InverterATF->>ATFState: Read correction table<br/>& state under mutex
InverterATF-->>PowerLimiter: JSON object
PowerLimiter->>RuntimeData: write(RTD with ATF data)
end
sequenceDiagram
participant Config
participant InverterMeterCtrl as InverterMeters::Controller
participant Provider
participant PowerLimiter
participant PowerLimiterInv as PowerLimiterInverter
participant ExternalMeter
Config->>Config: Load InverterMeter config<br/>(source, endpoint, auth)
rect rgb(240, 255, 240)
note over Config,InverterMeterCtrl: Meter Initialization
Config->>InverterMeterCtrl: updateSettings()
InverterMeterCtrl->>Provider: Create provider<br/>(HTTP_JSON/SML/MQTT/SDM)
Provider->>Provider: init(settings)
Provider-->>InverterMeterCtrl: ready
end
rect rgb(220, 240, 255)
note over InverterMeterCtrl,ExternalMeter: Polling Cycle
InverterMeterCtrl->>InverterMeterCtrl: loop() via scheduler
InverterMeterCtrl->>Provider: loop() (advance polling)
Provider->>ExternalMeter: HTTP/MQTT/Serial request
ExternalMeter-->>Provider: power data
Provider->>Provider: Update last_update,<br/>cache power value
end
rect rgb(255, 245, 240)
note over PowerLimiter,InverterMeterCtrl: Power Limiter Integration
PowerLimiter->>InverterMeterCtrl: getPower(inverter_serial)
InverterMeterCtrl->>Provider: Query cached power
Provider-->>InverterMeterCtrl: float power (optional)
InverterMeterCtrl-->>PowerLimiter: std::optional<float>
PowerLimiter->>PowerLimiterInv: setCurrentOutputAcWatts(power)
PowerLimiterInv->>PowerLimiterInv: Override internal<br/>power reading
end
sequenceDiagram
participant Main
participant RuntimeData
participant Scheduler
participant LittleFS
participant PowerLimiter
participant InverterATF
Main->>RuntimeData: init(scheduler)
RuntimeData->>Scheduler: Create periodic loop task<br/>(minute interval)
rect rgb(220, 240, 255)
note over RuntimeData,Scheduler: Startup Read
RuntimeData->>Main: read(START_UP)
RuntimeData->>LittleFS: Load RTD JSON file
LittleFS-->>RuntimeData: JSON {battery_from_start, atf_data}
RuntimeData->>PowerLimiter: deserializeRTD(data)
RuntimeData->>InverterATF: deserializeATFData(atf)
end
rect rgb(240, 255, 240)
note over RuntimeData,Scheduler: Periodic Writes
Scheduler->>RuntimeData: loop() triggers
RuntimeData->>RuntimeData: getWriteTrigger()<br/>(check min interval)
RuntimeData->>PowerLimiter: serializeRTD(json)
RuntimeData->>InverterATF: serializeATFData(json)
RuntimeData->>LittleFS: Write RTD JSON<br/>{version, save_count,<br/>save_epoch, data}
end
rect rgb(255, 240, 240)
note over RuntimeData,LittleFS: Persistence Events
Main->>RuntimeData: write(freezeMinutes) on reboot
RuntimeData->>RuntimeData: Check last write<br/>vs freeze time
RuntimeData->>LittleFS: Persist current state
LittleFS-->>RuntimeData: Write complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
WalkthroughIntroduces three major features: ATF (Adaptive Thermal Framework) for inverter power correction, InverterMeter for reading power data via multiple protocols, and RuntimeData for persistent runtime state management. Extends PowerLimiter configuration, web APIs, and frontend UI to support these features alongside battery state machine refactoring. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser / UI
participant API as Web API
participant IL as PowerLimiter
participant INV as PowerLimiterInverter
participant ATF as InverterATF
participant RD as RuntimeData
participant FS as LittleFS
UI->>API: POST /api/invertermeter/config
API->>IL: updateInverterMeterSettings()
IL->>IL: setATFData(power, limit)
IL->>INV: via PowerLimiterInverter
INV->>ATF: setATFData(power, limit)
ATF->>ATF: Update correction table
Note over IL,ATF: ATF activation/management
IL->>INV: activateATF(nomPower)
INV->>ATF: activateATF(nomPower)
ATF->>ATF: Initialize power array
Note over RD,FS: Runtime data persistence
API->>RD: write(freezeMinutes)
RD->>RD: Serialize ATF + config state
RD->>FS: Write /runtime.json
FS->>RD: Success
RD->>API: Update complete
Note over IL,ATF: Power limit computation with ATF
IL->>INV: getCurrentLimitWatts()
INV->>ATF: isATFActive()?
alt ATF Active
INV->>ATF: getATFLimit(power)
ATF->>ATF: Interpolate correction table
ATF->>INV: Return corrected limit
else ATF Inactive
INV->>INV: Use linear calculation
end
INV->>IL: Return limit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webapp/src/views/PowerLimiterAdminView.vue (1)
741-750: Missing default value foruse_atfin new inverter config.When creating a default config for new inverters in
tidyUpInverterConfigs, theuse_atfproperty is not initialized. This may cause issues or warnings when the UI tries to bind toinv.use_atf.🔎 Proposed fix
newInv.power_source = 0; // battery newInv.use_overscaling_to_compensate_shading = false; newInv.allow_standby = true; + newInv.use_atf = false; inverters.push(newInv);
🧹 Nitpick comments (22)
include/PowerLimiterInverter.h (1)
96-97: Address or remove the TODO comment.This method is marked for deletion after testing. Consider tracking this in an issue or removing it before merging if testing is complete.
Do you want me to open an issue to track removing
getATFConfigPower()after testing is completed?src/RuntimeData.cpp (3)
10-10: Typo: "Threadsave" should be "Thread-safe".- * - Threadsave access to the data is provided by a mutex. + * - Thread-safe access to the data is provided by a mutex.
180-180: Minor: Unnecessary close on potentially unopened file.
fRuntime.close()is called unconditionally, but ifLittleFS.open()failed (line 154),fRuntimewould be invalid. While this is safe (callingclose()on an invalidFileis a no-op on ESP32), it's clearer to close only when open succeeded.🔎 Suggested fix
- if (fRuntime) { fRuntime.close(); } + fRuntime.close(); // safe even if not opened, but consider moving inside the if(fRuntime) block above for clarityOr restructure to close within the success block at line 161.
224-225: Consider clarifying the intent of thegetLocalTimecheck.The
getLocalTime(&time, 5)call on line 224 populatestime, but line 225 immediately overwrites it withlocaltime_r(&_writeEpoch, &time). The intent appears to be checking if time synchronization has occurred before converting the epoch. Consider using a clearer pattern:🔎 Suggested clarification
- if ((_writeEpoch != 0) && (getLocalTime(&time, 5))) { - localtime_r(&_writeEpoch, &time); + struct tm unused; + if ((_writeEpoch != 0) && (getLocalTime(&unused, 5))) { + localtime_r(&_writeEpoch, &time);Or add a comment explaining that
getLocalTimeis used as a time-sync availability check.include/InverterATF.h (1)
58-64: Clarify locking comments forgetATFPowerandgetATFLimit.The comments state "use a shared/unique lock when calling this method", but looking at the implementation in
src/InverterATF.cpp, these methods acquire locks internally (shared lock first, then upgrade to unique if cache miss). The current comments may mislead callers into thinking they need to hold a lock before calling.🔎 Suggested clarification
- // returns the power for the given limit according to the ATF - // use a shared/unique lock when calling this method + // returns the power for the given limit according to the ATF + // internally manages locking (shared lock with upgrade to unique on cache miss) uint16_t getATFPower(float const limit) const; - // returns the limit for the given power according to the ATF - // use a shared/unique lock when calling this method + // returns the limit for the given power according to the ATF + // internally manages locking (shared lock with upgrade to unique on cache miss) float getATFLimit(uint16_t const power) const;webapp/src/components/FirmwareInfo.vue (1)
86-89: Consider applying consistent number formatting.Line 84 uses
$n(systemStatus.cfgsavecount, 'decimal')forConfigSaveCount, but line 88 displaysruntime_savecountwithout number formatting. For consistency, consider applying the same formatting:🔎 Suggested fix
<tr> <th>{{ $t('firmwareinfo.RuntimeSaveCount') }}</th> - <td>{{ systemStatus.runtime_savecount }}</td> + <td>{{ $n(systemStatus.runtime_savecount, 'decimal') }}</td> </tr>src/PowerLimiterBatteryInverter.cpp (2)
52-52: TODO comment noted.The question about whether it would be safer to set the lower limit again as a fallback is valid. While not blocking, this defensive measure could prevent unexpected states if the standby path is not taken.
Would you like me to propose an implementation that sets the lower limit as a fallback in this case?
2-2: Remove the unused LogHelper.h include from line 2.The
#include "LogHelper.h"doesn't have any corresponding logging calls in the file. If logging will be added later, this include can be restored at that time.include/WebApi_invertermeter.h (1)
6-7: Consider removing unused includes from header.
ArduinoJson.handConfiguration.happear to be unused in this header file. They may only be needed in the implementation file (WebApi_invertermeter.cpp). Moving them there would reduce compilation dependencies.🔎 Suggested change
#include <ESPAsyncWebServer.h> #include <TaskSchedulerDeclarations.h> -#include <ArduinoJson.h> -#include "Configuration.h"Move these includes to
src/WebApi_invertermeter.cppif they're needed there.webapp/src/views/PowerLimiterAdminView.vue (1)
542-552: Consider simplifying withArray.some().The
isATFFullcomputed property correctly limits ATF to one inverter. A minor simplification usingArray.some()would be more idiomatic:🔎 Suggested simplification
isATFFull(): boolean { - let full = false; - for (const inv of this.powerLimiterConfigList.inverters) { - if (inv.use_atf) { - // currently ATF is limited to 1 inverter only - full = true; - break; - } - } - return full; + // currently ATF is limited to 1 inverter only + return this.powerLimiterConfigList.inverters?.some((inv) => inv.use_atf) ?? false; },src/PowerLimiterInverter.cpp (2)
195-201: ATF‑aware limit/power calculations integrate cleanly; consider optional safety clampUsing
isATFActive()to switch betweengetATFLimit(...)/getATFPower(...)and the existing linear calculation keeps the old behavior as a fallback and localizes ATF logic well. As a small hardening step, you might consider clampingnewRelativeLimit(and the ATF‑derived power) to a sane range (e.g. 0–100% / 0–getInverterMaxPowerWatts()) before logging and sending it on the wire, to guard against unexpected ATF table contents or upstream data glitches.Also applies to: 354-364
318-320: Tighten external inverter‑meter handling and align styleFunctionality-wise this works (external AC power is preferred when recent and the inverter is eligible), but a few refinements would help:
- The new function body style (
...timestamp) {) and the missing space intimestamp == 0||are inconsistent with the rest of the file and may trip cpplint; consider moving{to the next line and fixing spacing.- Since
_oInverterMeterPoweris afloatbutgetCurrentOutputAcWatts()returnsuint16_t, an explicitstd::lround/clamp to[0, getInverterMaxPowerWatts()]would make the narrowing intentional and avoid surprising truncation if a slightly out‑of‑range value is ever passed in.- If
falsefromsetCurrentOutputAcWattsis meant to indicate “wait for fresher meter data before acting”, it’s worth double‑checking the caller logic to ensure that path really does defer DPL decisions as intended.Also applies to: 464-481
src/WebApi_invertermeter.cpp (1)
219-260: HTTP test endpoints are useful; add a small guard around empty resultsThe two test endpoints follow the existing PowerMeter pattern: parse a one‑off config, construct a provider,
init()+poll(), and return either a formatted success string or the provider’s error string. This is a good debugging aid.For
onTestHttpJsonRequest, you currently assume thatDataPointContaineris non‑empty wheneverpoll()returns the variant’s “values” alternative:auto const& vals = std::get<values_t>(res); auto iter = vals.cbegin(); auto pos = snprintf(..., iter->second.getValueText().c_str());If the provider implementation ever returns an empty container on success, dereferencing
iterwould be undefined. Adding a quickif (vals.empty())branch that returns a “no values” message (or treats it as an error) before dereferencing would make this bullet‑proof without changing the normal success path. The SML test endpoint already handles its “no data” case via an explicitisEmpty()check, so this would just align the JSON path.Also applies to: 263-295
webapp/src/types/InverterMeterConfig.ts (1)
19-25: Inconsistent interface naming:PowerMeterHttpJsonValueshould beInverterMeterHttpJsonValue.All other interfaces use the
InverterMeterprefix (e.g.,InverterMeterMqttValue,InverterMeterMqttConfig), but this interface usesPowerMeter. Consider renaming for consistency.🔎 Proposed fix
-export interface PowerMeterHttpJsonValue { +export interface InverterMeterHttpJsonValue { http_request: HttpRequestConfig; enabled: boolean; json_path: string; unit: number; sign_inverted: boolean; }And update line 30 accordingly:
- values: Array<PowerMeterHttpJsonValue>; + values: Array<InverterMeterHttpJsonValue>;src/InverterATF.cpp (1)
46-49: Redundant nullptr check afterstd::make_unique.
std::make_uniquethrowsstd::bad_allocon allocation failure rather than returning nullptr. This check will never trigger in standard C++.🔎 Proposed fix
// allocate memory for the ATF data array _realPower = std::make_unique<float[]>(_size); - if (_realPower == nullptr) { - DTU_LOGE("Unable to allocate memory for ATF data array"); - return false; - }If you need to handle allocation failures gracefully without exceptions, consider using
std::unique_ptr<float[]>(_size, std::nothrow)with appropriate error handling, or wrap in a try-catch block.src/invertermeter/Controller.cpp (2)
52-79: Add a default case to the switch statement.If
imcfg.Sourcecontains an unexpected value, the switch silently falls through without setting_upProvider. Consider adding a default case to log a warning or handle the unknown source type explicitly.🔎 Proposed fix
case PowerMeters::Provider::Type::MODBUS_UDP_VICTRON: _upProvider = std::make_unique<::PowerMeters::Modbus::Udp::Victron::Provider>(imcfg.UdpVictron); break; + default: + // Unknown source type - _upProvider remains nullptr + break; }
93-107: Potential TOCTOU: Configuration read before mutex acquisition.
Configuration.get().InverterMeter.Serialis read at line 96 before acquiring the mutex at line 98. If configuration can be updated concurrently, this could lead to inconsistent behavior. Consider moving the configuration read inside the lock, or document that configuration changes require explicit synchronization.🔎 Proposed fix
std::optional<float> Controller::getPower(uint64_t inverterID) const { - // for now we only support one inverter meter - auto serial = Configuration.get().InverterMeter.Serial; - std::lock_guard<std::mutex> l(_mutex); + // for now we only support one inverter meter + auto serial = Configuration.get().InverterMeter.Serial; + if (inverterID != serial) { return std::nullopt; }include/RuntimeData.h (1)
22-22: Document the freezeMinutes behavior.The
writemethod'sfreezeMinutesparameter throttles writes, but the default of 10 minutes may surprise callers. Consider adding a brief doc comment or ensuring this is documented in the implementation.include/invertermeter/Controller.h (1)
4-13: Locking policy comment is misleading.The comment at line 6 mentions "shared lock" but
std::mutex(line 47) only provides exclusive locking. For actual shared/exclusive locking,std::shared_mutexwithstd::shared_lockwould be needed. Consider updating the comment to reflect that all locks are exclusive, or switch tostd::shared_mutexif reader-writer semantics are intended.src/PowerLimiter.cpp (1)
1049-1062: Remove commented-out debug code.Lines 1052 and 1054 contain commented-out code that appears to be debug/testing remnants. Clean up for production readiness.
Proposed cleanup
auto maxPower = upInv->getInverterMaxPowerWatts(); - //if (maxPower == 0) { maxPower = upInv->getATFConfigPower(); } // todo: delete after testing if (maxPower == 0) { - //DTU_LOGW("cannot initialize ATF: inverter max power unknown"); allDone = false; continue; } if (!upInv->activateATF(maxPower)) { - //DTU_LOGW("cannot initialize ATF: activate data structure"); allDone = false; continue; }webapp/src/views/InverterMeterAdminView.vue (2)
350-358: Missing error handling for fetch operations.
getInverterMeterConfig()doesn't handle network errors or rejected promises. If the fetch fails,dataLoadingremainstrueindefinitely, leaving the UI stuck in a loading state. Add a.catch()handler or use try/catch with async/await.Proposed fix
getInverterMeterConfig() { this.dataLoading = true; fetch('/api/invertermeter/config', { headers: authHeader() }) .then((response) => handleResponse(response, this.$emitter, this.$router)) .then((data) => { this.inverterMeterConfigList = data; this.dataLoading = false; - }); + }) + .catch((error) => { + this.dataLoading = false; + this.alertMessage = error.message || 'Failed to load configuration'; + this.alertType = 'danger'; + this.showAlert = true; + }); },
378-401: Hardcoded strings should use i18n.Lines 380 and 404 use hardcoded English strings "Triggering HTTP request..." instead of
this.$t(). For consistency with the rest of the component, these should be localized.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
include/Configuration.h(2 hunks)include/InverterATF.h(1 hunks)include/PowerLimiter.h(3 hunks)include/PowerLimiterInverter.h(4 hunks)include/RuntimeData.h(1 hunks)include/WebApi.h(2 hunks)include/WebApi_invertermeter.h(1 hunks)include/invertermeter/Controller.h(1 hunks)src/Configuration.cpp(4 hunks)src/Datastore.cpp(2 hunks)src/InverterATF.cpp(1 hunks)src/PowerLimiter.cpp(15 hunks)src/PowerLimiterBatteryInverter.cpp(3 hunks)src/PowerLimiterInverter.cpp(4 hunks)src/RuntimeData.cpp(1 hunks)src/WebApi.cpp(1 hunks)src/WebApi_firmware.cpp(2 hunks)src/WebApi_invertermeter.cpp(1 hunks)src/WebApi_maintenance.cpp(2 hunks)src/WebApi_sysstatus.cpp(2 hunks)src/WebApi_ws_live.cpp(2 hunks)src/invertermeter/Controller.cpp(1 hunks)src/main.cpp(2 hunks)webapp/src/components/FirmwareInfo.vue(1 hunks)webapp/src/components/NavBar.vue(1 hunks)webapp/src/locales/de.json(4 hunks)webapp/src/locales/en.json(4 hunks)webapp/src/router/index.ts(2 hunks)webapp/src/types/InverterMeterConfig.ts(1 hunks)webapp/src/types/PowerLimiterConfig.ts(1 hunks)webapp/src/types/SystemStatus.ts(1 hunks)webapp/src/views/InverterMeterAdminView.vue(1 hunks)webapp/src/views/PowerLimiterAdminView.vue(2 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_sysstatus.cppsrc/PowerLimiterBatteryInverter.cppsrc/PowerLimiterInverter.cppsrc/WebApi_firmware.cppinclude/Configuration.hsrc/WebApi_invertermeter.cppsrc/WebApi_maintenance.cppsrc/main.cppsrc/WebApi.cppinclude/WebApi.hinclude/WebApi_invertermeter.hsrc/WebApi_ws_live.cppsrc/InverterATF.cppsrc/invertermeter/Controller.cppinclude/PowerLimiter.hsrc/PowerLimiter.cppsrc/RuntimeData.cppinclude/InverterATF.hsrc/Datastore.cppinclude/invertermeter/Controller.hsrc/Configuration.cppinclude/PowerLimiterInverter.hinclude/RuntimeData.h
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/types/PowerLimiterConfig.tswebapp/src/router/index.tswebapp/src/components/FirmwareInfo.vuewebapp/src/types/SystemStatus.tswebapp/src/types/InverterMeterConfig.tswebapp/src/views/PowerLimiterAdminView.vuewebapp/src/components/NavBar.vuewebapp/src/views/InverterMeterAdminView.vue
🧠 Learnings (2)
📓 Common learnings
Learnt from: SW-Niko
Repo: hoylabs/OpenDTU-OnBattery PR: 2262
File: src/main.cpp:149-159
Timestamp: 2025-10-17T08:29:40.205Z
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).
📚 Learning: 2025-10-17T08:29:40.205Z
Learnt from: SW-Niko
Repo: hoylabs/OpenDTU-OnBattery PR: 2262
File: src/main.cpp:149-159
Timestamp: 2025-10-17T08:29:40.205Z
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/WebApi_firmware.cppsrc/WebApi_maintenance.cppsrc/main.cppinclude/PowerLimiter.hsrc/PowerLimiter.cppsrc/RuntimeData.cppinclude/RuntimeData.h
🧬 Code graph analysis (8)
src/PowerLimiterBatteryInverter.cpp (4)
src/PowerLimiterInverter.cpp (4)
getCurrentOutputAcWatts(316-320)getCurrentOutputAcWatts(316-316)getCurrentLimitWatts(354-364)getCurrentLimitWatts(354-354)src/PowerLimiterSmartBufferInverter.cpp (2)
standby(87-92)standby(87-87)src/PowerLimiterSolarInverter.cpp (2)
standby(122-128)standby(122-122)src/PowerLimiterOverscalingInverter.cpp (2)
setAcOutput(120-129)setAcOutput(120-120)
src/WebApi_invertermeter.cpp (1)
src/WebApi.cpp (2)
init(18-53)init(18-18)
webapp/src/types/InverterMeterConfig.ts (1)
webapp/src/types/HttpRequestConfig.ts (1)
HttpRequestConfig(1-9)
include/WebApi_invertermeter.h (2)
src/WebApi.cpp (2)
init(18-53)init(18-18)src/WebApi_invertermeter.cpp (12)
init(18-29)init(18-18)onStatus(32-68)onStatus(32-32)onAdminGet(71-78)onAdminGet(71-71)onAdminPost(81-216)onAdminPost(81-81)onTestHttpJsonRequest(219-260)onTestHttpJsonRequest(219-219)onTestHttpSmlRequest(263-295)onTestHttpSmlRequest(263-263)
include/PowerLimiter.h (1)
src/PowerLimiter.cpp (12)
serializeRTD(1015-1020)serializeRTD(1015-1015)deserializeRTD(1022-1027)deserializeRTD(1022-1022)initATF(1036-1074)initATF(1036-1036)serializeATFtoRTD(1079-1091)serializeATFtoRTD(1079-1079)deserializeRTDtoATF(1096-1111)deserializeRTDtoATF(1096-1096)getATFInverterPower(1117-1127)getATFInverterPower(1117-1117)
include/InverterATF.h (1)
src/InverterATF.cpp (18)
activateATF(37-69)activateATF(37-37)deactivateATF(75-84)deactivateATF(75-75)setATFData(91-178)setATFData(91-91)deserializeATFData(341-369)deserializeATFData(341-341)serializeATFData(325-335)serializeATFData(325-325)printATFReport(375-429)printATFReport(375-375)getATFPower(215-254)getATFPower(215-215)getATFLimit(261-319)getATFLimit(261-261)makeAveragePower(184-208)makeAveragePower(184-184)
include/invertermeter/Controller.h (1)
src/invertermeter/Controller.cpp (18)
init(31-39)init(31-31)updateSettings(41-84)updateSettings(41-41)getPowerTotal(86-91)getPowerTotal(86-86)getLastUpdate(122-127)getLastUpdate(122-122)isDataValid(129-134)isDataValid(129-129)getPower(93-107)getPower(93-93)getTime(109-120)getTime(109-109)getRequestTime(136-148)getRequestTime(136-136)loop(150-157)loop(150-150)
include/PowerLimiterInverter.h (2)
src/PowerLimiterInverter.cpp (5)
PowerLimiterInverter(35-46)setCurrentOutputAcWatts(464-481)setCurrentOutputAcWatts(464-464)getCurrentOutputAcWatts(316-320)getCurrentOutputAcWatts(316-316)include/InverterATF.h (1)
InverterATF(21-85)
🪛 GitHub Actions: cpplint
src/InverterATF.cpp
[error] 399-399: cpplint: Do not use variable-length arrays. Use an appropriately named ('k' followed by CamelCase) compile-time constant for the size. [runtime/arrays]
🔇 Additional comments (50)
webapp/src/types/PowerLimiterConfig.ts (1)
32-32: LGTM!The
use_atfboolean field is correctly added to the interface, following the existing snake_case naming convention used for other fields likeis_governedandis_behind_power_meter.include/PowerLimiterInverter.h (2)
78-94: LGTM!The ATF integration methods are well-designed:
- Clear separation between
isATFEnabled()(config flag) andisATFActive()(runtime state from base class)setATFData()correctly pairs current output power with the current limit percentage- The comment on line 89-90 helpfully documents the synchronization requirement
162-164: LGTM!The
_oInverterMeterPoweroptional member correctly stores the external meter value. Usingstd::optional<float>allows distinguishing between "no value available" and "value is zero", which is appropriate for sensor data that may be unavailable.src/RuntimeData.cpp (1)
78-140: LGTM!The
write()method is well-structured:
- Uses a clean exit lambda for consistent logging and state updates
- Properly guards shared state with mutex
- Only commits new state (
_writeVersion,_writeEpoch,_writeCount) after successful file write- File handle is properly closed in all code paths
include/InverterATF.h (2)
21-28: LGTM!The class is correctly marked as non-copyable and non-movable (deleted copy/move constructors and assignments), which is appropriate since it manages unique resources (
_realPowerunique_ptr) and contains synchronization primitives (_mutex).
70-79: LGTM!Good use of
std::atomic<bool>for_useATFto allow lock-free reads for quick eligibility checks. Themutablequalifiers on_cacheand_mutexcorrectly enable their modification inconstmember functions. The fixed size of 101 elements supports direct percentage indexing (0-100%).webapp/src/components/FirmwareInfo.vue (1)
87-87: No action required. Both the translation keyRuntimeSaveCountand the type fieldruntime_savecountare properly defined and implemented.webapp/src/types/SystemStatus.ts (1)
34-34: LGTM!The new
runtime_savecountfield is correctly typed as a string and logically placed alongsidecfgsavecount. This aligns with the backend API changes inWebApi_sysstatus.cppthat populate this field viaRuntimeData.getWriteCountAndTimeString().src/WebApi_sysstatus.cpp (2)
17-17: LGTM!The include is necessary for the RuntimeData usage on line 83.
83-83: LGTM!The runtime save count is correctly exposed via the system status API, aligning with the frontend
SystemStatustype definition. The placement aftercfgsavecountis logical and maintains consistency.webapp/src/components/NavBar.vue (1)
89-93: LGTM!The new Inverter Meter Settings navigation item follows the established pattern and is logically placed after Power Meter Settings. The routing and translation key structure are consistent with existing menu items.
src/WebApi_maintenance.cpp (2)
11-11: LGTM!The include is necessary for RuntimeData usage on line 49.
47-50: LGTM!Persisting runtime data before reboot is appropriate. The placement after sending the response but before triggering the restart ensures the write operation completes. The 60-minute throttle prevents excessive writes while still capturing recent state changes.
src/WebApi.cpp (1)
44-44: LGTM!The Inverter Meter web API initialization follows the established pattern and is logically positioned between Power Meter and Power Limiter. This aligns with the member declaration in
include/WebApi.h.src/PowerLimiterBatteryInverter.cpp (1)
13-13: LGTM - Strengthened standby condition.The standby logic now requires both the
allowStandbyparameter AND the_config.AllowStandbyflag to be true. This adds configuration-level control over standby behavior, which is appropriate for the ATF integration where standby must be more strictly controlled. The change is consistently applied across all three locations where standby is invoked.Also applies to: 48-48, 60-60
include/WebApi.h (2)
23-23: LGTM!The include is necessary for the
WebApiInverterMeterClassmember declaration on line 78 and follows the established pattern for web API component headers.
78-78: LGTM!The Inverter Meter web API component is correctly declared and logically positioned between Power Meter and Power Limiter, matching the initialization order in
src/WebApi.cpp.src/WebApi_firmware.cpp (2)
13-13: LGTM!The include is necessary for RuntimeData usage on line 53.
51-54: LGTM!Persisting runtime data before the firmware update restart is appropriate and consistent with the reboot handling in
WebApi_maintenance.cpp. The placement after sending the response ensures the user receives feedback, and the 60-minute throttle is reasonable for this infrequent operation.src/WebApi_ws_live.cpp (2)
17-17: LGTM!The inclusion of
PowerLimiter.his necessary for accessinggetATFInverterPower().
276-281: LGTM!The ATF power integration looks correct. The code properly uses
std::optionalpattern to check for ATF availability and falls back to the standard calculation when ATF is not active for this inverter.webapp/src/router/index.ts (2)
15-15: LGTM!Import follows the established naming convention and is appropriately placed alphabetically.
131-135: LGTM!The new route is correctly defined and follows the existing route structure patterns. The route name and path are consistent with other settings routes.
src/Datastore.cpp (2)
8-8: LGTM!Include for the InverterMeter controller is appropriately added.
95-106: Potential inconsistency: InverterMeter path ignores polling state.When
InverterMeter.getPower()returns a value, the power is accumulated regardless of whether polling is enabled for the inverter (lines 96-98). However, the fallback path (lines 100-104) only accumulates power wheninv->getEnablePolling()is true.This could lead to unexpected behavior where an inverter with polling disabled still contributes to
_totalAcPowerEnabledwhen using InverterMeter. Please verify if this is intentional or if the InverterMeter path should also checkinv->getEnablePolling().include/Configuration.h (2)
152-152: LGTM!The
UseATFboolean flag is appropriately added to the inverter configuration structure.
441-450: LGTM!The
InverterMeterConfigstruct follows the same pattern asPowerMeterConfig, reusing existing power meter configuration types for different providers. Serialization and deserialization are properly implemented in the corresponding read/write methods in Configuration.cpp, with all member fields (Enabled, Source, Serial, and nested configurations) correctly handled.include/WebApi_invertermeter.h (1)
9-21: LGTM!The class structure follows the established pattern for WebApi classes in the codebase. The public
init()method and private handler methods are appropriately declared.webapp/src/views/PowerLimiterAdminView.vue (2)
164-168: LGTM!The condition is updated to show
AllowStandbyfor both battery-powered (0) and SmartBuffer-powered (2) inverters, which aligns with the ATF feature requirements.
170-178: LGTM!The
UseATFinput is correctly:
- Shown only for battery-powered inverters (
power_source == 0)- Disabled when ATF is already assigned to another inverter (
isATFFull && !inv.use_atf)This enforces the stated limitation that ATF is available for only one inverter at a time.
src/Configuration.cpp (1)
221-230: ATF flag and InverterMeter config wiring look consistent and backward compatible
use_atfis serialized and deserialized symmetrically with a safe default (false), and the newinvertermetersection reuses the existing powermeter (de)serialization helpers so legacy configs will naturally fall back to defaults without special migration. No issues from my side here.Also applies to: 433-452, 665-679, 924-932
src/main.cpp (1)
32-32: InverterMeter and RuntimeData initialization order is correctIncluding the new controllers and placing
InverterMeter.init(...)before PowerLimiter/GridCharger/Battery, thenRuntimeData.init(...)andRuntimeData.read()afterwards, respects the documented requirement that RuntimeData be initialized after these components but before entering the main loop. Based on learnings, this order looks right.Also applies to: 40-40, 150-162
webapp/src/locales/de.json (1)
15-16: DE localization for Inverter Meter, RuntimeData, and ATF matches backend/EN keysThe added German strings (menu entry, firmware runtime counter,
invertermeteradminblock, and ATF‑related descriptions) line up with the English keys and backend expectations, and the terminology is consistent with existing DPL wording. No changes needed.Also applies to: 286-287, 678-684, 740-743
webapp/src/locales/en.json (1)
15-16: EN localization additions for Inverter Meter and ATF are coherent with the new featuresThe new English keys for the Inverter Meter menu/admin view, runtime save counter, and ATF options are clear, technically accurate, and consistent with existing DPL terminology. They also match the corresponding DE keys, which keeps the locales in sync.
Also applies to: 286-287, 678-684, 740-743
webapp/src/types/InverterMeterConfig.ts (1)
1-53: Well-structured type definitions for InverterMeter configuration.The interfaces correctly model the configuration hierarchy for various meter sources (MQTT, SDM, HTTP JSON/SML, UDP Victron) and properly import the shared
HttpRequestConfigtype.src/InverterATF.cpp (2)
37-69: Proper mutex usage and initialization logic.The
activateATFfunction correctly usesstd::unique_lockfor exclusive access, initializes the correction table with linear values, and handles the already-initialized case. The state management with atomic_useATFflag is appropriate.
215-254: The_cachemember is properly declared asmutablein the header file (include/InverterATF.h, line 78), allowing bothgetATFPowerandgetATFLimitto modify it despite being const methods. The implementation is correct.src/invertermeter/Controller.cpp (1)
31-39: Clean initialization with TaskScheduler integration.The controller properly initializes the loop task with the scheduler, binds the callback, and calls
updateSettings()to configure the provider. Good use ofTASK_FOREVERfor continuous operation.include/PowerLimiter.h (3)
61-68: Well-designed RTD/ATF public interface.The new public methods for runtime data and ATF functionality are cleanly organized:
- Serialization/deserialization pairs for RTD and ATF
initATFreturning bool for initialization statusgetATFInverterPowerreturningstd::optionalfor safe value retrieval
87-90: Clear battery state machine design.The
BatteryStateenum with explicit states (STOP, NO_DISCHARGE, DISCHARGE_ALLOWED, DISCHARGE_NIGHT) provides better readability and type safety compared to raw booleans. The associated state variables_fromStartand_oneStopPerNightDonesupport the logic well.
37-37: New status enumerator for inverter power meter pending state.
InverterPowerMeterPendingextends the status enum to reflect the additional state when waiting for inverter meter data, aligning with the new InverterMeter integration.include/RuntimeData.h (3)
10-17: Proper non-copyable, non-movable semantics.The class correctly deletes copy and move constructors/assignment operators, which is appropriate for a singleton-like runtime data manager that owns scheduler tasks and file handles.
37-45: Good use of atomics and mutex for thread safety.The design appropriately uses:
std::atomic<bool>for flags that are read/written from different contexts (_readOK,_writeOK,_readNow,_writeNow)mutable std::mutexfor protecting shared state that may be accessed from const methods
48-48: RuntimeData initialization order is correct. The code initializes SolarCharger, PowerMeter, PowerLimiter, GridCharger, and Battery before RuntimeData, then calls RuntimeData.init() and RuntimeData.read() before the main loop. The initialization sequence matches the required ordering and is properly documented with inline comments.include/invertermeter/Controller.h (1)
22-49: LGTM!The
Controllerclass is well-structured with a clean public interface and appropriate thread-safety via mutex. The implementation inController.cppcorrectly usesstd::lock_guardfor all public accessors.src/PowerLimiter.cpp (4)
60-73: LGTM!The new
InverterPowerMeterPendingstatus is properly added with the frozen::map size updated to 12 to match the entry count.
676-682: LGTM!The early exit to standby all battery-powered inverters when
BatteryState::STOPis active prevents further battery discharge below the stop threshold.
1117-1127: Consider whether zero power should returnstd::nullopt.Line 1124 returns
std::nulloptifpower == 0. If zero is a valid ATF power output (e.g., inverter producing nothing), the UI might want to display "0 W" rather than omitting the value. Verify this is the intended behavior for the live view display.
211-222: No actionable issues found. The code correctly handles unavailable InverterMeter data: whengetPower()returnsstd::nullopt, the fallback to0.0ftriggers thepower <= 0.0fcheck insidesetCurrentOutputAcWatts(), which returnstrueand allows the system to proceed using internal inverter measurements, as intended.Likely an incorrect or invalid review comment.
webapp/src/views/InverterMeterAdminView.vue (1)
282-335: LGTM!Component structure follows existing patterns in the codebase. Data types are properly defined, and the component correctly uses existing UI components like
BasePage,CardElement, andInputElement.
| void PowerLimiterClass::serializeRTD(JsonObject const& obj) const | ||
| { | ||
| // Note: Up to now the PowerLimiterClass does not use a mutex | ||
| // Use of atomic<bool> would be an solution but I want to avoid the overhead | ||
| obj["battery_from_start"] = _fromStart; | ||
| } | ||
|
|
||
| void PowerLimiterClass::deserializeRTD(JsonObject const& obj) | ||
| { | ||
| // Note: Up to now the PowerLimiterClass does not use a mutex | ||
| // Use of atomic<bool> would be an solution but I want to avoid the overhead | ||
| _fromStart = obj["battery_from_start"] | false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find call sites of serializeRTD and deserializeRTD
echo "=== serializeRTD/deserializeRTD call sites ==="
rg -n "serializeRTD|deserializeRTD" --type cpp -B 2 -A 2
# Find all _fromStart accesses
echo -e "\n=== _fromStart variable accesses ==="
rg -n "_fromStart" --type cpp -B 1 -A 1
# Check for threading/task creation in PowerLimiter
echo -e "\n=== Loop function and task context ==="
rg -n "void.*loop|xTaskCreate|xTaskCreatePinnedToCore" src/PowerLimiter.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 4099
🏁 Script executed:
#!/bin/bash
# Check where RuntimeData functions are called from (to see if different task)
echo "=== RuntimeData function calls ==="
rg -n "RuntimeData\." --type cpp -B 2 -A 2 | head -50
# Look for task creation related to RuntimeData or file I/O
echo -e "\n=== Task creation patterns ==="
rg -n "xTaskCreate|xTaskCreatePinnedToCore|Thread" --type cpp | grep -E "(RuntimeData|File|SPIFFS)" | head -20
# Check if RuntimeData runs on different core/task
echo -e "\n=== RuntimeData.cpp file to understand execution context ==="
head -50 src/RuntimeData.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 4332
🏁 Script executed:
#!/bin/bash
# Check RuntimeData's mutex protection and how it calls serializeRTD/deserializeRTD
echo "=== RuntimeData.write() and read() with mutex context ==="
sed -n '100,200p' src/RuntimeData.cpp
# Also check the lock/mutex handling
echo -e "\n=== Mutex usage in RuntimeData ==="
rg -n "mutex|lock" src/RuntimeData.cpp -i -B 2 -A 2Repository: hoylabs/OpenDTU-OnBattery
Length of output: 6137
Fix unprotected access to _fromStart in deserializeRTD.
The deserializeRTD method writes to _fromStart without mutex protection, while loop() continuously reads and writes this variable. Although RuntimeData.write() correctly protects serializeRTD with a mutex lock, the corresponding deserializeRTD call at RuntimeData.cpp:174 executes outside the mutex scope, contradicting the code comment at lines 171-172 which instructs to "protect the shared data with a local mutex."
This creates a data race between the scheduled RuntimeData.read() task (running once per minute) and PowerLimiter.loop(). Protect _fromStart with either: (1) a mutex lock in deserializeRTD, (2) wrapping the deserializeRTD call in RuntimeData.read() with the same mutex used for serializeRTD, or (3) using std::atomic<bool> for _fromStart if performance is a concern.
🤖 Prompt for AI Agents
In src/PowerLimiter.cpp around lines 1015 to 1027, deserializeRTD writes to
_fromStart without the mutex used to protect serializeRTD, causing a data race
with loop(); fix by protecting _fromStart consistently — either make _fromStart
std::atomic<bool> and leave methods lock-free, or (preferred) acquire the same
local mutex used by RuntimeData::write()/serializeRTD before assigning
_fromStart in deserializeRTD so reads/writes are synchronized; ensure you use
the identical mutex object and scope as serializeRTD to fully eliminate the
race.
| void WebApiInverterMeterClass::onAdminPost(AsyncWebServerRequest* request) | ||
| { | ||
| if (!WebApi.checkCredentials(request)) { | ||
| return; | ||
| } | ||
|
|
||
| AsyncJsonResponse* response = new AsyncJsonResponse(); | ||
| JsonDocument root; | ||
| if (!WebApi.parseRequestData(request, response, root)) { | ||
| return; | ||
| } | ||
|
|
||
| auto& retMsg = response->getRoot(); | ||
|
|
||
| if (!(root["enabled"].is<bool>() && root["source"].is<uint32_t>())) { | ||
| retMsg["message"] = "Values are missing!"; | ||
| response->setLength(); | ||
| request->send(response); | ||
| return; | ||
| } | ||
|
|
||
| auto checkHttpConfig = [&](JsonObject const& cfg) -> bool { | ||
|
|
||
| if (!cfg["url"].is<String>() | ||
| || (!cfg["url"].as<String>().startsWith("http://") | ||
| && !cfg["url"].as<String>().startsWith("https://"))) { | ||
| retMsg["message"] = "URL must either start with http:// or https://!"; | ||
| response->setLength(); | ||
| request->send(response); | ||
| return false; | ||
| } | ||
|
|
||
| if ((cfg["auth_type"].as<uint8_t>() != HttpRequestConfig::Auth::None) | ||
| && (cfg["username"].as<String>().length() == 0 || cfg["password"].as<String>().length() == 0)) { | ||
| retMsg["message"] = "Username or password must not be empty!"; | ||
| response->setLength(); | ||
| request->send(response); | ||
| return false; | ||
| } | ||
|
|
||
| if (!cfg["timeout"].is<uint16_t>() | ||
| || cfg["timeout"].as<uint16_t>() <= 0) { | ||
| retMsg["message"] = "Timeout must be greater than 0 ms!"; | ||
| response->setLength(); | ||
| request->send(response); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| }; | ||
|
|
||
| if (static_cast<::PowerMeters::Provider::Type>(root["source"].as<uint8_t>()) == ::PowerMeters::Provider::Type::HTTP_JSON) { | ||
| JsonObject httpJson = root["http_json"]; | ||
| JsonArray valueConfigs = httpJson["values"]; | ||
| for (uint8_t i = 0; i < valueConfigs.size(); i++) { | ||
| JsonObject valueConfig = valueConfigs[i].as<JsonObject>(); | ||
|
|
||
| if (i > 0 && !valueConfig["enabled"].as<bool>()) { | ||
| continue; | ||
| } | ||
|
|
||
| if (i == 0 || httpJson["individual_requests"].as<bool>()) { | ||
| if (!checkHttpConfig(valueConfig["http_request"].as<JsonObject>())) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (!valueConfig["json_path"].is<String>() | ||
| || valueConfig["json_path"].as<String>().length() == 0) { | ||
| retMsg["message"] = "Json path must not be empty!"; | ||
| response->setLength(); | ||
| request->send(response); | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (static_cast<::PowerMeters::Provider::Type>(root["source"].as<uint8_t>()) == ::PowerMeters::Provider::Type::HTTP_SML) { | ||
| JsonObject httpSml = root["http_sml"]; | ||
| if (!checkHttpConfig(httpSml["http_request"].as<JsonObject>())) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (static_cast<::PowerMeters::Provider::Type>(root["source"].as<uint8_t>()) == ::PowerMeters::Provider::Type::MODBUS_UDP_VICTRON) { | ||
| JsonObject udpVictron = root["udp_victron"]; | ||
| if (!udpVictron["ip_address"].is<String>() | ||
| || udpVictron["ip_address"].as<String>().length() == 0) { | ||
| retMsg["message"] = "IP address must not be empty!"; | ||
| response->setLength(); | ||
| request->send(response); | ||
| return; | ||
| } | ||
|
|
||
| if (!udpVictron["polling_interval_ms"].is<uint32_t>() | ||
| || udpVictron["polling_interval_ms"].as<uint32_t>() <= 0) { | ||
| retMsg["message"] = "Polling interval must be greater than 0 ms!"; | ||
| response->setLength(); | ||
| request->send(response); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| { | ||
| auto guard = Configuration.getWriteGuard(); | ||
| auto& config = guard.getConfig(); | ||
| config.InverterMeter.Enabled = root["enabled"].as<bool>(); | ||
| config.InverterMeter.Source = root["source"].as<uint8_t>(); | ||
|
|
||
| // Interpret the string as a hex value and convert it to uint64_t | ||
| const uint64_t serial = strtoll(root["serial"].as<String>().c_str(), NULL, 16); | ||
| config.InverterMeter.Serial = serial; | ||
|
|
||
| Configuration.deserializePowerMeterMqttConfig(root["mqtt"].as<JsonObject>(), | ||
| config.InverterMeter.Mqtt); | ||
|
|
||
| Configuration.deserializePowerMeterSerialSdmConfig(root["serial_sdm"].as<JsonObject>(), | ||
| config.InverterMeter.SerialSdm); | ||
|
|
||
| Configuration.deserializePowerMeterHttpJsonConfig(root["http_json"].as<JsonObject>(), | ||
| config.InverterMeter.HttpJson); | ||
|
|
||
| Configuration.deserializePowerMeterHttpSmlConfig(root["http_sml"].as<JsonObject>(), | ||
| config.InverterMeter.HttpSml); | ||
|
|
||
| Configuration.deserializePowerMeterUdpVictronConfig(root["udp_victron"].as<JsonObject>(), | ||
| config.InverterMeter.UdpVictron); | ||
| } | ||
|
|
||
| WebApi.writeConfig(retMsg); | ||
|
|
||
| WebApi.sendJsonResponse(request, response, __FUNCTION__, __LINE__); | ||
|
|
||
| InverterMeter.updateSettings(); | ||
|
|
||
| } |
There was a problem hiding this comment.
Config POST flow mirrors PowerMeter; consider requiring serial explicitly
The admin POST handler is structured well: credentials check, basic enabled/source validation, per‑provider HTTP/UDP validation, guarded config update via getWriteGuard(), then a writeConfig + InverterMeter.updateSettings().
One small robustness gap: you don’t currently validate the presence/format of root["serial"] before calling strtoll(...). If the client omits serial or sends an empty string, this silently writes 0 into config.InverterMeter.Serial, which is also your “unused” sentinel elsewhere. Requiring a non‑empty hex string and failing fast would make misconfigurations more obvious to users.
A minimal adjustment would be to extend the initial Values are missing! check (or add a dedicated block) to assert root["serial"].is<String>() && root["serial"].as<String>().length() > 0 and return a clear error message if it’s not met.
🤖 Prompt for AI Agents
In src/WebApi_invertermeter.cpp around lines 81 to 216, add validation for
root["serial"] before it is parsed: ensure root["serial"].is<String>() &&
root["serial"].as<String>().length() > 0 (optionally also validate it contains
only hex digits) and if not set retMsg["message"] = "Serial must be a non-empty
hex string!", call response->setLength(); request->send(response); and return;
then only call strtoll(...) after this check; place the check either by
extending the initial "Values are missing!" validation or as a small dedicated
block immediately before the strtoll call so an omitted/empty serial no longer
silently writes 0.
| <template v-if="inverterMeterConfigList.enabled"> | ||
| <!-- yarn linter wants us to not combine v-if with v-for, so we need to wrap the CardElements //--> | ||
| <template v-if="inverterMeterConfigList.source === 0"> | ||
| <CardElement | ||
| v-for="(mqtt, index) in inverterMeterConfigList.mqtt.values" | ||
| v-bind:key="index" | ||
| :text="$t('powermeteradmin.MqttValue', { valueNumber: index + 1 })" | ||
| textVariant="text-bg-primary" | ||
| add-space | ||
| > | ||
| <InputElement | ||
| :label="$t('powermeteradmin.MqttTopic')" | ||
| v-model="mqtt.topic" | ||
| type="text" | ||
| maxlength="256" | ||
| wide | ||
| /> | ||
|
|
||
| <InputElement | ||
| :label="$t('powermeteradmin.mqttJsonPath')" | ||
| v-model="mqtt.json_path" | ||
| type="text" | ||
| maxlength="256" | ||
| :tooltip="$t('powermeteradmin.valueJsonPathDescription')" | ||
| wide | ||
| /> | ||
|
|
||
| <div class="row mb-3"> | ||
| <label for="mqtt_power_unit" class="col-sm-4 col-form-label"> | ||
| {{ $t('powermeteradmin.valueUnit') }} | ||
| </label> | ||
| <div class="col-sm-8"> | ||
| <select id="mqtt_power_unit" class="form-select" v-model="mqtt.unit"> | ||
| <option v-for="u in unitTypeList" :key="u.key" :value="u.key"> | ||
| {{ u.value }} | ||
| </option> | ||
| </select> | ||
| </div> | ||
| </div> | ||
|
|
||
| <InputElement | ||
| :label="$t('powermeteradmin.valueSignInverted')" | ||
| v-model="mqtt.sign_inverted" | ||
| :tooltip="$t('powermeteradmin.valueSignInvertedHint')" | ||
| type="checkbox" | ||
| wide | ||
| /> | ||
| </CardElement> | ||
| </template> | ||
|
|
||
| <CardElement | ||
| v-if="inverterMeterConfigList.source === 1 || inverterMeterConfigList.source === 2" | ||
| :text="$t('powermeteradmin.SDM')" | ||
| textVariant="text-bg-primary" | ||
| add-space | ||
| > | ||
| <InputElement | ||
| :label="$t('powermeteradmin.pollingInterval')" | ||
| v-model="inverterMeterConfigList.serial_sdm.polling_interval" | ||
| type="number" | ||
| min="1" | ||
| max="15" | ||
| :postfix="$t('powermeteradmin.seconds')" | ||
| wide | ||
| /> | ||
|
|
||
| <InputElement | ||
| :label="$t('powermeteradmin.sdmaddress')" | ||
| v-model="inverterMeterConfigList.serial_sdm.address" | ||
| type="number" | ||
| wide | ||
| /> | ||
| </CardElement> | ||
|
|
||
| <template v-if="inverterMeterConfigList.source === 3"> | ||
| <CardElement :text="$t('invertermeteradmin.HTTP')" textVariant="text-bg-primary" add-space> | ||
| <InputElement | ||
| :label="$t('powermeteradmin.httpIndividualRequests')" | ||
| v-model="inverterMeterConfigList.http_json.individual_requests" | ||
| type="checkbox" | ||
| wide | ||
| /> | ||
|
|
||
| <InputElement | ||
| :label="$t('powermeteradmin.pollingInterval')" | ||
| v-model="inverterMeterConfigList.http_json.polling_interval" | ||
| type="number" | ||
| min="1" | ||
| max="15" | ||
| :postfix="$t('powermeteradmin.seconds')" | ||
| wide | ||
| /> | ||
| </CardElement> | ||
|
|
||
| <CardElement | ||
| v-for="(httpJson, index) in inverterMeterConfigList.http_json.values" | ||
| :key="index" | ||
| :text="$t('powermeteradmin.httpValue', { valueNumber: index + 1 })" | ||
| textVariant="text-bg-primary" | ||
| add-space | ||
| > | ||
| <InputElement | ||
| v-if="index > 0" | ||
| :label="$t('powermeteradmin.httpEnabled')" | ||
| v-model="httpJson.enabled" | ||
| type="checkbox" | ||
| wide | ||
| /> | ||
|
|
||
| <template v-if="httpJson.enabled || index == 0"> | ||
| <HttpRequestSettings | ||
| v-model="httpJson.http_request" | ||
| v-if="index == 0 || inverterMeterConfigList.http_json.individual_requests" | ||
| /> | ||
|
|
||
| <InputElement | ||
| :label="$t('powermeteradmin.valueJsonPath')" | ||
| v-model="httpJson.json_path" | ||
| type="text" | ||
| maxlength="256" | ||
| :tooltip="$t('powermeteradmin.valueJsonPathDescription')" | ||
| wide | ||
| /> | ||
|
|
||
| <div class="row mb-3"> | ||
| <label for="power_unit" class="col-sm-4 col-form-label"> | ||
| {{ $t('powermeteradmin.valueUnit') }} | ||
| </label> | ||
| <div class="col-sm-8"> | ||
| <select id="power_unit" class="form-select" v-model="httpJson.unit"> | ||
| <option v-for="u in unitTypeList" :key="u.key" :value="u.key"> | ||
| {{ u.value }} | ||
| </option> | ||
| </select> | ||
| </div> | ||
| </div> | ||
|
|
||
| <InputElement | ||
| :label="$t('powermeteradmin.valueSignInverted')" | ||
| v-model="httpJson.sign_inverted" | ||
| :tooltip="$t('powermeteradmin.valueSignInvertedHint')" | ||
| type="checkbox" | ||
| wide | ||
| /> | ||
| </template> | ||
| </CardElement> | ||
|
|
||
| <CardElement | ||
| :text="$t('powermeteradmin.testHttpJsonHeader')" | ||
| textVariant="text-bg-primary" | ||
| add-space | ||
| > | ||
| <div class="text-center mb-3"> | ||
| <button type="button" class="btn btn-primary" @click="testHttpJsonRequest()"> | ||
| {{ $t('powermeteradmin.testHttpJsonRequest') }} | ||
| </button> | ||
| </div> | ||
|
|
||
| <BootstrapAlert | ||
| v-model="testHttpJsonRequestAlert.show" | ||
| dismissible | ||
| :variant="testHttpJsonRequestAlert.type" | ||
| > | ||
| {{ testHttpJsonRequestAlert.message }} | ||
| </BootstrapAlert> | ||
| </CardElement> | ||
| </template> | ||
|
|
||
| <template v-if="inverterMeterConfigList.source === 6"> | ||
| <CardElement :text="$t('powermeteradmin.HTTP_SML')" textVariant="text-bg-primary" add-space> | ||
| <InputElement | ||
| :label="$t('powermeteradmin.pollingInterval')" | ||
| v-model="inverterMeterConfigList.http_sml.polling_interval" | ||
| type="number" | ||
| min="1" | ||
| max="15" | ||
| :postfix="$t('powermeteradmin.seconds')" | ||
| wide | ||
| /> | ||
|
|
||
| <HttpRequestSettings v-model="inverterMeterConfigList.http_sml.http_request" /> | ||
| </CardElement> | ||
|
|
||
| <CardElement | ||
| :text="$t('powermeteradmin.testHttpSmlHeader')" | ||
| textVariant="text-bg-primary" | ||
| add-space | ||
| > | ||
| <div class="text-center mb-3"> | ||
| <button type="button" class="btn btn-primary" @click="testHttpSmlRequest()"> | ||
| {{ $t('powermeteradmin.testHttpSmlRequest') }} | ||
| </button> | ||
| </div> | ||
|
|
||
| <BootstrapAlert | ||
| v-model="testHttpSmlRequestAlert.show" | ||
| dismissible | ||
| :variant="testHttpSmlRequestAlert.type" | ||
| > | ||
| {{ testHttpSmlRequestAlert.message }} | ||
| </BootstrapAlert> | ||
| </CardElement> | ||
| </template> | ||
|
|
||
| <template v-if="inverterMeterConfigList.source === 7"> | ||
| <CardElement :text="$t('powermeteradmin.UDP_VICTRON')" textVariant="text-bg-primary" add-space> | ||
| <InputElement | ||
| :label="$t('powermeteradmin.pollingInterval')" | ||
| v-model="udpVictronPollIntervalSeconds" | ||
| type="number" | ||
| min="0.5" | ||
| max="15.0" | ||
| step="0.1" | ||
| :postfix="$t('powermeteradmin.seconds')" | ||
| wide | ||
| /> | ||
|
|
||
| <InputElement | ||
| :label="$t('powermeteradmin.ipAddress')" | ||
| v-model="inverterMeterConfigList.udp_victron.ip_address" | ||
| type="text" | ||
| maxlength="15" | ||
| wide | ||
| /> | ||
| </CardElement> | ||
| </template> | ||
| </template> |
There was a problem hiding this comment.
Missing configuration UI for SML (source 4) and SMAHM2 (source 5).
The source dropdown includes "SML" (key 4) and "SMAHM2" (key 5) options, but there are no corresponding <template v-if="..."> sections to configure them. Users selecting these sources will see no configuration fields. Either add UI sections for these sources or remove them from the dropdown if they're not supported for InverterMeter.
🤖 Prompt for AI Agents
In webapp/src/views/InverterMeterAdminView.vue around lines 49 to 275, the
component renders configuration blocks for many inverterMeterConfigList.source
values but is missing UI sections for source 4 (SML) and source 5 (SMAHM2); add
corresponding <template v-if="inverterMeterConfigList.source === 4"> and
<template v-if="inverterMeterConfigList.source === 5"> blocks (similar to the
existing HTTP/HTTP_SML/SDM/UDP sections) that render CardElement wrappers and
InputElement fields bound to the appropriate inverterMeterConfigList.sml and
inverterMeterConfigList.sma_hm2 properties (e.g., polling interval,
request/settings, address/IP, enable flags, JSON path/unit/sign_inverted as
applicable), or alternatively remove/disable the 4 and 5 options from the source
dropdown and related translations if those sources aren’t supported for
InverterMeter; ensure v-model bindings match the data model and reuse existing
components (HttpRequestSettings, InputElement, unitTypeList) for consistency.
| <div class="row mb-3"> | ||
| <label for="mqtt_power_unit" class="col-sm-4 col-form-label"> | ||
| {{ $t('powermeteradmin.valueUnit') }} | ||
| </label> | ||
| <div class="col-sm-8"> | ||
| <select id="mqtt_power_unit" class="form-select" v-model="mqtt.unit"> | ||
| <option v-for="u in unitTypeList" :key="u.key" :value="u.key"> | ||
| {{ u.value }} | ||
| </option> | ||
| </select> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Duplicate id attributes in v-for loop.
Line 81 uses a static id="mqtt_power_unit" inside a v-for loop, creating duplicate IDs when multiple MQTT values exist. This violates HTML standards and can cause accessibility issues with label associations.
Proposed fix
- <label for="mqtt_power_unit" class="col-sm-4 col-form-label">
+ <label :for="`mqtt_power_unit_${index}`" class="col-sm-4 col-form-label">
{{ $t('powermeteradmin.valueUnit') }}
</label>
<div class="col-sm-8">
- <select id="mqtt_power_unit" class="form-select" v-model="mqtt.unit">
+ <select :id="`mqtt_power_unit_${index}`" class="form-select" v-model="mqtt.unit">🤖 Prompt for AI Agents
In webapp/src/views/InverterMeterAdminView.vue around lines 76 to 87, the select
and its label use a static id/for ("mqtt_power_unit") inside a v-for, producing
duplicate IDs; change both to bound attributes that include a unique suffix
(e.g., the loop index or the mqtt object id) so each instance has a unique id
and the label's for points to that id (use :id and :for with a template string
like `mqtt_power_unit_${index}` or `mqtt_power_unit_${mqtt.id}`).
| <div class="row mb-3"> | ||
| <label for="power_unit" class="col-sm-4 col-form-label"> | ||
| {{ $t('powermeteradmin.valueUnit') }} | ||
| </label> | ||
| <div class="col-sm-8"> | ||
| <select id="power_unit" class="form-select" v-model="httpJson.unit"> | ||
| <option v-for="u in unitTypeList" :key="u.key" :value="u.key"> | ||
| {{ u.value }} | ||
| </option> | ||
| </select> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Duplicate id attributes in v-for loop.
Same issue as the MQTT section - line 178 uses static id="power_unit" inside a v-for loop for HTTP JSON values.
Proposed fix
- <label for="power_unit" class="col-sm-4 col-form-label">
+ <label :for="`power_unit_${index}`" class="col-sm-4 col-form-label">
{{ $t('powermeteradmin.valueUnit') }}
</label>
<div class="col-sm-8">
- <select id="power_unit" class="form-select" v-model="httpJson.unit">
+ <select :id="`power_unit_${index}`" class="form-select" v-model="httpJson.unit">Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In webapp/src/views/InverterMeterAdminView.vue around lines 173 to 184, the
select element uses a static id="power_unit" inside a v-for context which
creates duplicate id attributes; update the template to generate unique ids (for
example bind :id to include the loop index or the unit key) and update the
corresponding label's for attribute to match that bound id so each select/label
pair has a unique identifier.
| computed: { | ||
| udpVictronPollIntervalSeconds: { | ||
| get(): number { | ||
| return this.inverterMeterConfigList.udp_victron.polling_interval_ms / 1000; | ||
| }, | ||
| set(value: number) { | ||
| this.inverterMeterConfigList.udp_victron.polling_interval_ms = value * 1000; | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Potential runtime error accessing nested property on empty object.
inverterMeterConfigList is initialized as {} (line 305). The computed property at line 342 accesses this.inverterMeterConfigList.udp_victron.polling_interval_ms, which will throw a TypeError before data is loaded. Add a guard or provide a default structure.
Proposed fix
computed: {
udpVictronPollIntervalSeconds: {
get(): number {
+ if (!this.inverterMeterConfigList.udp_victron) return 0;
return this.inverterMeterConfigList.udp_victron.polling_interval_ms / 1000;
},
set(value: number) {
+ if (!this.inverterMeterConfigList.udp_victron) return;
this.inverterMeterConfigList.udp_victron.polling_interval_ms = value * 1000;
},
},
},📝 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.
| computed: { | |
| udpVictronPollIntervalSeconds: { | |
| get(): number { | |
| return this.inverterMeterConfigList.udp_victron.polling_interval_ms / 1000; | |
| }, | |
| set(value: number) { | |
| this.inverterMeterConfigList.udp_victron.polling_interval_ms = value * 1000; | |
| }, | |
| }, | |
| }, | |
| computed: { | |
| udpVictronPollIntervalSeconds: { | |
| get(): number { | |
| if (!this.inverterMeterConfigList.udp_victron) return 0; | |
| return this.inverterMeterConfigList.udp_victron.polling_interval_ms / 1000; | |
| }, | |
| set(value: number) { | |
| if (!this.inverterMeterConfigList.udp_victron) return; | |
| this.inverterMeterConfigList.udp_victron.polling_interval_ms = value * 1000; | |
| }, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In webapp/src/views/InverterMeterAdminView.vue around lines 339-348, the
computed getter/setter directly access
this.inverterMeterConfigList.udp_victron.polling_interval_ms which will throw if
inverterMeterConfigList or udp_victron is not set; update the getter to return a
safe default (e.g., 0) when the nested path is missing using a guard/optional
chaining and nullish coalescing, and update the setter to ensure
inverterMeterConfigList.udp_victron exists (create an object) before assigning
polling_interval_ms = value * 1000 so the ms↔s conversion remains correct
without runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (8)
webapp/src/views/InverterMeterAdminView.vue (4)
76-87: Duplicateidattributes in v-for loop.The
id="mqtt_power_unit"is static inside av-forloop, creating duplicate IDs when multiple MQTT values exist. This violates HTML standards and causes accessibility issues.Proposed fix
- <label for="mqtt_power_unit" class="col-sm-4 col-form-label"> + <label :for="`mqtt_power_unit_${index}`" class="col-sm-4 col-form-label"> {{ $t('powermeteradmin.valueUnit') }} </label> <div class="col-sm-8"> - <select id="mqtt_power_unit" class="form-select" v-model="mqtt.unit"> + <select :id="`mqtt_power_unit_${index}`" class="form-select" v-model="mqtt.unit">
173-184: Duplicateidattributes in v-for loop.Same issue as the MQTT section -
id="power_unit"is static inside av-forloop for HTTP JSON values.Proposed fix
- <label for="power_unit" class="col-sm-4 col-form-label"> + <label :for="`power_unit_${index}`" class="col-sm-4 col-form-label"> {{ $t('powermeteradmin.valueUnit') }} </label> <div class="col-sm-8"> - <select id="power_unit" class="form-select" v-model="httpJson.unit"> + <select :id="`power_unit_${index}`" class="form-select" v-model="httpJson.unit">
339-348: Potential runtime error accessing nested property on empty object.
inverterMeterConfigListis initialized as{}(line 305). The computed property accessesthis.inverterMeterConfigList.udp_victron.polling_interval_ms, which will throw a TypeError before data is loaded.Proposed fix
computed: { udpVictronPollIntervalSeconds: { get(): number { + if (!this.inverterMeterConfigList.udp_victron) return 0; return this.inverterMeterConfigList.udp_victron.polling_interval_ms / 1000; }, set(value: number) { + if (!this.inverterMeterConfigList.udp_victron) return; this.inverterMeterConfigList.udp_victron.polling_interval_ms = value * 1000; }, }, },
306-314: Missing configuration UI for SML (source 4) and SMAHM2 (source 5).The source dropdown includes "SML" (key 4) and "SMAHM2" (key 5) options, but there are no corresponding
<template v-if="...">sections to configure them. Users selecting these sources will see no configuration fields.include/invertermeter/Controller.h (1)
15-19: Missing<optional>header forstd::optionalreturn type.Line 35 uses
std::optional<float>but the<optional>header is not included. This may compile on some implementations due to transitive includes but is not portable.Proposed fix
#include <powermeter/Provider.h> #include <TaskSchedulerDeclarations.h> #include <memory> #include <mutex> +#include <optional>src/WebApi_invertermeter.cpp (1)
189-192: Missing validation forserialfield before parsing.The code calls
strtoll(root["serial"].as<String>().c_str(), NULL, 16)without validating thatserialexists or is a valid non-empty hex string. If omitted or empty, this silently writes0which is the "unused" sentinel value.Proposed fix
Add validation before parsing the serial:
+ if (!root["serial"].is<String>() || root["serial"].as<String>().length() == 0) { + retMsg["message"] = "Serial must be a non-empty hex string!"; + response->setLength(); + request->send(response); + return; + } + // Interpret the string as a hex value and convert it to uint64_t const uint64_t serial = strtoll(root["serial"].as<String>().c_str(), NULL, 16);src/PowerLimiter.cpp (2)
306-318: Unreachable fallback code remains.Lines 314-317 are unreachable because the if-else at lines 307-312 exhaustively covers both
dayvalues (true and false). This was previously flagged - please remove the dead code.
1015-1027: Data race on_fromStartbetween serialization methods andloop().Both
serializeRTDanddeserializeRTDaccess_fromStartwithout synchronization whileloop()reads and writes the same variable viagetBatteryState(). The comment acknowledges this but dismissesstd::atomic<bool>due to "overhead" concerns.For
boolon ESP32/Xtensa,std::atomic<bool>has negligible overhead (typically compiles to the same instructions) and provides correctness guarantees. Consider usingstd::atomic<bool>for_fromStartto eliminate the data race without measurable performance impact.
🧹 Nitpick comments (12)
include/InverterATF.h (2)
34-64: Clarify locking documentation on public methods.The locking policy header (lines 4-11) states that public mutating methods take locks internally, but the comments on individual public methods like
activateATF,deactivateATF,setATFData, etc. say "use a unique lock when calling this method" or "use a shared lock when calling this method". This is contradictory and may confuse maintainers.If the methods do acquire locks internally (as per the header policy), the per-method comments should be removed or rephrased (e.g., "acquires an exclusive lock internally").
72-72: Consider documenting why size must remain fixed.The emphatic comment "never NEVER change the size!" is important but doesn't explain why. Since ATF data is persisted to runtime storage, changing the array size would corrupt deserialized data. A brief clarifying comment would help future maintainers.
🔎 Suggested clarification
- static constexpr uint8_t _size = 101; // Fixed size of the ATF data array, never NEVER change the size! + static constexpr uint8_t _size = 101; // Fixed size of the ATF data array (0-100%), persisted to storage; changing breaks deserializationsrc/PowerLimiterBatteryInverter.cpp (2)
2-2: Unused include.
LogHelper.his included but no logging calls are visible in this file. Consider removing it if not needed.🔎 Proposed fix
#include "PowerLimiterBatteryInverter.h" -#include "LogHelper.h"
52-52: Address the TODO comment.The TODO raises a valid point about whether setting the lower limit would be safer than returning 0 when standby is not allowed. Returning 0 means no reduction is applied, which may leave the inverter at its current limit. If this is intentional, consider documenting the rationale; otherwise, setting the lower limit as a fallback could provide more predictable behavior.
Would you like me to help analyze the implications of each approach or open an issue to track this?
webapp/src/views/PowerLimiterAdminView.vue (1)
161-168: ATF UI wiring is consistent; consider a safer and more focusedisATFFullimplementation
- The extended
AllowStandbycondition and the newUseATFcheckbox forpower_source == 0match the backend config and PR objectives.isATFFullcurrently iterates overthis.powerLimiterConfigList.inverterswithout null‑guarding and counts every inverter, governed or not.To make this more robust and aligned with the “only one ATF inverter” constraint, you could:
isATFFull(): boolean { const inverters = this.powerLimiterConfigList?.inverters || []; // Optionally restrict to governed battery-powered inverters if that matches backend limits: // const inverters = this.governedBatteryPoweredInverters; return inverters.some((inv: PowerLimiterInverterConfig) => inv.use_atf); }This avoids any risk if
invertersis temporarily undefined and makes the capacity rule explicit.Also applies to: 170-178, 542-552
src/PowerLimiterInverter.cpp (2)
316-320: External inverter meter override for output power is reasonable; consider an explicit castPreferring
_oInverterMeterPowerwhen present and falling back to inverterTYPE_AC/CH0/FLD_PACstats implements the intended override cleanly. For clarity and to make the narrowing explicit, you might cast the float touint16_t:if (_oInverterMeterPower.has_value()) { return static_cast<uint16_t>(_oInverterMeterPower.value()); } return _spInverter->Statistics()->getChannelFieldValue(TYPE_AC, CH0, FLD_PAC);
464-481:setCurrentOutputAcWattslogic looks sound; verify call-site threading modelThe function correctly:
- Rejects obviously invalid meter values (non-positive power, zero timestamp, missing stats, or ineligible inverter).
- Ensures the meter timestamp is not more than ~2s older than the latest stats (
statsMillis - timestamp > 2000path).- Clears
_oInverterMeterPowerwhen data is stale and only accepts fresh samples.Two follow-ups to consider:
- If power readings of exactly
0.0ffrom the external meter should be treated as valid (e.g., explicit standby confirmation), you may want to relaxpower <= 0.0fto< 0.0f.- If
setCurrentOutputAcWattscan be called from a different context thanupdate()/getCurrentOutputAcWatts, protect_oInverterMeterPowerwith the same mutex used for stats/limits to avoid data races.src/RuntimeData.cpp (1)
194-232:getWriteCountAndTimeStringis fine; minor clarity tweak optionalThe helper correctly formats
<count> / <dd>-<mon> <hh>:<mm>when_writeEpochis non-zero andgetLocalTimesucceeds, and otherwise uses" / no time". You might simplify the logic slightly by separating “time available” from the formatting, but the current implementation is clear and thread-safe thanks to the mutex.src/InverterATF.cpp (1)
44-49: Redundant null check afterstd::make_unique.
std::make_uniquethrowsstd::bad_allocon allocation failure; it never returnsnullptr. The check at line 46 is unreachable dead code.Proposed fix
// allocate memory for the ATF data array _realPower = std::make_unique<float[]>(_size); - if (_realPower == nullptr) { - DTU_LOGE("Unable to allocate memory for ATF data array"); - return false; - }If you want to handle allocation failure gracefully, use
std::make_uniquein a try-catch block or usestd::unique_ptrwithnew (std::nothrow)instead.src/invertermeter/Controller.cpp (1)
52-79: Missing default case in switch statement.If a new
PowerMeters::Provider::Typeis added but not handled here,_upProviderremains unset from the previousreset()call without any warning. Consider adding a default case to log an error or assert.Proposed fix
case PowerMeters::Provider::Type::MODBUS_UDP_VICTRON: _upProvider = std::make_unique<::PowerMeters::Modbus::Udp::Victron::Provider>(imcfg.UdpVictron); break; + default: + // Unknown provider type - log warning and leave provider null + break; }webapp/src/types/InverterMeterConfig.ts (1)
19-25: Naming inconsistency:PowerMeterHttpJsonValuevsInverterMeter*pattern.All other interfaces use the
InverterMeterprefix, but this one usesPowerMeter. Consider renaming toInverterMeterHttpJsonValuefor consistency.Proposed fix
-export interface PowerMeterHttpJsonValue { +export interface InverterMeterHttpJsonValue { http_request: HttpRequestConfig; enabled: boolean; json_path: string; unit: number; sign_inverted: boolean; }Also update the reference in
InverterMeterHttpJsonConfig:export interface InverterMeterHttpJsonConfig { polling_interval: number; individual_requests: boolean; - values: Array<PowerMeterHttpJsonValue>; + values: Array<InverterMeterHttpJsonValue>; }src/PowerLimiter.cpp (1)
1036-1074: Clean up commented-out code and consider early loop exit.
- Lines 1052 and 1054 contain commented-out code that should be removed.
- Line 1042: Using
continueaftercounter >= 1causes the loop to iterate through remaining inverters without doing anything. Considerbreakfor efficiency, or restructure if the intent is to deactivate ATF on non-selected inverters (which currently happens at lines 1068-1070).Proposed cleanup
if (upInv->isATFEnabled() && !upInv->isATFActive()) { auto maxPower = upInv->getInverterMaxPowerWatts(); - //if (maxPower == 0) { maxPower = upInv->getATFConfigPower(); } // todo: delete after testing if (maxPower == 0) { - //DTU_LOGW("cannot initialize ATF: inverter max power unknown"); allDone = false; continue; } if (!upInv->activateATF(maxPower)) { - //DTU_LOGW("cannot initialize ATF: activate data structure"); allDone = false; continue; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
include/Configuration.hinclude/InverterATF.hinclude/PowerLimiter.hinclude/PowerLimiterInverter.hinclude/RuntimeData.hinclude/WebApi.hinclude/WebApi_invertermeter.hinclude/invertermeter/Controller.hsrc/Configuration.cppsrc/Datastore.cppsrc/InverterATF.cppsrc/PowerLimiter.cppsrc/PowerLimiterBatteryInverter.cppsrc/PowerLimiterInverter.cppsrc/RuntimeData.cppsrc/WebApi.cppsrc/WebApi_firmware.cppsrc/WebApi_invertermeter.cppsrc/WebApi_maintenance.cppsrc/WebApi_sysstatus.cppsrc/WebApi_ws_live.cppsrc/invertermeter/Controller.cppsrc/main.cppwebapp/src/components/FirmwareInfo.vuewebapp/src/components/NavBar.vuewebapp/src/locales/de.jsonwebapp/src/locales/en.jsonwebapp/src/router/index.tswebapp/src/types/InverterMeterConfig.tswebapp/src/types/PowerLimiterConfig.tswebapp/src/types/SystemStatus.tswebapp/src/views/InverterMeterAdminView.vuewebapp/src/views/PowerLimiterAdminView.vue
🧰 Additional context used
📓 Path-based instructions (2)
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/types/SystemStatus.tswebapp/src/types/PowerLimiterConfig.tswebapp/src/components/NavBar.vuewebapp/src/router/index.tswebapp/src/components/FirmwareInfo.vuewebapp/src/views/PowerLimiterAdminView.vuewebapp/src/types/InverterMeterConfig.tswebapp/src/views/InverterMeterAdminView.vue
{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.cppsrc/WebApi_sysstatus.cppsrc/WebApi_ws_live.cppinclude/Configuration.hsrc/PowerLimiterInverter.cppsrc/Datastore.cppsrc/WebApi_firmware.cppsrc/WebApi_invertermeter.cppsrc/main.cppinclude/invertermeter/Controller.hsrc/invertermeter/Controller.cppsrc/WebApi_maintenance.cppsrc/PowerLimiterBatteryInverter.cppinclude/RuntimeData.hinclude/PowerLimiter.hinclude/WebApi.hinclude/WebApi_invertermeter.hsrc/Configuration.cppsrc/RuntimeData.cppsrc/PowerLimiter.cppsrc/InverterATF.cppinclude/InverterATF.hinclude/PowerLimiterInverter.h
🧠 Learnings (1)
📚 Learning: 2025-10-17T08:29:40.205Z
Learnt from: SW-Niko
Repo: hoylabs/OpenDTU-OnBattery PR: 2262
File: src/main.cpp:149-159
Timestamp: 2025-10-17T08:29:40.205Z
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/main.cppinclude/RuntimeData.hinclude/PowerLimiter.hsrc/RuntimeData.cppsrc/PowerLimiter.cpp
🧬 Code graph analysis (7)
src/PowerLimiterInverter.cpp (1)
include/PowerLimiterInverter.h (1)
getInverterMaxPowerWatts(35-74)
webapp/src/types/InverterMeterConfig.ts (1)
webapp/src/types/HttpRequestConfig.ts (1)
HttpRequestConfig(1-9)
src/WebApi_invertermeter.cpp (1)
src/WebApi.cpp (2)
init(18-53)init(18-18)
include/invertermeter/Controller.h (3)
src/invertermeter/Controller.cpp (16)
init(31-39)init(31-31)updateSettings(41-84)updateSettings(41-41)getPowerTotal(86-91)getPowerTotal(86-86)getLastUpdate(122-127)getLastUpdate(122-122)getPower(93-107)getPower(93-93)getTime(109-120)getTime(109-109)getRequestTime(136-148)getRequestTime(136-136)loop(150-157)loop(150-150)lib/Frozen/frozen/bits/pmh.h (2)
std(128-142)private(151-169)include/battery/zendure/Stats.h (1)
optional(65-144)
include/WebApi_invertermeter.h (2)
src/WebApi.cpp (2)
init(18-53)init(18-18)src/WebApi_invertermeter.cpp (12)
init(18-29)init(18-18)onStatus(32-68)onStatus(32-32)onAdminGet(71-78)onAdminGet(71-71)onAdminPost(81-216)onAdminPost(81-81)onTestHttpJsonRequest(219-260)onTestHttpJsonRequest(219-219)onTestHttpSmlRequest(263-295)onTestHttpSmlRequest(263-263)
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/PowerLimiterInverter.h (1)
include/InverterATF.h (1)
InverterATF(21-85)
🔇 Additional comments (35)
include/Configuration.h (2)
146-158: LGTM!The
UseATFfield addition toPOWERLIMITER_INVERTER_CONFIG_Tis correctly placed and follows the existing naming and type conventions in the struct.
441-451: LGTM!The
InverterMeterConfigstruct appropriately reuses existing PowerMeter sub-config types and adds theSerialfield to associate the meter with a specific inverter. This is consistent with the PR objective of allowing inverter meter configuration and assignment via WebUI.src/WebApi_ws_live.cpp (2)
17-17: LGTM!The include is necessary for accessing
PowerLimiter.getATFInverterPower().
276-281: LGTM!The ATF power integration correctly prioritizes ATF-calculated values when available while maintaining the linear fallback calculation. The optional handling with
has_value()andvalue()is correct.webapp/src/components/NavBar.vue (1)
89-93: LGTM!The new menu item follows the established pattern for navigation items, uses proper i18n, and is logically positioned in the settings dropdown near related power/meter settings.
src/WebApi_sysstatus.cpp (1)
17-17: LGTM!The runtime save count is appropriately added to system status, following the existing pattern of
cfgsavecount. This provides useful diagnostic information about runtime data persistence.Also applies to: 83-83
webapp/src/types/SystemStatus.ts (1)
34-34: LGTM!The
runtime_savecountfield is correctly typed asstringto match the backend's formatted string output, and is logically placed next tocfgsavecount.webapp/src/types/PowerLimiterConfig.ts (1)
32-32: LGTM!The
use_atffield correctly mirrors the backend'sUseATFconfiguration option and follows the snake_case naming convention used throughout this TypeScript interface.src/WebApi_maintenance.cpp (1)
47-50: LGTM!Persisting runtime data before an intentional reboot is good practice. The 60-minute threshold prevents excessive flash wear while ensuring recent data is saved. The implementation is safe:
RuntimeData.write()is a synchronous blocking operation that completes before returning, andRestartHelper.triggerRestart()schedules a restart via the task scheduler with approximately 1 second delay, providing ample time for the write to finish before the actual restart occurs.include/WebApi.h (1)
23-23: LGTM!The new
WebApi_invertermeter.hinclude andWebApiInverterMeterClassmember follow the established pattern and are consistently placed alongside related power meter modules.Also applies to: 78-78
src/WebApi_firmware.cpp (1)
51-54: LGTM!Persisting runtime data before an OTA-triggered reboot ensures no data loss. The 60-minute threshold prevents excessive flash writes while the placement after
request->send()ensures the response is sent before any blocking operation.webapp/src/locales/en.json (1)
15-15: LGTM!The new localization entries follow existing patterns and provide clear, descriptive text for the Inverter Meter settings, runtime save count display, and ATF configuration options.
Also applies to: 286-286, 678-684, 740-742
webapp/src/components/FirmwareInfo.vue (1)
86-89: LGTM!The new RuntimeSaveCount row correctly displays the pre-formatted string from
systemStatus.runtime_savecount. Since the backend provides this as a combined "count / time" string (viagetWriteCountAndTimeString()), direct rendering without numeric formatting is appropriate.webapp/src/router/index.ts (1)
15-15: LGTM!The new route for Inverter Meter Settings follows the established routing conventions and is logically placed after the Inverter Settings route.
Also applies to: 131-135
src/WebApi.cpp (1)
44-44: LGTM!The inverter meter Web API initialization is correctly placed in the initialization sequence, maintaining consistency with the header declaration order and logical grouping of related modules.
src/main.cpp (1)
32-32: LGTM!The initialization sequence correctly places:
InverterMeter.init()afterPowerMeter.init()and beforePowerLimiter.init()RuntimeData.init()andRuntimeData.read()after all other componentsThis aligns with the established requirement that RuntimeData must be initialized after the components it serves. Based on learnings, this ordering ensures RuntimeData acts as a persistence service accessed during runtime operations rather than initialization.
Also applies to: 40-40, 150-161
src/PowerLimiterBatteryInverter.cpp (1)
13-13: LGTM!The addition of
&& _config.AllowStandbycorrectly enforces the user's configuration setting. Previously, theallowStandbyruntime parameter alone controlled standby behavior, ignoring the per-inverterAllowStandbyconfig option. This fix ensures both conditions must be satisfied.Also applies to: 48-48, 60-60
src/Datastore.cpp (1)
8-8: InverterMeter fast-path for AC power looks correct; just confirm interaction with polling and controller thread-safetyUsing
InverterMeter.getPower(inv->serial())with anstd::optionaland falling back to per‑channelTYPE_ACstats preserves previous behavior when no meter is configured, and ensures we don’t double‑count. The digits minimum of 1 when the meter is used is a reasonable fallback. Please just verify that:
InverterMeter.getPower()is cheap/safe to call inside this locked loop, and- It’s intended that AC power is summed even if
inv->getEnablePolling()is false, as long as the external meter provides a value.Also applies to: 95-106
src/PowerLimiterInverter.cpp (1)
195-201: ATF-relative-limit integration inupdateLimitis consistent with existing flowSwitching
newRelativeLimitto usegetATFLimit(*_oTargetPowerLimitWatts)whenisATFActive()and falling back to the previous linear conversion keeps the rest of the update logic (deviation checks, logging, retries) intact. This is a clean, local change; just ensuregetATFLimit()gracefully handles edge cases (e.g., limits near 0/100%) so we never send obviously out-of-range percentages.src/Configuration.cpp (1)
215-230: PowerLimiteruse_atfand InverterMeter config round-trip look correct; watch defaults
- Adding
t["use_atf"] = s.UseATF;andinv.UseATF = s["use_atf"] | false;makes the new flag fully round-trippable with a sane default offalse.- The new
invertermeterblock mirrors the existingpowermeterhandling (enabled/source/serial + MQTT, SDM, HTTP JSON/SML, UDP Victron) and read/write paths are symmetric.Two small points to keep in mind:
- On
read(), if older config files don’t contain aninvertermetersection at all, thedeserializePowerMeter*Configcalls will see emptyJsonObjects and rely on their internal| defaultbehavior. That matches howpowermeteris already handled but is worth keeping in mind when debugging migrations.config.InverterMeter.Sourcedefaults toPOWERMETER_SOURCEwhen absent; verify that this default makes sense for inverter meters (instead of e.g. a dedicatedINVERTERMETER_SOURCE_DEFAULT), to avoid misconfiguration if the section is partially missing.Also applies to: 433-452, 638-679, 924-933
src/RuntimeData.cpp (4)
33-40: RuntimeData singleton and loop scheduling fit the intended usageCreating
RuntimeClass RuntimeData{RUNTIME_VERSION};and scheduling itsloop()every minute is consistent with the design of a background persistence service and with the initialization ordering described for this project (RuntimeData after the major components, then used at runtime). The loop simply reacts to_writeNow/_readNowflags and the daily trigger, so there’s no unexpected behavior here.Based on learnings, RuntimeData is being used as intended in the lifecycle.
Also applies to: 43-51
78-140: Write throttling and commit-after-success semantics are solidThe
write(freezeMinutes)implementation:
- Enforces a minimum interval between writes when
freezeMinutes > 0.- Uses a local
JsonDocument, fillsinfo.version/save_count/save_epoch, then delegates toPowerLimiter.serializeRTD()andserializeATFtoRTD()while holding the mutex.- Only updates
_writeVersion,_writeEpoch,_writeCountafter a successful file write.This is a good pattern for durability and protects against partial updates if a write fails. Just ensure all contributors to runtime data (currently only
PowerLimiter) follow the comment and use their own internal locking as needed when touching shared state during serialization.
148-188: Read path and START_UP vs ON_DEMAND split look correctThe
read(ReadMode mode)function:
- Attempts to open and deserialize
/runtime.jsonbut still initializes_writeVersion,_writeCount,_writeEpochfrom whatever is present (or defaults) even on error.- For
START_UP, callsPowerLimiter.deserializeRTD(doc["power_limiter"]);- For
ON_DEMAND, callsPowerLimiter.deserializeRTDtoATF(doc.as<JsonVariant>());This matches the intended two-phase initialization (configurable data at startup, ATF data on demand) and fails over to default values when the file is missing or invalid. That’s appropriate for runtime-only data.
238-255: Daily write trigger window logic behaves as intended
getWriteTrigger():
- Waits for valid local time.
- Returns
trueonce between 00:05 and 00:10 per day, using_lastTriggerto edge-detect the minute range.- Resets
_lastTriggeroutside the window.This is a straightforward and efficient way to guarantee at most one scheduled write per day in addition to explicit triggers.
include/PowerLimiter.h (2)
61-68: LGTM! Well-structured ATF and RTD API additions.The new public methods for serialization/deserialization and ATF functionality follow a consistent pattern and provide clear separation of concerns. The
constqualifiers and return types are appropriate.
87-90: Clean state machine design for battery discharge management.The
BatteryStateenum with clear state names (STOP, NO_DISCHARGE, DISCHARGE_ALLOWED, DISCHARGE_NIGHT) provides a well-defined state machine. The related tracking flags_fromStartand_oneStopPerNightDoneappropriately support the state transitions.include/invertermeter/Controller.h (1)
4-13: Excellent locking policy documentation.The detailed comments explaining the locking strategy (shared vs exclusive locks, public vs private method conventions, atomic flag usage) provide valuable guidance for maintainers and help prevent concurrency bugs.
src/WebApi_invertermeter.cpp (1)
219-260: LGTM! Test endpoint implementation is well-structured.The
onTestHttpJsonRequesthandler properly validates credentials, parses request data, creates a scoped provider viastd::make_unique, polls for data, and formats results appropriately. Error handling distinguishes between success and failure cases.include/RuntimeData.h (1)
10-46: Well-designed runtime data persistence class.The design demonstrates good practices:
- Deleted copy/move semantics prevent accidental duplication of the singleton-like resource
- Atomic flags for
_readNow/_writeNowenable lock-free signaling between threads- Mutable mutex correctly supports locking in const methods
- Version parameter enables future format migrations
src/InverterATF.cpp (2)
219-254: Thread-safe cache with double-checked locking pattern.The
getATFPowermethod correctly implements double-checked locking: first checking the cache under a shared lock, then upgrading to an exclusive lock and re-checking before computing. This optimizes for the common cache-hit case while maintaining thread safety.
394-399: VLA issue resolved with named constant.The buffer size is now defined using a properly named compile-time constant
kTableWidth, which satisfies cpplint requirements and ensures the array size is known at compile time.src/invertermeter/Controller.cpp (1)
31-39: LGTM! Clean initialization flow.The initialization properly sets up the task scheduler callback, configures infinite iterations, enables the task, and calls
updateSettings()to set up the initial provider based on configuration.src/PowerLimiter.cpp (3)
402-415: LGTM!The periodic ATF data update and reporting logic is clean and appropriately gated by the 60-second interval check.
676-682: LGTM!Early exit for battery stop state correctly puts all battery-powered inverters into standby before attempting limit calculations.
214-222: Distinguish between unconfigured and unavailable meter data to prevent silent 0.0f power usage.When
InverterMeter.getPower()returnsstd::nullopt, the code defaults to0.0fwithout distinguishing between two cases: (1) the meter is not configured for this specific inverter, or (2) the meter is configured but data is temporarily unavailable. Both conditions result in the same silent fallback to zero power. If the meter is configured but data is stale, the system should returnInverterPowerMeterPendingto wait for fresh data rather than proceeding with zero power, which could cause incorrect power calculations.
| #include "InverterATF.h" | ||
|
|
||
| class PowerLimiterInverter { | ||
| class PowerLimiterInverter : public InverterATF { | ||
| public: |
There was a problem hiding this comment.
Add a virtual destructor and clean up ATF helper surface
The new ATF integration and external meter plumbing are conceptually fine, but there are a couple of structural issues:
-
Non-virtual base destructor with polymorphic use
PowerLimiterInverteris used polymorphically (std::unique_ptr<PowerLimiterInverter>holding derived instances), but it has no virtual destructor. This is undefined behavior when deleting derived objects via a base pointer. Please add:class PowerLimiterInverter : public InverterATF { public: virtual ~PowerLimiterInverter() = default; // ... };
-
Inline ATF helpers couple the header tightly to implementation details
setATFData()calls intoInverterATF::setATFData(getCurrentOutputAcWatts(), _spInverter->SystemConfigPara()->getLimitPercent());directly from the header. That’s OK, but it makes every inclusion of this header depend on these internals; consider moving the definition to the.cppif compile times become an issue.getATFConfigPower()is marked “todo: delete after testing”. It’s better to remove or#ifdeftest-only APIs before merging to avoid public API creep.
-
External meter state
_oInverterMeterPowerasstd::optional<float>aligns with the new override semantics; just ensure any concurrent access is guarded consistently with how the rest of the class is used.
Also applies to: 78-99, 162-164
| uint32_t Controller::getRequestTime() const | ||
| { | ||
| std::lock_guard<std::mutex> l(_mutex); | ||
| if (_upProvider) { | ||
| if (Configuration.get().InverterMeter.Source == static_cast<uint32_t>(PowerMeters::Provider::Type::HTTP_JSON)) { | ||
| return Configuration.get().InverterMeter.HttpJson.PollingInterval + 200; | ||
| } | ||
| if (Configuration.get().InverterMeter.Source == static_cast<uint32_t>(PowerMeters::Provider::Type::HTTP_SML)) { | ||
| return Configuration.get().InverterMeter.HttpSml.PollingInterval + 200; | ||
| } | ||
| } | ||
| return 2000; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Controller.cpp file around the getRequestTime method
# Look at line 40 comment about milliseconds and lines 136-148
# First find the file
find . -name "Controller.cpp" -path "*/invertermeter/*" -type f
# Then examine line 40 and the getRequestTime method
if [ -f "src/invertermeter/Controller.cpp" ]; then
echo "=== Line 40 and surrounding context ==="
sed -n '35,45p' src/invertermeter/Controller.cpp
echo ""
echo "=== getRequestTime method (lines 136-148) ==="
sed -n '136,148p' src/invertermeter/Controller.cpp
fiRepository: hoylabs/OpenDTU-OnBattery
Length of output: 976
🏁 Script executed:
#!/bin/bash
# Search for PollingInterval usage and definition
echo "=== PollingInterval usages in C++ files ==="
rg -n "PollingInterval" --type cpp -C 2 | head -100
echo ""
echo "=== Check InverterMeterAdminView.vue for polling interval config ==="
find . -name "InverterMeterAdminView.vue" -type f -exec head -300 {} \;Repository: hoylabs/OpenDTU-OnBattery
Length of output: 20497
🏁 Script executed:
#!/bin/bash
# Check how PollingInterval is defined in configuration structs
rg -n "PollingInterval" --type cpp -A 1 -B 1 | grep -E "(^\s*(uint|int|double|float|long).*PollingInterval|PollingInterval.*=|struct|class)" | head -50Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1113
🏁 Script executed:
#!/bin/bash
# Find POWERMETER_POLLING_INTERVAL definition
echo "=== POWERMETER_POLLING_INTERVAL definition ==="
rg -n "POWERMETER_POLLING_INTERVAL\s*=" --type cpp -B 1 -A 1
echo ""
echo "=== Look at Configuration.h struct definitions ==="
rg -n "POWERMETER_HTTP_JSON_CONFIG_T|POWERMETER_HTTP_SML_CONFIG_T" include/Configuration.h -A 8
echo ""
echo "=== How getRequestTime() is used ==="
rg -n "getRequestTime" --type cpp -B 2 -A 2Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Check header file for more details on the structs and polling interval fields
echo "=== Check full struct definitions in Configuration.h ==="
sed -n '120,160p' include/Configuration.h
echo ""
echo "=== Find all polling interval related constants ==="
rg -n "POLLING_INTERVAL" include/ --type cpp | head -30Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1683
🏁 Script executed:
#!/bin/bash
# Double-check how the return value of getRequestTime is used to confirm it expects milliseconds
echo "=== Search for calls/usage of getRequestTime() result ==="
rg -n "getRequestTime()" --type cpp -B 2 -A 3 | head -50Repository: hoylabs/OpenDTU-OnBattery
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Look at what happens with the 2000ms default return value to understand intent
echo "=== Look at full getRequestTime method context and surrounding code ==="
sed -n '130,155p' src/invertermeter/Controller.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 967
Multiply PollingInterval by 1000 on lines 141 and 144.
The method returns milliseconds (per the comment at line 40), but PollingInterval is in seconds (UI range 1-15 seconds, default 10). Lines 141 and 144 must convert to milliseconds:
return Configuration.get().InverterMeter.HttpJson.PollingInterval * 1000 + 200;
return Configuration.get().InverterMeter.HttpSml.PollingInterval * 1000 + 200;This is consistent with how POWERMETER_UDP_VICTRON_CONFIG_T explicitly names its field PollingIntervalMs and Configuration.cpp line 574 multiplies POWERMETER_POLLING_INTERVAL by 1000 for that type.
🤖 Prompt for AI Agents
In src/invertermeter/Controller.cpp around lines 136 to 148, the function
getRequestTime returns milliseconds but uses PollingInterval (which is in
seconds) directly; change the two return expressions to multiply PollingInterval
by 1000 before adding 200 so the value is in milliseconds (e.g., use
PollingInterval * 1000 + 200 for both HTTP_JSON and HTTP_SML branches). Ensure
locking/logic remains unchanged and only adjust the arithmetic to convert
seconds to ms.
| std::optional<uint16_t> PowerLimiterClass::getATFInverterPower(uint64_t inverterSerial, float limit) const { | ||
| for (auto& upInv : _inverters) { | ||
| if (!upInv->isATFActive()) { continue; } | ||
| if (upInv->getSerial() != inverterSerial) { continue; } | ||
|
|
||
| // found matching inverter, get power from its ATF | ||
| auto power = upInv->getATFPower(limit); | ||
| if (power != 0) { return power; } | ||
| } | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Zero power treated as invalid may cause incorrect behavior.
Line 1124 returns std::nullopt when power == 0, but 0W could be a legitimate ATF calculation (e.g., when the inverter should produce no power). This conflates "no data available" with "calculated power is zero."
Consider returning the value unconditionally once a matching active inverter is found:
Proposed fix
// found matching inverter, get power from its ATF
auto power = upInv->getATFPower(limit);
- if (power != 0) { return power; }
+ return power;
}
return std::nullopt;📝 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.
| std::optional<uint16_t> PowerLimiterClass::getATFInverterPower(uint64_t inverterSerial, float limit) const { | |
| for (auto& upInv : _inverters) { | |
| if (!upInv->isATFActive()) { continue; } | |
| if (upInv->getSerial() != inverterSerial) { continue; } | |
| // found matching inverter, get power from its ATF | |
| auto power = upInv->getATFPower(limit); | |
| if (power != 0) { return power; } | |
| } | |
| return std::nullopt; | |
| } | |
| std::optional<uint16_t> PowerLimiterClass::getATFInverterPower(uint64_t inverterSerial, float limit) const { | |
| for (auto& upInv : _inverters) { | |
| if (!upInv->isATFActive()) { continue; } | |
| if (upInv->getSerial() != inverterSerial) { continue; } | |
| // found matching inverter, get power from its ATF | |
| auto power = upInv->getATFPower(limit); | |
| return power; | |
| } | |
| return std::nullopt; | |
| } |
🤖 Prompt for AI Agents
In src/PowerLimiter.cpp around lines 1117-1127 the code treats a computed ATF
power of 0 as "no data" and returns std::nullopt; instead, once an active
inverter with matching serial is found return its computed power unconditionally
(even if zero). Remove the "if (power != 0)" branch and return the calculated
value wrapped in std::optional<uint16_t> (perform the appropriate
conversion/cast from float to uint16_t), leaving std::nullopt only for the case
where no matching active inverter is found.
| uint16_t PowerLimiterInverter::getCurrentLimitWatts() const | ||
| { | ||
| auto currentLimitPercent = _spInverter->SystemConfigPara()->getLimitPercent(); | ||
| return static_cast<uint16_t>(currentLimitPercent * getInverterMaxPowerWatts() / 100); | ||
|
|
||
| // get the power from ATF or from the linear calculation | ||
| if (isATFActive()) { | ||
| return getATFPower(currentLimitPercent); // ATF | ||
| } else { | ||
| return static_cast<uint16_t>(currentLimitPercent * getInverterMaxPowerWatts() / 100); // linear | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PowerLimiterInverter.cpp" -o -name "PowerLimiterInverter.h" | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 132
🏁 Script executed:
cat -n src/PowerLimiterInverter.cpp | sed -n '354,363p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 549
🏁 Script executed:
# Search for getATFPower implementation
grep -n "getATFPower" src/PowerLimiterInverter.cpp | head -5Repository: hoylabs/OpenDTU-OnBattery
Length of output: 128
🏁 Script executed:
# Search for getEligibility implementation
grep -n "getEligibility" src/PowerLimiterInverter.cpp | head -5Repository: hoylabs/OpenDTU-OnBattery
Length of output: 278
🏁 Script executed:
# Search for isATFActive implementation
grep -n "isATFActive" src/PowerLimiterInverter.cpp | head -5Repository: hoylabs/OpenDTU-OnBattery
Length of output: 130
🏁 Script executed:
# Get getEligibility() implementation
cat -n src/PowerLimiterInverter.cpp | sed -n '48,80p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1849
🏁 Script executed:
# Search for getATFPower implementation
grep -n "uint16_t.*getATFPower" src/PowerLimiterInverter.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 51
🏁 Script executed:
# Get more context on the getEligibility logic - check for CurrentLimitUnknown
grep -n "CurrentLimitUnknown\|Eligibility::" src/PowerLimiterInverter.cpp | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1074
🏁 Script executed:
# Find where Eligibility enum is defined
grep -rn "enum.*Eligibility\|CurrentLimitUnknown" include/PowerLimiterInverter.hRepository: hoylabs/OpenDTU-OnBattery
Length of output: 145
🏁 Script executed:
# Search for getATFPower implementation
grep -n "getATFPower" src/PowerLimiterInverter.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 128
🏁 Script executed:
# Search in header file
grep -n "getATFPower" include/PowerLimiterInverter.hRepository: hoylabs/OpenDTU-OnBattery
Length of output: 51
🏁 Script executed:
# Look for ATF-related implementations
grep -rn "getATFPower" --include="*.cpp" --include="*.h"Repository: hoylabs/OpenDTU-OnBattery
Length of output: 396
🏁 Script executed:
# Get InverterATF::getATFPower implementation
cat -n src/InverterATF.cpp | sed -n '215,240p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1204
🏁 Script executed:
# Get more context
cat -n src/InverterATF.cpp | sed -n '210,260p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 2116
🏁 Script executed:
# Search for _useATF usage and initialization
grep -n "_useATF" src/InverterATF.cpp | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 377
🏁 Script executed:
# Search for ATF learning phase
grep -n "learning\|_useATF\|ATF" src/InverterATF.cpp | head -30Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1637
🏁 Script executed:
# Check header for _useATF member
grep -n "_useATF" include/InverterATF.hRepository: hoylabs/OpenDTU-OnBattery
Length of output: 256
🏁 Script executed:
# Find when activateATF is called
grep -rn "activateATF" --include="*.cpp" --include="*.h"Repository: hoylabs/OpenDTU-OnBattery
Length of output: 479
🏁 Script executed:
# Check PowerLimiterInverter constructor and initialization
grep -n "isATFActive\|activateATF" src/PowerLimiterInverter.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 130
🏁 Script executed:
# Search for ATF enable/initialization logic
grep -n "isATFActive\|ATF" include/PowerLimiterInverter.h | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 762
🏁 Script executed:
# Get context around the activateATF call in PowerLimiter.cpp
cat -n src/PowerLimiter.cpp | sed -n '1050,1075p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1097
🏁 Script executed:
# Check if there's any scenario where getATFPower returns 0 after activation
# Look at setATFData implementation
cat -n src/InverterATF.cpp | sed -n '91,120p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1697
🏁 Script executed:
# Check how the _realPower array is initialized in activateATF
cat -n src/InverterATF.cpp | sed -n '37,73p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1411
🏁 Script executed:
# Check cpplint configuration
find . -name ".cpplint*" -o -name "cpplint*" | grep -v node_modulesRepository: hoylabs/OpenDTU-OnBattery
Length of output: 100
🏁 Script executed:
# Check if there's a setup for cpplint or clang-format configuration
cat .cpplint 2>/dev/null || echo "No .cpplint file found"Repository: hoylabs/OpenDTU-OnBattery
Length of output: 91
🏁 Script executed:
# Verify the actual behavior: with ATF initialized and valid limit, what gets returned?
# Let's trace the _realPower initialization more carefully
cat -n src/InverterATF.cpp | sed -n '60,65p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 327
🏁 Script executed:
# Check the cpplint workflow to understand what rules are enforced
cat .github/workflows/cpplint.ymlRepository: hoylabs/OpenDTU-OnBattery
Length of output: 1035
🏁 Script executed:
# Let's check if there are any recent changes or TODOs in PowerLimiterInverter.cpp
grep -n "TODO\|FIXME\|BUG\|XXX" src/PowerLimiterInverter.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 51
🏁 Script executed:
# Verify that _realPower is properly initialized before being used
cat -n src/InverterATF.cpp | sed -n '44,70p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1093
🏁 Script executed:
# Search for getLimitPercent to understand the range of values
grep -rn "getLimitPercent" --include="*.cpp" --include="*.h" | grep -E "getLimitPercent\(\)|return.*Limit"Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1779
🏁 Script executed:
# Check what values are valid for currentLimitPercent
grep -rn "LimitPercent\|currentLimitPercent" src/PowerLimiterInverter.cpp | head -10Repository: hoylabs/OpenDTU-OnBattery
Length of output: 415
🏁 Script executed:
# Get the getLimitPercent implementation
cat -n lib/Hoymiles/src/parser/SystemConfigParaParser.cpp | sed -n '48,70p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 969
🏁 Script executed:
# Search for where SystemConfigPara limit is set/initialized
grep -rn "SystemConfigPara\|setLimit" lib/Hoymiles/src/parser/SystemConfigParaParser.cpp | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1218
🏁 Script executed:
# Check if there's any handling for very low or zero limits
grep -n "limit.*0\|zero.*limit" src/InverterATF.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 918
🏁 Script executed:
# Check the _size constant to understand the array dimensions
grep -n "_size\|size.*=" include/InverterATF.h | head -10Repository: hoylabs/OpenDTU-OnBattery
Length of output: 193
🏁 Script executed:
# Let's verify the actual initialization values for a small inverter
cat -n include/InverterATF.h | sed -n '65,75p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 804
🏁 Script executed:
# Check if there's any minimum value enforcement or special handling during learning
grep -n "State::DEFAULT_INIT\|State::RTD_INIT" src/InverterATF.cppRepository: hoylabs/OpenDTU-OnBattery
Length of output: 368
🏁 Script executed:
# Look at what happens during cache initialization and low limits
cat -n src/InverterATF.cpp | sed -n '215,254p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 1788
🏁 Script executed:
# Check if there's any state-based logic that prevents returning 0 during learning
grep -n "_state\|DEFAULT_INIT" src/InverterATF.cpp | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 396
🏁 Script executed:
# Verify that _cache is initialized to {0, 0.0f}
grep -n "_cache.*=" include/InverterATF.hRepository: hoylabs/OpenDTU-OnBattery
Length of output: 191
🏁 Script executed:
# Check how currentLimitPercent flows through the system
cat -n src/PowerLimiterInverter.cpp | sed -n '354,365p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 566
🏁 Script executed:
# Final confirmation: can getLimitPercent return exactly 0?
cat -n lib/Hoymiles/src/parser/SystemConfigParaParser.cpp | sed -n '48,58p'Repository: hoylabs/OpenDTU-OnBattery
Length of output: 554
Ensure getATFPower() preserves non-zero results for all valid limits during ATF learning
The ATF path routing in getCurrentLimitWatts() is correct, but getATFPower() can return 0 for limits between 0% and ~1% due to rounding during interpolation (lines 240-253 in InverterATF.cpp). Since getEligibility() treats a return value of 0 as CurrentLimitUnknown (line 69 in PowerLimiterInverter.cpp), this edge case could keep ATF-enabled inverters ineligible longer than necessary during the initial learning phase. Consider ensuring that a valid, non-zero limit percentage always results in a non-zero power output, even during the DEFAULT_INIT state.
🤖 Prompt for AI Agents
In src/PowerLimiterInverter.cpp around lines 354 to 363, ensure ATF path does
not yield zero watts for tiny but valid non-zero limits during ATF learning:
after calling getATFPower(currentLimitPercent) (or by changing getATFPower
itself), if currentLimitPercent > 0 and the ATF result is 0, return a minimal
non-zero power (e.g., 1 watt or a small floor based on inverter max power) so
that getEligibility() does not treat the value as CurrentLimitUnknown; implement
the floor in the ATF branch only (or inside getATFPower) and ensure it respects
inverter max power and DEFAULT_INIT behavior.
| "DeviceManager": "Hardware", | ||
| "SolarChargerSettings": "Solarladeregler", | ||
| "PowerMeterSettings": "Stromzähler", | ||
| "InverterMeterSettings": "Wechselrichter-Stromzähler", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "de.json" --type fRepository: hoylabs/OpenDTU-OnBattery
Length of output: 95
🏁 Script executed:
# Find the German locale file
find . -name "de.json" -type f | head -5Repository: hoylabs/OpenDTU-OnBattery
Length of output: 97
🏁 Script executed:
# Also search for the implementation code related to ATF
rg "isATFFull|use_atf" -t js -t ts -t json --max-count 20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 483
🏁 Script executed:
# Search for powerlimiteradmin in the codebase
rg "powerlimiteradmin" -A 3 -B 3Repository: hoylabs/OpenDTU-OnBattery
Length of output: 29246
🏁 Script executed:
# Read the German JSON file - check specific lines
sed -n '270,290p' webapp/src/locales/de.jsonRepository: hoylabs/OpenDTU-OnBattery
Length of output: 1306
🏁 Script executed:
# Also check lines around 15, and the ATF-related section
sed -n '10,20p' webapp/src/locales/de.json
sed -n '675,710p' webapp/src/locales/de.jsonRepository: hoylabs/OpenDTU-OnBattery
Length of output: 3084
🏁 Script executed:
# Find the isATFFull method implementation
sed -n '/isATFFull/,+10p' webapp/src/views/PowerLimiterAdminView.vueRepository: hoylabs/OpenDTU-OnBattery
Length of output: 1010
🏁 Script executed:
# Search for UseATFHint in de.json
rg "UseATFHint" webapp/src/locales/de.json -A 1 -B 1Repository: hoylabs/OpenDTU-OnBattery
Length of output: 706
🏁 Script executed:
# Also check English locale for comparison
rg "UseATFHint" webapp/src/locales/en.json -A 1 -B 1Repository: hoylabs/OpenDTU-OnBattery
Length of output: 663
🏁 Script executed:
# Get line numbers for context
rg "UseATFHint|UseATF" webapp/src/locales/de.json --line-numberRepository: hoylabs/OpenDTU-OnBattery
Length of output: 483
Update ATF capacity text from 2 to 1 inverter in both German and English locales
The German locale at line 741 states: "Diese Option kann für maximal 2 Inverter aktiviert werden." However, the implementation in PowerLimiterAdminView.vue confirms ATF is limited to a single inverter—the isATFFull() method sets the flag true as soon as any one inverter has use_atf enabled. Update the German text to "maximal 1 Wechselrichter" and the English equivalent to "maximum of one inverter" to match the actual implementation limit.
🤖 Prompt for AI Agents
In webapp/src/locales/de.json around lines 15 (and specifically the entry that
reads "Diese Option kann für maximal 2 Inverter aktiviert werden." near line
~741), update the German text to read "Diese Option kann für maximal 1
Wechselrichter aktiviert werden."; also update the English locale in
webapp/src/locales/en.json to change "This option can be enabled for a maximum
of 2 inverters." (or similar phrasing) to "This option can be enabled for a
maximum of one inverter." so both locale strings match the actual ATF
implementation limit of a single inverter.
| export interface InverterMeterConfig { | ||
| enabled: boolean; | ||
| source: number; | ||
| serial: string; | ||
| interval: number; | ||
| mqtt: InverterMeterMqttConfig; | ||
| serial_sdm: InverterMeterSerialSdmConfig; | ||
| http_json: InverterMeterHttpJsonConfig; | ||
| http_sml: InverterMeterHttpSmlConfig; | ||
| udp_victron: InverterMeterUdpVictronConfig; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if 'interval' is serialized in the backend for InverterMeter
echo "=== Searching for 'interval' in WebApi_invertermeter.cpp ==="
rg -n '"interval"' src/WebApi_invertermeter.cpp 2>/dev/null || echo "No matches found"
echo ""
echo "=== Searching for 'Interval' in relevant backend files ==="
rg -n 'Interval' src/WebApi_invertermeter.cpp include/Configuration.h 2>/dev/null | grep -i invertermeter || echo "No matches found"
echo ""
echo "=== Broader search for interval in invertermeter files ==="
rg -n 'interval' src/ include/ 2>/dev/null | grep -i invertermeter || echo "No matches found"Repository: hoylabs/OpenDTU-OnBattery
Length of output: 599
🏁 Script executed:
#!/bin/bash
# Find and examine the onStatus handler in WebApi_invertermeter.cpp
echo "=== Looking for onStatus function ==="
rg -n 'onStatus|onInverterMeterStatus' src/WebApi_invertermeter.cpp -A 30 | head -100
echo ""
echo "=== Checking what fields are serialized in the response ==="
rg -n 'root\[|insert|add' src/WebApi_invertermeter.cpp | grep -E '(enabled|source|serial|mqtt|http|udp|interval)' | head -20Repository: hoylabs/OpenDTU-OnBattery
Length of output: 4692
Remove the unused interval field from the InverterMeterConfig interface.
The interval field is not serialized by the backend's onStatus handler in WebApi_invertermeter.cpp. The handler only returns: enabled, source, serial, mqtt, serial_sdm, http_json, http_sml, and udp_victron. Keeping this field will create a mismatch between the TypeScript types and the actual API response.
🤖 Prompt for AI Agents
In webapp/src/types/InverterMeterConfig.ts around lines 43 to 53, the interface
includes an unused field `interval` that the backend's onStatus handler does not
return; remove the `interval: number;` property from the InverterMeterConfig
interface so the TypeScript type matches the actual API response, and scan for
any local usages or tests referencing .interval to update or remove them
accordingly.
2 participants
This PR is specifically for HM-xxx inverters powered by a 24VDC battery.
See also #1601
Snapshot with an inverter (ATF and inverter meter active)
(1) Measurement from the inverter meter
(2) ATF calculated values (limit and power)
(3) Values measured by the inverter
Limitations:
ATF is only available for battery-powered inverters.
Currently, only one inverter can be selected for ATF/inverter meters.
Note:
This PR is based on PRs: #2355, #2262 and #2340