Curtailment: report curtailed percent instead of bool - #32010
Merged
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In EEBus.CurtailedPercent, the percent calculation
int(-limit.Value / nominal * 100)can yield values outside 0–100 or be sensitive to floating‑point rounding; consider explicitly clamping the result to [0,100] and/or rounding instead of truncating. - Several curtailed percent scripts (e.g. sunspec, enphase, huawei, atmoce) directly forward or derive percentages without bounds checking; it may be safer to normalize their outputs to a consistent 0–100 range to avoid unexpected values propagating through the Curtailer API.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In EEBus.CurtailedPercent, the percent calculation `int(-limit.Value / nominal * 100)` can yield values outside 0–100 or be sensitive to floating‑point rounding; consider explicitly clamping the result to [0,100] and/or rounding instead of truncating.
- Several curtailed percent scripts (e.g. sunspec, enphase, huawei, atmoce) directly forward or derive percentages without bounds checking; it may be safer to normalize their outputs to a consistent 0–100 range to avoid unexpected values propagating through the Curtailer API.
## Individual Comments
### Comment 1
<location path="meter/eebus.go" line_range="272-281" />
<code_context>
c.mu.Lock()
defer c.mu.Unlock()
limit, err := eebusReadValue(c.eg.EgLPPInterface, c.egLppEntity, eebus.LPPLimit, c.eg.EgLPPInterface.ProductionLimit)
if err != nil {
- return false, err
+ return 0, err
+ }
+
+ // production limits are negative watts, a positive value is invalid
+ if !limit.IsActive || limit.Value > 0 {
+ return 100, nil
+ }
+
+ // without a nominal reference the limit cannot be expressed as a percent
+ nominal, err := c.eg.EgLPPInterface.ProductionNominalMax(c.egLppEntity)
+ if err != nil || nominal <= 0 {
+ return 0, api.ErrNotAvailable
}
- // Check if limit is active and has a valid power value (valid is zero or negative)
- return limit.IsActive && limit.Value <= 0, nil
+ return int(-limit.Value / nominal * 100), nil
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** CurtailedPercent calculation lacks clamping and may generate out-of-range or sign-incorrect values.
`int(-limit.Value / nominal * 100)` relies on `limit.Value` always being negative and `nominal` positive. With float representations, noise, or overshoot, this can yield values <0 or >100. Consider clamping the result to a defined range (e.g. 0..100) and explicitly handling cases like `limit.Value == 0`, in line with the intended EEBus semantics, to avoid propagating unexpected values to callers and UI.
Suggested implementation:
```golang
limit, err := eebusReadValue(c.eg.EgLPPInterface, c.egLppEntity, eebus.LPPLimit, c.eg.EgLPPInterface.ProductionLimit)
```
```golang
limit, err := eebusReadValue(c.eg.EgLPPInterface, c.egLppEntity, eebus.LPPLimit, c.eg.EgLPPInterface.ProductionLimit)
if err != nil {
return 0, err
}
// production limits are negative watts, a positive value is invalid
if !limit.IsActive || limit.Value > 0 {
return 100, nil
}
// explicitly handle "no curtailment" when an active limit is exactly zero
if limit.Value == 0 {
return 0, nil
}
// without a nominal reference the limit cannot be expressed as a percent
nominal, err := c.eg.EgLPPInterface.ProductionNominalMax(c.egLppEntity)
if err != nil || nominal <= 0 {
return 0, api.ErrNotAvailable
}
// calculate curtailed percent and clamp to [0, 100] to avoid propagating out-of-range values
percent := -limit.Value / nominal * 100
if percent < 0 {
percent = 0
} else if percent > 100 {
percent = 100
}
return int(percent), nil
```
If `limit.Value` and `nominal` are not floating-point types, adjust the calculation to ensure floating-point division (e.g. cast to `float64` before computing `percent`) so rounding and clamping behave as expected. Also verify that the semantics of `CurtailedPercent` in the rest of the codebase expect `0` for "no curtailment" and `100` for "fully curtailed" or "fully available"; if the meaning is reversed, you may need to invert the clamping logic accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
/cc @CiNcH83 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
pairs with #32006
api.Curtailerreported curtailment as a bool, which cannot express which limit a device currently applies. As a resultcurtailPVcompares "is curtailed" against "should be curtailed", so a HEMS moving between two curtailed steps (FNN emits 0/30/60/100, EEBus derives arbitrary percents from watts) is skipped and the new limit never reaches the device. Reporting the percent makes the comparison meaningful and mirrorsSetCurtailPercentandapi.HEMS.CurtailedPercent.Curtailed() (bool, error)becomesCurtailedPercent() (int, error), 100 = uncurtailedcurtailedplugin config changes from a bool to a percent, all curtailable meter templates read the limit register they already writeErrNotAvailablewhen the nominal production power needed to express the watt limit as a percent is unknown, instead of treating any active limit as curtailedCustom configurations with a hand-written
curtailedplugin need to return the percent instead of a bool.Note that #32006 adds a further curtailable template whose
curtailedscript needs the same change, whichever merges second.🤖 Generated with Claude Code