Battery: add HoldCharge mode, rename Hold to HoldDischarge#30259
Conversation
Introduce a new hardware-level battery mode that prevents charging while still allowing discharge. This is implemented via the SunSpec Model 124 InWRte register (charge rate limit set to 0%), which has been verified on Fronius GEN24 hardware. The mode is mapped as case 4 in battery controller templates: - StorCtl_Mod=1 (limit charge rate) - InWRte=0% (zero charge rate) Template support is added for Fronius GEN24, Fronius Verto Plus, and the generic SunSpec inverter control template. The Solar API template returns ErrNotAvailable as it has no equivalent HTTP endpoint. E3DC and SOC-based battery limit controllers are also updated to handle the new mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The existing BatteryHold mode prevents discharge while allowing charge.
With the addition of BatteryHoldCharge (prevents charge, allows
discharge), the asymmetric naming was confusing.
Rename BatteryHold to BatteryHoldDischarge consistently across Go API,
enumer, core, meter, cmd, TypeScript enum, Vue components, and i18n
keys.
Breaking change: the wire-format value sent to the REST endpoint
POST /batterymode/{value}, the CLI flag --battery-mode, and the MQTT
batteryMode topic changes from "hold" to "holddischarge". Integrations
referencing the old value must be updated.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Given the explicit breaking change of renaming
hold→holddischarge, consider accepting the legacyholdstring in the battery mode parsing layer (e.g.BatteryModeStringmap / HTTP handler) and mapping it toBatteryHoldDischargeso existing integrations keep working while you return the new canonical value. - For meters that don’t support
BatteryHoldChargeand currently returnErrNotAvailable(e.g. SolarAPI), it may be worth aligning behavior across backends so that unsupported modes either consistently no-op or consistently error, depending on what clients expect from/batterymode.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Given the explicit breaking change of renaming `hold` → `holddischarge`, consider accepting the legacy `hold` string in the battery mode parsing layer (e.g. `BatteryModeString` map / HTTP handler) and mapping it to `BatteryHoldDischarge` so existing integrations keep working while you return the new canonical value.
- For meters that don’t support `BatteryHoldCharge` and currently return `ErrNotAvailable` (e.g. SolarAPI), it may be worth aligning behavior across backends so that unsupported modes either consistently no-op or consistently error, depending on what clients expect from `/batterymode`.
## Individual Comments
### Comment 1
<location path="templates/definition/meter/fronius-gen24.yaml" line_range="259" />
<code_context>
+ value: 1
+ set:
+ source: sunspec
+ uri: {{ .host }}:{{ .port }}
+ id: 1
+ value: 124:0:StorCtl_Mod
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The new `uri` usage is inconsistent with the rest of this template (which uses `joinHostPort`) and likely with the README’s helper-guidance.
In this file, other cases use `uri: {{ joinHostPort .host .port }}` (e.g. in the existing case right above this change). To stay consistent with the existing templates and the likely conventions from `templates/README.md`, this should probably also use the `joinHostPort` helper instead of manually concatenating `{{ .host }}:{{ .port }}`.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `templates/**/*.yaml`
**Instructions:**
Verify that the changes comply to the rules defined in templates/README.md. Also check consistence (wording) with other files inside templates directory.
</details>
</issue_to_address>
### Comment 2
<location path="templates/definition/meter/fronius-vertoplus.yaml" line_range="255" />
<code_context>
+ value: 1
+ set:
+ source: sunspec
+ uri: {{ .host }}:{{ .port }}
+ id: 1
+ value: 124:0:StorCtl_Mod
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This new `uri` format diverges from the rest of the template’s use of `joinHostPort`, breaking internal consistency and likely the README’s conventions.
Similar to `fronius-gen24.yaml`, other cases in this file use `uri: {{ joinHostPort .host .port }}`. For consistency with existing templates and the conventions likely described in `templates/README.md`, consider switching this to `joinHostPort` instead of interpolating `{{ .host }}:{{ .port }}` manually.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `templates/**/*.yaml`
**Instructions:**
Verify that the changes comply to the rules defined in templates/README.md. Also check consistence (wording) with other files inside templates directory.
</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.
| value: 1 | ||
| set: | ||
| source: sunspec | ||
| uri: {{ .host }}:{{ .port }} |
There was a problem hiding this comment.
suggestion (review_instructions): The new uri usage is inconsistent with the rest of this template (which uses joinHostPort) and likely with the README’s helper-guidance.
In this file, other cases use uri: {{ joinHostPort .host .port }} (e.g. in the existing case right above this change). To stay consistent with the existing templates and the likely conventions from templates/README.md, this should probably also use the joinHostPort helper instead of manually concatenating {{ .host }}:{{ .port }}.
Review instructions:
Path patterns: templates/**/*.yaml
Instructions:
Verify that the changes comply to the rules defined in templates/README.md. Also check consistence (wording) with other files inside templates directory.
| value: 1 | ||
| set: | ||
| source: sunspec | ||
| uri: {{ .host }}:{{ .port }} |
There was a problem hiding this comment.
suggestion (review_instructions): This new uri format diverges from the rest of the template’s use of joinHostPort, breaking internal consistency and likely the README’s conventions.
Similar to fronius-gen24.yaml, other cases in this file use uri: {{ joinHostPort .host .port }}. For consistency with existing templates and the conventions likely described in templates/README.md, consider switching this to joinHostPort instead of interpolating {{ .host }}:{{ .port }} manually.
Review instructions:
Path patterns: templates/**/*.yaml
Instructions:
Verify that the changes comply to the rules defined in templates/README.md. Also check consistence (wording) with other files inside templates directory.
|
Is there a good reason to rename hold and not keep it as a separate state? My Atmoce battery is charging nor discharging at hold, so actual hold/pause. |
Consistency
It still is a separate state? |
Clear.
I meant besides holdcharge and holddischarge 🙂 |
|
For what use case? |
More like a pause of the battery (no charging or discharging), but thinking it over again, you could get the same result if you select holdcharge/holddischarge depending on the situation. Disabling both charging and discharging at the same time makes the battery a bit useless. |
3 participants
replaces #27906
fixes #30249
Adds a new battery mode that prevents charging while still allowing discharge (BatteryHoldCharge), and renames the existing BatteryHold (prevents discharge) to BatteryHoldDischarge for symmetry. Builds on @larsxschneider's commits from #27906.
batteryHold.Breaking change. The wire-format value used by
POST /batterymode/{value}, the--battery-modeCLI flag, and thebatteryModeMQTT topic changes fromholdtoholddischarge. Integrations referencing the old value must be updated. The new fourth mode isholdcharge.