DPL: Full-Solar-Passthrough: Respect DC loads #2264
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Build ArtifactsFirmware built from this pull request's code:
Notice
|
AndreasBoehm
force-pushed
the
andreasboehm/improvement/solar-passthrough-respect-dc-loads
branch
from
4ed203d to
f7acaf6
Compare
We calculate the power drawn by to us unknown DC loads to avoid that the battery is getting drained during full-solar-pasthrough.
AndreasBoehm
force-pushed
the
andreasboehm/improvement/solar-passthrough-respect-dc-loads
branch
from
f7acaf6 to
c60b2db
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe change replaces solar passthrough power with solar output power and introduces DC-load-aware calculations. New accessors expose inverter DC power and battery stats age. Implementations compute DC loads, inverter DC consumption, and adjust logging and control paths accordingly. Headers and source files are updated to match the new API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor ControlLoop
participant PL as PowerLimiter
participant INV as Inverter(s)
participant BAT as Batteries::Stats
ControlLoop->>PL: evaluateLimits()
PL->>PL: getSolarOutputPower()
note right of PL: Replaces prior solar passthrough getter
PL->>INV: getCurrentDcPower() (per inverter)
INV-->>PL: sum(PDC across DC channels)
PL->>PL: getBatteryInvertersOutputDcWatts()
PL->>BAT: read charge/discharge metrics
BAT-->>PL: current, power, ages
PL->>PL: getDcLoadsPowerDc()<br/>= inverter DC + DC loads ± battery flow
alt Passthrough allowed
PL-->>ControlLoop: apply full solar passthrough
else Limit required
PL-->>ControlLoop: apply limits based on DC-load-aware budget
end
note over PL: Logging updated to include solar output and DC loads
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (1 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/PowerLimiter.h(1 hunks)include/PowerLimiterInverter.h(1 hunks)include/battery/Stats.h(1 hunks)src/PowerLimiter.cpp(2 hunks)src/PowerLimiterInverter.cpp(1 hunks)
Comment on lines
+707
to
739
| auto solarOutputDc = getSolarOutputPower(); | ||
| auto solarOutputAc = dcPowerBusToInverterAc(solarOutputDc); | ||
| if (isFullSolarPassthroughActive() && solarOutputAc > powerRequested) { | ||
| DTU_LOGD("using %u/%u W DC/AC from DC power bus (full solar-passthrough)", | ||
| solarOutputDc, solarOutputAc); | ||
|
|
||
| return solarOutputAc; | ||
| auto dcLoadsPowerDc = getDcLoadsPowerDc(); | ||
| auto solarPassthroughPowerDc = solarOutputDc - dcLoadsPowerDc; | ||
| auto solarPassthroughPowerAc = dcPowerBusToInverterAc(solarPassthroughPowerDc); | ||
| if (isFullSolarPassthroughActive() && solarPassthroughPowerAc > powerRequested) { | ||
| DTU_LOGD("using %u/%u W DC/AC from DC power bus, solar power is %u/%u W DC/AC, " | ||
| "dc loads are %u W DC (full solar-passthrough)", | ||
| solarPassthroughPowerDc, solarPassthroughPowerAc, | ||
| solarOutputDc, solarOutputAc, | ||
| dcLoadsPowerDc); | ||
|
|
||
| return solarPassthroughPowerAc; | ||
| } | ||
|
|
||
| auto oBatteryDischargeLimit = getBatteryDischargeLimit(); | ||
| if (!oBatteryDischargeLimit) { | ||
| DTU_LOGD("granting %d W from DC power bus (no battery discharge " | ||
| "limit), solar power is %u/%u W DC/AC", | ||
| powerRequested, solarOutputDc, solarOutputAc); | ||
| "limit), solar power is %u/%u W DC/AC, dc loads are %u W DC", | ||
| powerRequested, solarOutputDc, solarOutputAc, dcLoadsPowerDc); | ||
| return powerRequested; | ||
| } | ||
|
|
||
| auto batteryAllowanceAc = dcPowerBusToInverterAc(*oBatteryDischargeLimit); | ||
|
|
||
| DTU_LOGD("battery allowance is %u/%u W DC/AC, solar power is %u/%u W DC/AC, " | ||
| "requested are %u W AC", | ||
| "dc loads are %u W DC, requested are %u W AC", | ||
| *oBatteryDischargeLimit, batteryAllowanceAc, | ||
| solarOutputDc, solarOutputAc, powerRequested); | ||
| solarOutputDc, solarOutputAc, | ||
| dcLoadsPowerDc, powerRequested); | ||
|
|
||
| uint16_t allowance = batteryAllowanceAc + solarOutputAc; |
There was a problem hiding this comment.
Clamp solar passthrough power before converting to AC
solarPassthroughPowerDc is computed as solarOutputDc - dcLoadsPowerDc. When the DC loads exceed the solar output this becomes negative, yet it is immediately passed to dcPowerBusToInverterAc(uint16_t). The implicit conversion wraps the negative value to a huge positive number, so the limiter can believe >60 kW DC is available and will request far too much AC power. Please clamp the difference to zero (or early-out) before casting back to an unsigned type, and adjust the logging accordingly.
- auto solarPassthroughPowerDc = solarOutputDc - dcLoadsPowerDc;
- auto solarPassthroughPowerAc = dcPowerBusToInverterAc(solarPassthroughPowerDc);
+ int32_t solarPassthroughPowerDc = static_cast<int32_t>(solarOutputDc) - static_cast<int32_t>(dcLoadsPowerDc);
+ if (solarPassthroughPowerDc < 0) {
+ solarPassthroughPowerDc = 0;
+ }
+ auto solarPassthroughPowerAc = dcPowerBusToInverterAc(static_cast<uint16_t>(solarPassthroughPowerDc));
if (isFullSolarPassthroughActive() && solarPassthroughPowerAc > powerRequested) {
DTU_LOGD("using %u/%u W DC/AC from DC power bus, solar power is %u/%u W DC/AC, "
"dc loads are %u W DC (full solar-passthrough)",
- solarPassthroughPowerDc, solarPassthroughPowerAc,
+ static_cast<uint16_t>(solarPassthroughPowerDc), solarPassthroughPowerAc,
solarOutputDc, solarOutputAc,
dcLoadsPowerDc);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto solarOutputDc = getSolarOutputPower(); | |
| auto solarOutputAc = dcPowerBusToInverterAc(solarOutputDc); | |
| if (isFullSolarPassthroughActive() && solarOutputAc > powerRequested) { | |
| DTU_LOGD("using %u/%u W DC/AC from DC power bus (full solar-passthrough)", | |
| solarOutputDc, solarOutputAc); | |
| return solarOutputAc; | |
| auto dcLoadsPowerDc = getDcLoadsPowerDc(); | |
| auto solarPassthroughPowerDc = solarOutputDc - dcLoadsPowerDc; | |
| auto solarPassthroughPowerAc = dcPowerBusToInverterAc(solarPassthroughPowerDc); | |
| if (isFullSolarPassthroughActive() && solarPassthroughPowerAc > powerRequested) { | |
| DTU_LOGD("using %u/%u W DC/AC from DC power bus, solar power is %u/%u W DC/AC, " | |
| "dc loads are %u W DC (full solar-passthrough)", | |
| solarPassthroughPowerDc, solarPassthroughPowerAc, | |
| solarOutputDc, solarOutputAc, | |
| dcLoadsPowerDc); | |
| return solarPassthroughPowerAc; | |
| } | |
| auto oBatteryDischargeLimit = getBatteryDischargeLimit(); | |
| if (!oBatteryDischargeLimit) { | |
| DTU_LOGD("granting %d W from DC power bus (no battery discharge " | |
| "limit), solar power is %u/%u W DC/AC", | |
| powerRequested, solarOutputDc, solarOutputAc); | |
| "limit), solar power is %u/%u W DC/AC, dc loads are %u W DC", | |
| powerRequested, solarOutputDc, solarOutputAc, dcLoadsPowerDc); | |
| return powerRequested; | |
| } | |
| auto batteryAllowanceAc = dcPowerBusToInverterAc(*oBatteryDischargeLimit); | |
| DTU_LOGD("battery allowance is %u/%u W DC/AC, solar power is %u/%u W DC/AC, " | |
| "requested are %u W AC", | |
| "dc loads are %u W DC, requested are %u W AC", | |
| *oBatteryDischargeLimit, batteryAllowanceAc, | |
| solarOutputDc, solarOutputAc, powerRequested); | |
| solarOutputDc, solarOutputAc, | |
| dcLoadsPowerDc, powerRequested); | |
| uint16_t allowance = batteryAllowanceAc + solarOutputAc; | |
| auto dcLoadsPowerDc = getDcLoadsPowerDc(); | |
| - auto solarPassthroughPowerDc = solarOutputDc - dcLoadsPowerDc; | |
| int32_t solarPassthroughPowerDc = static_cast<int32_t>(solarOutputDc) | |
| - static_cast<int32_t>(dcLoadsPowerDc); | |
| if (solarPassthroughPowerDc < 0) { | |
| solarPassthroughPowerDc = 0; | |
| } | |
| auto solarPassthroughPowerAc = dcPowerBusToInverterAc( | |
| static_cast<uint16_t>(solarPassthroughPowerDc)); | |
| if (isFullSolarPassthroughActive() && solarPassthroughPowerAc > powerRequested) { | |
| DTU_LOGD("using %u/%u W DC/AC from DC power bus, solar power is %u/%u W DC/AC, " | |
| "dc loads are %u W DC (full solar-passthrough)", | |
| static_cast<uint16_t>(solarPassthroughPowerDc), solarPassthroughPowerAc, | |
| solarOutputDc, solarOutputAc, | |
| dcLoadsPowerDc); |
Comment on lines
+347
to
+358
| float PowerLimiterInverter::getCurrentDcPower() | ||
| { | ||
| float dcPower = 0; | ||
| auto pStats = _spInverter->Statistics(); | ||
| std::vector<ChannelNum_t> dcChannels = _spInverter->getChannelsDC(); | ||
|
|
||
| for (auto& c : dcChannels) { | ||
| dcPower += pStats->getChannelFieldValue(TYPE_DC, c, FLD_PDC); | ||
| } | ||
|
|
||
| return dcPower; | ||
| } |
There was a problem hiding this comment.
Make getCurrentDcPower() const-safe
_inverters is iterated as auto const&, so upInv->getCurrentDcPower() is invoked on a PowerLimiterInverter const*. Because this definition lacks a const qualifier the code will not compile. Please mark the method const here and update the declaration in include/PowerLimiterInverter.h to match.
-float PowerLimiterInverter::getCurrentDcPower()
+float PowerLimiterInverter::getCurrentDcPower() const📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| float PowerLimiterInverter::getCurrentDcPower() | |
| { | |
| float dcPower = 0; | |
| auto pStats = _spInverter->Statistics(); | |
| std::vector<ChannelNum_t> dcChannels = _spInverter->getChannelsDC(); | |
| for (auto& c : dcChannels) { | |
| dcPower += pStats->getChannelFieldValue(TYPE_DC, c, FLD_PDC); | |
| } | |
| return dcPower; | |
| } | |
| float PowerLimiterInverter::getCurrentDcPower() const | |
| { | |
| float dcPower = 0; | |
| auto pStats = _spInverter->Statistics(); | |
| std::vector<ChannelNum_t> dcChannels = _spInverter->getChannelsDC(); | |
| for (auto& c : dcChannels) { | |
| dcPower += pStats->getChannelFieldValue(TYPE_DC, c, FLD_PDC); | |
| } | |
| return dcPower; | |
| } |
🤖 Prompt for AI Agents
In src/PowerLimiterInverter.cpp around lines 347-358, the member function
getCurrentDcPower() must be made const to allow calls on const
PowerLimiterInverter instances; add the const qualifier after the parameter list
in the definition and update the declaration in include/PowerLimiterInverter.h
to match (i.e., declare getCurrentDcPower() as a const member). Ensure the
implementation only calls const-qualified methods/data or otherwise avoids
mutating members so the compiler accepts the const qualification.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces enhancements to the solar and battery power management logic, focusing on more accurate calculation and reporting of available DC and AC power, as well as improving the tracking of battery and inverter statistics. The main changes involve refactoring how solar output and passthrough power are computed, adding new methods to expose internal state, and increasing the precision of battery statistics.
Power calculation and reporting improvements:
PowerLimiterClass::calcPowerBusUsageand related methods to distinguish between solar passthrough power and total solar output, improving clarity and accuracy in power allocation.PowerLimiterClassfor retrieving solar output power, solar passthrough power, and battery inverter DC output, enabling more granular control and reporting of power flows. [1] [2]Inverter statistics enhancements:
PowerLimiterInverter::getCurrentDcPowerto calculate the total DC power output for each inverter, supporting more precise measurement of inverter contributions to the power bus. [1] [2]Battery statistics improvements:
Statsfor retrieving the age (in seconds) of charge current and discharge current limit readings, improving the reliability of decisions based on battery data freshness.