DPL: reduce power based on limit when ac ouput is higher than limit #2272
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a power reduction calculation issue in Dynamic Power Limiter (DPL) where the system needs to reduce power based on the current limit rather than the actual output when AC output exceeds the set limit.
- Introduces baseline calculation logic to use the current limit when output exceeds it
- Applies the same fix to both solar inverter and smart buffer inverter implementations
- Ensures power reduction calculations work correctly when inverters are operating above their configured limits
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/PowerLimiterSolarInverter.cpp | Adds baseline calculation using current limit when AC output exceeds limit |
| src/PowerLimiterSmartBufferInverter.cpp | Applies same baseline calculation fix and refactors variable usage |
Build ArtifactsFirmware built from this pull request's code:
Notice
|
|
@lmcmike Probier bitte mal wie die Test-Version aus dem Kommentar bei dir läuft. Wenn ich mich nicht Irre sollte dein Problem damit erledigt sein. |
|
@AndreasBoehm Danke, habe die Test-Version gedownloaded und werde sie ausprobieren sobald sich hier bei mir mal wieder die Sonne zeigt. Ich berichte anschließend. |
|
@AndreasBoehm |
|
Ich brauche ein Log der Situation bitte. |
|
Wenn ich den entsprechenden Augenblick erwische mache ich natürlich ein Log. |
|
So anbei ein Log. Ich hoffe dass ich den richtigen Moment erwischt habe. |
|
Im Log sieht alles gut aus, ist aber auch eine reguläre Situation und nicht wie du sie Ursprünglich beschrieben hast. Aktuelles Limit 121 W |
|
Nur noch. mal zum Verständnis: Die Anpassung kann nicht verhindern das die Situation eintritt das dein WR mehr produziert als das geforderte Limit. Was die Anpassung macht ist diese Situation beim nächsten DPL Regellauf feststellen und das neue Limit auf eine andere Art zu berechnen um das Problem auszugleichen. Protokollierst du Limit und Ausgangsleistung irgendwo? Home Assistant? Dann könntest du dort mal schauen ob das Problem nun weniger häufig auftritt bzw schnell wieder ausgeregelt wird. |
|
Hab ich doch nicht den richtigen Augeblick erwischt 👎 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughIntroduces baseline-aware reduction in applyReduction for SmartBuffer and Solar inverters. Baseline equals min(currentOutputAcWatts, currentLimitWatts). Reduction and output updates use baseline instead of raw output. Standby path now returns min(reduction, currentOutputAcWatts). No public interfaces changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Controller
participant INV as Inverter
participant PL as applyReduction()
C->>INV: Request limit update
INV->>PL: applyReduction(currentOutputAcWatts, currentLimitWatts, LowerPowerLimit)
note over PL: Compute baseline = min(currentOutputAcWatts, currentLimitWatts)
alt Standby allowed and triggered
PL-->>INV: return min(reduction, currentOutputAcWatts)
note right of INV: Skip further adjustments
else Reduction path
PL->>PL: reduction <= (baseline - LowerPowerLimit)
PL->>INV: setAcOutput(baseline - reduction)
INV-->>C: Limit applied using baseline
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/PowerLimiterSmartBufferInverter.cpp (1)
74-82: Baseline logic mirrors SolarInverter implementation.The baseline selection and reduction application correctly handle the case when output exceeds the limit, consistent with the fix in
PowerLimiterSolarInverter.cpp.Optional: Consider extracting baseline logic into a shared helper.
The baseline calculation (lines 76-79 here and lines 115-119 in
PowerLimiterSolarInverter.cpp) is duplicated across both files. If these classes share a common base or if this pattern is used elsewhere, extracting it into a helper method could reduce duplication:uint16_t getBaselineForReduction(uint16_t currentOutputAcWatts, uint16_t currentLimitWatts) { // when the output is higher than the limit, we must use the limit as the baseline return (currentOutputAcWatts > currentLimitWatts) ? currentLimitWatts : currentOutputAcWatts; }This is a minor refinement and can be deferred if you prefer to keep the logic explicit in each class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/PowerLimiterSmartBufferInverter.cpp(2 hunks)src/PowerLimiterSolarInverter.cpp(1 hunks)
🔇 Additional comments (3)
src/PowerLimiterSolarInverter.cpp (2)
112-119: LGTM! Baseline logic correctly addresses the issue.The baseline selection properly handles the case when
currentOutputAcWatts > currentLimitWattsby using the limit as the baseline. This ensures that when the inverter is producing more than the configured limit, the reduction is calculated from the limit rather than the actual output, preventing the DPL from inadvertently requesting a higher limit.The local variable
currentLimitWattsalso eliminates redundantgetCurrentLimitWatts()calls, improving both clarity and efficiency.
120-122: Reduction calculation correctly uses baseline.The updated condition
(baseline - _config.LowerPowerLimit) >= reductionand subsequentsetAcOutput(baseline - reduction)properly apply the reduction against the baseline, ensuring consistent behavior whether the output is above or below the limit.src/PowerLimiterSmartBufferInverter.cpp (1)
63-65: Guard condition correctly uses both limit and output.The updated
lowcalculation usingstd::min(currentLimitWatts, currentOutputAcWatts)properly checks whether either the limit or the output is at or below the lower power limit, ensuring the inverter cannot be reduced below that threshold.The local
currentLimitWattsvariable also avoids repeated function calls.
| uint16_t baseline = currentOutputAcWatts; | ||
|
|
||
| // when the output is higher than the limit, we must use the limit as the baseline | ||
| if (currentOutputAcWatts > currentLimitWatts) { | ||
| baseline = currentLimitWatts; | ||
| } | ||
|
|
||
| if ((baseline - _config.LowerPowerLimit) >= reduction) { | ||
| setAcOutput(baseline - reduction); | ||
| return reduction; |
There was a problem hiding this comment.
Incorrect return value when output exceeds limit.
Same issue as in PowerLimiterSolarInverter::applyReduction: when currentOutputAcWatts > currentLimitWatts, line 83 returns reduction instead of the actual reduction in output.
The actual reduction in output is:
currentOutputAcWatts - (baseline - reduction)
This inconsistency with lines 88 and 92 (which correctly return actual reduction from currentOutputAcWatts) causes the DPL to receive incorrect reduction values and make suboptimal power distribution decisions.
Apply this diff to fix the return value:
if ((baseline - _config.LowerPowerLimit) >= reduction) {
setAcOutput(baseline - reduction);
- return reduction;
+ return currentOutputAcWatts - (baseline - reduction);
}📝 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.
| uint16_t baseline = currentOutputAcWatts; | |
| // when the output is higher than the limit, we must use the limit as the baseline | |
| if (currentOutputAcWatts > currentLimitWatts) { | |
| baseline = currentLimitWatts; | |
| } | |
| if ((baseline - _config.LowerPowerLimit) >= reduction) { | |
| setAcOutput(baseline - reduction); | |
| return reduction; | |
| uint16_t baseline = currentOutputAcWatts; | |
| // when the output is higher than the limit, we must use the limit as the baseline | |
| if (currentOutputAcWatts > currentLimitWatts) { | |
| baseline = currentLimitWatts; | |
| } | |
| if ((baseline - _config.LowerPowerLimit) >= reduction) { | |
| setAcOutput(baseline - reduction); | |
| return currentOutputAcWatts - (baseline - reduction); | |
| } |
🤖 Prompt for AI Agents
In src/PowerLimiterSmartBufferInverter.cpp around lines 74 to 83, the function
currently returns 'reduction' when currentOutputAcWatts > currentLimitWatts,
which is incorrect; update the return value to compute and return the actual
reduction applied relative to currentOutputAcWatts (i.e., currentOutputAcWatts -
(baseline - reduction)) after calling setAcOutput(baseline - reduction). Ensure
the baseline logic remains (baseline = currentLimitWatts when output exceeds
limit) and mirror the correct return behavior used on lines 88 and 92 so the
function reports the true delta applied to the output.
| uint16_t currentLimitWatts = getCurrentLimitWatts(); | ||
| uint16_t baseline = currentOutputAcWatts; | ||
|
|
||
| if ((currentOutputAcWatts - _config.LowerPowerLimit) >= reduction) { | ||
| setAcOutput(currentOutputAcWatts - reduction); | ||
| // when the output is higher than the limit, we must use the limit as the baseline | ||
| if (currentOutputAcWatts > currentLimitWatts) { | ||
| baseline = currentLimitWatts; | ||
| } | ||
|
|
||
| if ((baseline - _config.LowerPowerLimit) >= reduction) { | ||
| setAcOutput(baseline - reduction); | ||
| return reduction; |
There was a problem hiding this comment.
Incorrect return value when output exceeds limit.
When currentOutputAcWatts > currentLimitWatts, the baseline is set to currentLimitWatts and reduction is applied to it. However, line 122 returns reduction instead of the actual reduction in output.
The actual reduction in output is:
currentOutputAcWatts - (baseline - reduction)
When baseline < currentOutputAcWatts, returning just reduction underreports the actual reduction to the caller (DPL), leading to incorrect power distribution decisions across inverters.
Compare with line 126 and the standby() method (line 134), which correctly return the actual reduction from currentOutputAcWatts.
Apply this diff to fix the return value:
if ((baseline - _config.LowerPowerLimit) >= reduction) {
setAcOutput(baseline - reduction);
- return reduction;
+ return currentOutputAcWatts - (baseline - reduction);
}🤖 Prompt for AI Agents
In src/PowerLimiterSolarInverter.cpp around lines 112 to 122, the function
returns just `reduction` after calling setAcOutput(baseline - reduction), which
underreports the actual output reduction when baseline was clipped to
currentLimitWatts; change the return value to the actual reduction applied to
the inverter: compute and return currentOutputAcWatts - (baseline - reduction)
(keeping the setAcOutput call as-is) so callers receive the true delta in
output.
|
Was bedeuten die Ausgaben von coderabbitai für meine Problemdarstellung? |
|
Hier noch einmal anbei ein Log, ich glaube jetzt im richtigen Moment gemacht. |
|
Was @coderabbitai da ausgespuckt hat hab ich mir noch nicht angeschaut. Dein Log ist leider sehr kurz, mehrere Zyklen wären gut um zu sehen wie alles reagiert. Aktuelles Limit 182 W |
|
Tip For best results, initiate chat on the files or code changes. @AndreasBoehm: Vielen Dank für die Rückmeldung! Die Testergebnisse sehen vielversprechend aus. Wenn du meine Review anschauen möchtest und Fragen oder Anmerkungen hast, stehe ich gerne zur Verfügung. Die von mir identifizierten Punkte beziehen sich hauptsächlich auf Code-Qualität und Dokumentation, aber die Kernlogik der Änderung erscheint solide. |
2 participants
Fixes #2271
This pull request updates the logic for applying power reduction in both the smart buffer inverter and solar inverter classes. The main improvement is that reductions are now calculated using the lower of the current output or the current limit as the baseline, ensuring output never exceeds configured limits after reduction.
Power reduction logic improvements:
PowerLimiterSmartBufferInverter::applyReductionandPowerLimiterSolarInverter::applyReduction, the baseline for reduction is now set to the lower of the current output or the current limit, preventing output from exceeding the allowed limit after a reduction. [1] [2]currentLimitWattsin both methods for clarity and to avoid repeated function calls. [1] [2]