Meter (Growatt TL-XH): write battery-first time slot atomically#30403
Merged
Conversation
Combine the start (3038) and end (3039) time-slot registers into a single FC16 multi-register write. The firmware rejects a standalone write to 3039 with modbus exception 0, aborting the mode sequence so battery hold never engages.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The large uint32 constants (536876859, 2684360507) are hard to read and verify; consider expressing them as computed values (e.g.
(8192 << 16) | 5947/0x2000173B) or via named variables to make the intent and calculation clearer. - The sequences for the enabled time slot are duplicated with the same constant and Modbus write in multiple branches; consider factoring this into a shared helper or template block to avoid divergence if the register layout changes again.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The large uint32 constants (536876859, 2684360507) are hard to read and verify; consider expressing them as computed values (e.g. `(8192 << 16) | 5947` / `0x2000173B`) or via named variables to make the intent and calculation clearer.
- The sequences for the enabled time slot are duplicated with the same constant and Modbus write in multiple branches; consider factoring this into a shared helper or template block to avoid divergence if the register layout changes again.
## Individual Comments
### Comment 1
<location path="templates/definition/meter/growatt-hybrid-tlxh.yaml" line_range="153" />
<code_context>
set:
source: sequence
set:
+ # value = (start << 16) | end = (40960 << 16) | 5947 = 0xA000173B; bit15=1 -> slot enabled
- source: const
- value: 40960
</code_context>
<issue_to_address>
**suggestion:** Clarify which word and bit the `bit15` reference applies to in the packed value.
The packed value is `(start << 16) | end`, so it’s unclear whether `bit15` refers to the `start` or `end` 16-bit word. Please update the comment to explicitly state which 16‑bit word’s bit 15 controls the slot enable (e.g., `bit15 of the start word`).
```suggestion
# value = (start << 16) | end = (40960 << 16) | 5947 = 0xA000173B; bit15 of the start word (high 16 bits) = 1 -> slot enabled
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| set: | ||
| source: sequence | ||
| set: | ||
| # value = (start << 16) | end = (40960 << 16) | 5947 = 0xA000173B; bit15=1 -> slot enabled |
Contributor
There was a problem hiding this comment.
suggestion: Clarify which word and bit the bit15 reference applies to in the packed value.
The packed value is (start << 16) | end, so it’s unclear whether bit15 refers to the start or end 16-bit word. Please update the comment to explicitly state which 16‑bit word’s bit 15 controls the slot enable (e.g., bit15 of the start word).
Suggested change
| # value = (start << 16) | end = (40960 << 16) | 5947 = 0xA000173B; bit15=1 -> slot enabled | |
| # value = (start << 16) | end = (40960 << 16) | 5947 = 0xA000173B; bit15 of the start word (high 16 bits) = 1 -> slot enabled |
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.
fixes #23300
The battery-mode switch wrote the "battery first" Time-1 slot as two separate single-register FC16 writes (start time 3038, then end time 3039). The TL-XH/MOD-XH firmware ACKs 3038 but rejects the standalone write to 3039 with modbus exception 0, so the
sequenceaborts before reaching 3049 and the requested hold never engages (battery keeps discharging into the car). The per-cycle retry also spamsbattery mode: modbus: exception '0', function '16'.Per the Growatt protocol, 3038/3039 form one time-period pair and must be set together via write multiple, the same atomicity the existing MIX/SPH
growatt-hybridtemplate documents.uint32write to 3038 (0xA000173B=(40960 << 16) | 5947), which evcc encodes big-endian as[3038]=0xA000, [3039]=0x173BTODO