Feature: DPL: make standby configurable for battery-powered inverters#2355
Feature: DPL: make standby configurable for battery-powered inverters#2355AndreasBoehm merged 1 commit intohoylabs:developmentfrom
Conversation
Build ArtifactsFirmware built from this pull request's code:
Notice
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 23 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughUpdate enforces immediate standby for battery-powered inverters when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/PowerLimiterBatteryInverter.cpp (1)
52-52: Consider the TODO: should the lower limit be set as a safety measure?The TODO raises a valid safety question: when standby is not allowed and the inverter is already at or below the lower power limit, should the code explicitly set the lower limit again to ensure the inverter stays at that boundary?
Current behavior: returns 0 (no action taken)
Alternative: explicitly callsetAcOutput(_config.LowerPowerLimit)before returning 0The alternative would be more defensive and ensure the inverter is explicitly commanded to the lower limit even if it should already be there. This could help recover from unexpected states.
Would you like me to propose a specific implementation for this safety improvement?
src/PowerLimiter.cpp (1)
240-305: Add unit tests to verify battery state machine transitions and edge cases.Verification confirms no existing tests cover the battery state logic (
BatteryStateenum used only ininclude/PowerLimiter.hwith no corresponding test cases). Given the complexity of the state machine—particularly the nested conditionals (lines 283-299), the_oneStopPerNightDoneflag preventing oscillation (lines 267, 287, 295, 298), and the unreachable fallback code (line 302)—adding comprehensive unit tests is essential to verify all state transitions and prevent regressions.The implementation appears sound for the documented scenarios, but test coverage will provide confidence that edge cases are correctly handled and prevent future breakage when state logic is modified.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
include/PowerLimiter.h(2 hunks)include/RuntimeData.h(1 hunks)src/PowerLimiter.cpp(9 hunks)src/PowerLimiterBatteryInverter.cpp(3 hunks)src/RuntimeData.cpp(1 hunks)src/WebApi_firmware.cpp(2 hunks)src/WebApi_maintenance.cpp(2 hunks)src/WebApi_sysstatus.cpp(2 hunks)src/main.cpp(2 hunks)webapp/src/components/FirmwareInfo.vue(1 hunks)webapp/src/locales/de.json(2 hunks)webapp/src/locales/en.json(2 hunks)webapp/src/types/SystemStatus.ts(1 hunks)webapp/src/views/PowerLimiterAdminView.vue(1 hunks)
🧰 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/components/FirmwareInfo.vuewebapp/src/types/SystemStatus.tswebapp/src/views/PowerLimiterAdminView.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_firmware.cppsrc/main.cppsrc/PowerLimiterBatteryInverter.cppinclude/PowerLimiter.hsrc/WebApi_maintenance.cppsrc/WebApi_sysstatus.cppinclude/RuntimeData.hsrc/RuntimeData.cppsrc/PowerLimiter.cpp
🧠 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/main.cppinclude/PowerLimiter.hsrc/WebApi_maintenance.cppinclude/RuntimeData.hsrc/RuntimeData.cppsrc/PowerLimiter.cpp
🧬 Code graph analysis (3)
src/PowerLimiterBatteryInverter.cpp (4)
src/PowerLimiterInverter.cpp (4)
getCurrentOutputAcWatts(310-313)getCurrentOutputAcWatts(310-310)getCurrentLimitWatts(347-351)getCurrentLimitWatts(347-347)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)
include/PowerLimiter.h (1)
src/PowerLimiter.cpp (4)
serializeRTD(987-992)serializeRTD(987-987)deserializeRTD(994-999)deserializeRTD(994-994)
include/RuntimeData.h (1)
src/RuntimeData.cpp (16)
init(42-49)init(42-42)read(137-172)read(137-137)write(70-131)write(70-70)getWriteCount(178-182)getWriteCount(178-178)getWriteEpochTime(188-192)getWriteEpochTime(188-188)getWriteCountAndTimeString(200-216)getWriteCountAndTimeString(200-200)loop(55-63)loop(55-55)getWriteTrigger(222-239)getWriteTrigger(222-222)
🪛 Clang (14.0.6)
include/RuntimeData.h
[error] 4-4: 'ArduinoJson.h' file not found
(clang-diagnostic-error)
src/RuntimeData.cpp
[error] 20-20: 'Utils.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (26)
include/PowerLimiter.h (2)
60-61: LGTM! Runtime data serialization hooks properly exposed.The public serialization methods integrate cleanly with the RuntimeData persistence system introduced in this PR.
79-83: LGTM! State machine approach improves clarity.The refactoring from boolean flags to a BatteryState enum with explicit states (STOP, NO_DISCHARGE, DISCHARGE_ALLOWED, DISCHARGE_NIGHT) improves code readability and maintainability. The persistence of only
_fromStartvia RTD serialization is appropriate for maintaining state across restarts.include/RuntimeData.h (1)
1-43: LGTM! Well-designed runtime data persistence class.The RuntimeClass design demonstrates good practices:
- Non-copyable via deleted constructors/assignment operators
- Thread-safe shared data access via mutex
- Atomic flags for status tracking
- Write throttling to prevent excessive disk I/O
- Daily automatic persistence trigger
Based on learnings
webapp/src/components/FirmwareInfo.vue (1)
86-89: LGTM! Runtime save count properly displayed.The new row correctly displays the runtime save count from the system status, consistent with the pattern used for ConfigSaveCount above.
src/WebApi_firmware.cpp (1)
13-13: LGTM! Runtime data persisted before firmware-induced restart.Appropriate to persist runtime data before the restart triggered by firmware updates, with a sensible 60-minute freeze window to prevent excessive writes.
Also applies to: 51-53
webapp/src/locales/de.json (1)
285-285: LGTM! German translation added.The translation for RuntimeSaveCount is appropriately added and maintains consistency with the English counterpart.
src/WebApi_sysstatus.cpp (1)
17-17: LGTM! Runtime save count exposed in system status.The new runtime_savecount field is appropriately placed alongside cfgsavecount and uses the formatted string method to provide both count and timestamp information.
Also applies to: 83-83
webapp/src/views/PowerLimiterAdminView.vue (1)
164-164: LGTM! Allow Standby option extended to battery-powered inverters.This change implements the core PR objective by making the "Allow Standby" option visible for both battery-powered (power_source == 0) and smart buffer (power_source == 2) inverters. Previously, this option was only available for smart buffer inverters.
src/main.cpp (1)
39-39: LGTM! RuntimeData initialization order is correct.RuntimeData is properly initialized after all other components (SolarCharger, PowerMeter, PowerLimiter, GridCharger, and Battery) as required. The added section comments improve code clarity. This aligns with the learning that RuntimeData acts as a persistence service accessed during runtime operations, not during component initialization.
Based on learnings
Also applies to: 149-159
webapp/src/types/SystemStatus.ts (1)
34-34: LGTM!The
runtime_savecountfield addition is correctly typed asstringto match the formatted output fromRuntimeData.getWriteCountAndTimeString()(which combines count and time). The placement betweencfgsavecountanduptimeis logical.webapp/src/locales/en.json (2)
285-285: LGTM!The translation string clearly describes the runtime data save count field with its time component.
732-732: LGTM!The updated hint text provides clearer guidance by explicitly stating that the inverter will be set to the minimum power limit when standby is not allowed and the calculated power is below the minimum. This improves user understanding of the feature behavior.
src/WebApi_maintenance.cpp (1)
11-11: LGTM!The runtime data persistence before reboot is correctly implemented with a 60-minute freeze interval to avoid excessive writes. The comment clearly explains the intent, and the operation sequence (send response → save data → reboot) is appropriate.
Based on learnings
Also applies to: 47-49
src/PowerLimiterBatteryInverter.cpp (1)
13-13: LGTM! Standby logic correctly gated by configuration.The implementation properly checks both
allowStandby(dynamic permission based on power calculation) AND_config.AllowStandby(user configuration) before allowing the inverter to enter standby. This ensures the new "Allow Standby" option is respected for battery-powered inverters.The three locations where this check occurs are:
- Line 13:
getMaxReductionWatts- determines if full output can be reduced (standby)- Line 48:
applyReduction- first standby entry point when already at/below lower limit- Line 60:
applyReduction- second standby entry point when reduction exceeds available rangeAlso applies to: 48-51, 60-63
src/RuntimeData.cpp (7)
32-36: LGTM! Well-defined constants and singleton.The constants
RUNTIME_FILENAMEandRUNTIME_VERSIONare appropriately scoped and named. The singleton pattern withRuntimeDatais consistent with other components in the codebase.
42-63: LGTM! Clean initialization and loop implementation.The minute-based task scheduling is appropriate for the use case, and the loop correctly handles both on-demand writes (
_writeNow) and daily scheduled writes viagetWriteTrigger().
70-131: LGTM! Robust write implementation with proper safeguards.The write method demonstrates excellent defensive programming:
- Thread safety via mutex guard (line 88)
- Time availability check before writing (line 84)
- Freeze interval enforcement (lines 91-93) to prevent excessive writes
- JSON allocation verification (lines 109-111)
- Atomic state update only after successful write (lines 124-126)
- Comprehensive error handling with the
cleanExitlambdaThe integration with PowerLimiter via
serializeRTD(line 105) is clean and extensible.Based on learnings
137-172: LGTM! Read method with appropriate fallback behavior.The read implementation correctly:
- Tolerates missing or corrupted files by using default values (line 142 comment)
- Protects shared state with mutex (lines 154-158)
- Deserializes PowerLimiter RTD data (line 161)
- Logs success/failure appropriately (lines 165-168)
The graceful degradation approach (returning false but continuing with defaults) is appropriate for runtime data that should not block system startup.
178-216: LGTM! Accessor methods are thread-safe and well-formatted.All accessor methods properly protect reads with mutex guards. The
getWriteCountAndTimeString()method's formatting with fallback to "no time" (lines 208-212) provides a user-friendly display even when time synchronization hasn't occurred yet.
222-239: LGTM! Daily trigger with oscillation prevention.The
getWriteTrigger()implementation correctly:
- Returns true once per day during the 00:05–00:10 window (line 230)
- Uses
_lastTriggerflag to prevent multiple triggers within the window (lines 231-234)- Resets the flag outside the window (lines 235-237)
- Guards shared state with mutex (line 229)
This prevents oscillation and ensures exactly one daily write as intended.
20-20: Note: Static analysis warning is a false positive.The static analysis tool reports that
Utils.his not found (line 20), but this is expected to be a false positive in the build environment. The file is part of the existing codebase and is used correctly here (e.g.,Utils::getEpoch,Utils::skipBom,Utils::checkJsonAlloc).src/PowerLimiter.cpp (5)
338-339: LGTM! State is properly computed and cached per DPL loop iteration.The battery state is calculated once per loop iteration and stored in
_batteryState, then used consistently throughout the loop in multiple decision points (lines 554, 648-650, 750, 835). This ensures consistent behavior within each iteration.
648-654: LGTM! Battery-powered inverters correctly stopped when battery is below threshold.This new logic properly handles the case where battery-powered inverters need to enter standby because the battery is below the stop threshold, regardless of power demand. The early return (line 653) prevents further processing for these inverters in this loop iteration.
The log message at line 652 clearly indicates why the inverters are being stopped.
750-753: LGTM! DC power bus usage correctly blocked when battery is in STOP state.This check ensures that when the battery is below the stop threshold, no power is drawn from the DC power bus for battery-powered inverters. The log message clearly documents the reason for blocking.
835-835: LGTM! Battery discharge limit correctly returns 0 for STOP and NO_DISCHARGE states.When the battery state is STOP (below stop threshold) or NO_DISCHARGE (solar passthrough only), the discharge limit is correctly set to 0 to prevent battery discharge.
987-999: LGTM! RTD serialization approach is appropriate.The
serializeRTDanddeserializeRTDmethods only persist_fromStart(which direction the system entered the stop-start zone), not the full battery state. This is the correct approach because:
- The full state (
_batteryState) is recalculated each loop iteration based on current conditions_fromStartis a persistent flag that affects state transitions and needs to survive restarts- The deserialization provides a sensible default (
false) if the value is missingNote on thread safety (lines 989-990, 996-997): The comments acknowledge the lack of mutex protection. Since
_fromStartis a boolean and the PowerLimiter runs on a single task, this is acceptable. However, if concurrent access becomes possible in the future, consider usingstd::atomic<bool>as the comment suggests.Based on learnings
|
@SW-Niko Can you rebase and reduce this PR to only contain the addition of standby settings for battery inverters? Thank you |
AndreasBoehm
changed the title
|
@AndreasBoehm I had been using the standby option since last year without any problems but just tested with one inverter. |
AndreasBoehm
force-pushed
the
DPLAllowStandby
branch
from
d6dea13 to
23390ee
Compare
- WebApp, add option standby - PowerLimiter, add standby on/off - PowerLimiter, handle battery state OFF - Improve standby tooltip
AndreasBoehm
force-pushed
the
DPLAllowStandby
branch
from
23390ee to
9d28d11
Compare
2 participants
This PR makes the "Allow Standby" option, already available for smart battery-powered inverters, available for battery-powered inverters as well. More precise... make the deactivation of "Allow Standby" configurable.
"Allow Standby" is enabled by default. After a software update, the inverters' behavior will not initially change.
Only actively modifying the DPL configuration can alter the inverters' behavior.
This option is particularly useful for problems like "124 Switched off by remote control." Further information can be found here: #2321
Disabling "Allow Standby" is not recommended, if the minimum inverter output is higher than the minimum power consumption.
For systems with multiple inverters, you should also consider whether it is wise to disable the option for all inverters.
Important:
There are other events that can put battery inverters into standby mode, such as:
These standby events are not affected or modified by this option!