Huawei: re-add curtail#30145
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider adding validation or at least a default for
curtailpercent(e.g. range-limiting 0–100) to avoid writing invalid values to register 47418 when the parameter is unset or misconfigured. - The
curtailblock’sifcondition usessource: convertwithconvert: bool2intbut no clear boolean source; it would be clearer and less error‑prone to reference an explicit flag or parameter that determines whether curtailment should be enabled. - The magic values written to register 47415 (0 and 7) are only described in comments; consider extracting them as named constants or adding a short explanation of the mode mapping to make future changes and reviews easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding validation or at least a default for `curtailpercent` (e.g. range-limiting 0–100) to avoid writing invalid values to register 47418 when the parameter is unset or misconfigured.
- The `curtail` block’s `if` condition uses `source: convert` with `convert: bool2int` but no clear boolean source; it would be clearer and less error‑prone to reference an explicit flag or parameter that determines whether curtailment should be enabled.
- The magic values written to register 47415 (0 and 7) are only described in comments; consider extracting them as named constants or adding a short explanation of the mode mapping to make future changes and reviews easier.
## Individual Comments
### Comment 1
<location path="templates/definition/meter/huawei-sun2000-hybrid.yaml" line_range="168-157" />
<code_context>
+ address: 47415 # Active power control mode
+ type: writesingle
+ encoding: uint16
+ else:
+ source: convert
+ convert: bool2int
+ set:
+ source: const
+ value: 0 # Unlimited
+ set:
+ source: modbus
+ {{- include "modbus" . | indent 8 }}
+ register:
+ address: 47415 # Active power control mode
+ type: writesingle
+ encoding: uint16
+ curtailed:
+ source: modbus
</code_context>
<issue_to_address>
**issue (bug_risk):** When disabling curtailment, the maximum feed-in power register remains unchanged, which may leave a stale limit configured.
In the `else` branch of `curtail`, only register `47415` is set to `0` (“Unlimited”), while `47418` (max feed-in power, 0–100%) is left unchanged. This can leave a stale limit that will be silently reused if the mode is later switched away from `0`. Consider also resetting `47418` (e.g. to 100%) when disabling curtailment, or clearly document that the firmware ignores `47418` when mode is `0`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| register: | ||
| address: 47418 # Maximum Feed Grid Power (0-100 %) | ||
| type: writesingle | ||
| encoding: uint16 |
There was a problem hiding this comment.
issue (bug_risk): When disabling curtailment, the maximum feed-in power register remains unchanged, which may leave a stale limit configured.
In the else branch of curtail, only register 47415 is set to 0 (“Unlimited”), while 47418 (max feed-in power, 0–100%) is left unchanged. This can leave a stale limit that will be silently reused if the mode is later switched away from 0. Consider also resetting 47418 (e.g. to 100%) when disabling curtailment, or clearly document that the firmware ignores 47418 when mode is 0.
|
Is there anything holding this back? |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the
curtailif/else block, theifbranch usessource: convertwithconvert: bool2intbut no underlying source value is specified, so the condition may never reflect any real input; consider wiring this to an explicit signal (e.g. a param or capability flag) so the if/else actually toggles based on a meaningful value. - Both branches of the
curtaillogic manipulate register 47415, but the logic does not appear to consider or preserve any existing control mode that might have been set previously; if this inverter can be controlled by other features, you may want to ensure the curtail flow does not overwrite unrelated modes unintentionally.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `curtail` if/else block, the `if` branch uses `source: convert` with `convert: bool2int` but no underlying source value is specified, so the condition may never reflect any real input; consider wiring this to an explicit signal (e.g. a param or capability flag) so the if/else actually toggles based on a meaningful value.
- Both branches of the `curtail` logic manipulate register 47415, but the logic does not appear to consider or preserve any existing control mode that might have been set previously; if this inverter can be controlled by other features, you may want to ensure the curtail flow does not overwrite unrelated modes unintentionally.
## Individual Comments
### Comment 1
<location path="templates/definition/meter/huawei-sun2000-hybrid.yaml" line_range="141-145" />
<code_context>
decode: uint32nan
scale: 0.01
maxacpower: {{ .maxacpower }}
+ curtail:
+ source: ifelse
+ if:
+ source: convert
+ convert: bool2int
+ set:
+ source: sequence
</code_context>
<issue_to_address>
**suggestion:** The `bool2int` conversions in the `curtail` pipeline appear unused and may be redundant.
Both branches wrap `source: convert` with `convert: bool2int`, but that converted value isn’t used; the `set` chains just perform the Modbus writes. If you don’t need the converted value, drop the `convert: bool2int` wrappers; if you do, consider wiring it into the `ifelse` condition so its effect is explicit.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
follow-up #30116