Charger (Phoenix EV-ETH): add millicurrentsupported option#31860
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the phoenix-ev-eth.yaml template, the
rendersection compares.millicurrentsupportedto the string "false"; since this is defined as a bool, it would be clearer and less brittle to rely on its boolean truth value (e.g.{{- if .millicurrentsupported }}) instead of a string comparison.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the phoenix-ev-eth.yaml template, the `render` section compares `.millicurrentsupported` to the string "false"; since this is defined as a bool, it would be clearer and less brittle to rely on its boolean truth value (e.g. `{{- if .millicurrentsupported }}`) instead of a string comparison.
## Individual Comments
### Comment 1
<location path="templates/definition/charger/phoenix-ev-eth.yaml" line_range="57-58" />
<code_context>
render: |
type: phoenix-ev-eth
{{- include "modbus" . }}
+ {{- if ne .millicurrentsupported "false" }}
+ millicurrentsupported: true
+ {{- end }}
</code_context>
<issue_to_address>
**issue (bug_risk):** The template compares a boolean to the string "false", which prevents millicurrentsupported from ever being rendered.
`millicurrentsupported` is a bool, but the template checks `if ne .millicurrentsupported "false"`, i.e., a bool vs string comparison that will always be false. As a result, this block never runs and the flag is never emitted. Use a boolean condition instead, e.g. `{{- if .millicurrentsupported }}` or `{{- if ne .millicurrentsupported false }}` (without quotes).
</issue_to_address>
### Comment 2
<location path="templates/definition/charger/phoenix-ev-eth.yaml" line_range="57" />
<code_context>
render: |
type: phoenix-ev-eth
{{- include "modbus" . }}
+ {{- if ne .millicurrentsupported "false" }}
+ millicurrentsupported: true
+ {{- end }}
</code_context>
<issue_to_address>
**issue (review_instructions):** The conditional rendering of a boolean param compares against the string "false", which is inconsistent with typical template rules for boolean params and likely violates the README guidelines.
For params with `type: bool`, the templates/README.md usually requires treating them as actual booleans in the render block (e.g., `{{- if .millicurrentsupported }}`) rather than comparing them to string values. Using `ne .millicurrentsupported "false"` suggests the value is a string, which is inconsistent with a boolean parameter and with how other templates handle similar flags. Please update the conditional to follow the boolean-handling pattern described in the README and used in other templates.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `templates/**/*.yaml`
**Instructions:**
Verify that the changes comply to the rules defined in templates/README.md.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| {{- if ne .millicurrentsupported "false" }} | ||
| millicurrentsupported: true |
There was a problem hiding this comment.
issue (bug_risk): The template compares a boolean to the string "false", which prevents millicurrentsupported from ever being rendered.
millicurrentsupported is a bool, but the template checks if ne .millicurrentsupported "false", i.e., a bool vs string comparison that will always be false. As a result, this block never runs and the flag is never emitted. Use a boolean condition instead, e.g. {{- if .millicurrentsupported }} or {{- if ne .millicurrentsupported false }} (without quotes).
| render: | | ||
| type: phoenix-ev-eth | ||
| {{- include "modbus" . }} | ||
| {{- if ne .millicurrentsupported "false" }} |
There was a problem hiding this comment.
issue (review_instructions): The conditional rendering of a boolean param compares against the string "false", which is inconsistent with typical template rules for boolean params and likely violates the README guidelines.
For params with type: bool, the templates/README.md usually requires treating them as actual booleans in the render block (e.g., {{- if .millicurrentsupported }}) rather than comparing them to string values. Using ne .millicurrentsupported "false" suggests the value is a string, which is inconsistent with a boolean parameter and with how other templates handle similar flags. Please update the conditional to follow the boolean-handling pattern described in the README and used in other templates.
Review instructions:
Path patterns: templates/**/*.yaml
Instructions:
Verify that the changes comply to the rules defined in templates/README.md.
fixes #31844
The Wallbe/extended-firmware detection reads
phxRegMaxCurrentand treats a raw value>= 60as "supports 0.1A steps". If the register was last written with a value< 60(e.g. an E3/DC main system re-configuring the wallbox while evcc is offline), evcc misdetects the firmware and then writes plain amperes (6-16) instead of decimal amps (60-160), limiting the charger to 0.6-1.6A. The misdetection persists until the register is manually corrected.Adds an advanced
millicurrentsupportedbool that skips the heuristic and forces mA regulation. Unset/false keeps the existing autodetection, so no behaviour change for existing configs.Only the false-negative direction is overridable: OEM firmware never leaves a value
>= 60in that register, so autodetection cannot falsely report support.🤖 Generated with Claude Code