SunSpec (model 124): refresh battery mode via InOutWRte_RvrtTms watchdog - #33041
SunSpec (model 124): refresh battery mode via InOutWRte_RvrtTms watchdog#33041andig wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the fronius-gen24 template you guard the negative maxchargerate with
{{ if .maxchargerate }}-{{ .maxchargerate }}{{ end }}, but in fronius-vertoplus and sunspec-inverter-control you use-{{ .maxchargerate }}directly; consider making this consistent so configurations without maxchargerate set don’t generate invalid values. - The new watchdog sequences for the different batterymode cases are largely duplicated across the three templates; you may want to extract common subsequences (e.g. setting
InOutWRte_RvrtTmsandStorCtl_Mod) via shared templates or YAML anchors to reduce the chance of future divergence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the fronius-gen24 template you guard the negative maxchargerate with `{{ if .maxchargerate }}-{{ .maxchargerate }}{{ end }}`, but in fronius-vertoplus and sunspec-inverter-control you use `-{{ .maxchargerate }}` directly; consider making this consistent so configurations without maxchargerate set don’t generate invalid values.
- The new watchdog sequences for the different batterymode cases are largely duplicated across the three templates; you may want to extract common subsequences (e.g. setting `InOutWRte_RvrtTms` and `StorCtl_Mod`) via shared templates or YAML anchors to reduce the chance of future divergence.
## Individual Comments
### Comment 1
<location path="templates/definition/meter/fronius-vertoplus.yaml" line_range="238-206" />
<code_context>
- uri: {{ joinHostPort .host .port }}
- id: 1
- value: 124:0:OutWRte
- - case: 3 # charge
- set:
- source: sequence
+ # device reverts to normal unless refreshed by the watchdog
+ - source: const
+ value: {{ durationSeconds .watchdog }} # s
+ set:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Charge mode depends on maxchargerate without guarding against it being unset or zero
Here `value: -{{ .maxchargerate }} # %` is used directly in case `3`. If `.maxchargerate` is unset or zero, this will produce an invalid/ineffective write to `OutWRte`. Other templates (e.g. GEN24) guard with `{{ if .maxchargerate }}`; please add a similar guard or a safe default so misconfigured `maxchargerate` doesn’t cause confusing device behavior.
Suggested implementation:
```
{{- if .maxchargerate }}
- source: const
value: -{{ .maxchargerate }} # %
{{- end }}
```
From the partial context, I can’t see the full `switch` and `case: 3 # charge` block. Please ensure that:
1. The `SEARCH` snippet (`- source: const` and `value: -{{ .maxchargerate }} # %`) exactly matches the current code inside `case: 3 # charge` in `templates/definition/meter/fronius-vertoplus.yaml`.
2. The `{{- if .maxchargerate }}` / `{{- end }}` wrapping is aligned with the rest of the templating style in this file (indentation, dash trimming, etc.) consistent with how GEN24 templates handle `maxchargerate`.
3. If the template uses a different structure (e.g., `source: sequence` with nested steps), the `if` guard should wrap only the single step that writes the negative `maxchargerate` (the charge limit) to the device, not the entire sequence.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Ich glaube laut Spezifikation ist InOutWRte_RvrtTms or RvrtTms in Model 124 optional. Ich bin nicht überzeugt, das es für den generischen sunspec-Meter passt. Nachfolger wären Modelle 704/713, die man ggf. berücksichtigen müsste? Haben Andere gen24-Nutzer den Wert einmalig auf 0 oder höher gesetzt bisher, oder ist das eine Anpassung des defaults der Firmware? Im sunspec-Modell eines Kostal, der ja nicht Teil dieses PR ist, fehlt der Wert trotz notwendiger Watchdog-Steuerung, aber er implementiert auch das Modell 124 (oder die 700er) nicht. |
|
Does this in any form have negative impact? If not we could merge for those devices that need it. |
|
I think atm this might have more impact than use? |
|
Which impact do you see? |
| set: | ||
| source: sunspec | ||
| {{- include "modbus" . | indent 12 }} | ||
| value: 124:0:InOutWRte_RvrtTms |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
double checked, even if I cannot test, the implementation should be fine. Even on watchdog is defined as zero it should be safe and any sunspec device should not fail on optional registers
fixes #33037
The battery mode is written once per mode transition, but SunSpec model 124 reverts the storage control settings after
InOutWRte_RvrtTmsseconds (3600 s on a GEN24), so a hold silently expires mid-session. Instead of disabling the revert timer, the timer now acts as a watchdog: it is set to the watchdog interval when a non-normal mode is engaged and the mode is re-applied at half that interval.fronius-gen24,fronius-vertoplus,sunspec-inverter-control:batterymodewrapped in thewatchdogplugin,InOutWRte_RvrtTmswritten on hold/charge/holdchargewatchdogparameter in seconds, default 60 — the battery returns to normal operation on its own if evcc stops refreshingsunspec/readservice so the parameter can be filled with the value the device currently uses🤖 Generated with Claude Code