Deye 3P Hybrid: use maxdischarge instead of TOU for active battery control #25945
Conversation
Replaces dangerous TOU manipulation with safe discharge current limiting: - Uses register 109 (Battery Max Discharging Current) for battery control - Parameters: maxdischargecurrent (100A default), holdcurrent (10A default) - Tested successfully on real Deye 3P hybrid inverter hardware - Safer approach that doesn't interfere with user's TOU schedules
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- With the switch to using register 109 only, the
minsocandmaxsocparams are no longer written to the inverter (only exposed as metadata at the bottom); if they are not actually used anywhere else, consider either wiring them to the appropriate SoC registers again or dropping them from this template to avoid confusing, no-op configuration options. - It might be safer to clamp or validate
maxdischargecurrentandholdcurrentagainst the inverter’s documented min/max current range to avoid writing out-of-spec values to register 109. - Please double-check whether
writemultiplewithdecode: uint16is the correct mode for register 109 on this device; if it is a single 16-bit register,write/holdingmight be more appropriate and less surprising for future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- With the switch to using register 109 only, the `minsoc` and `maxsoc` params are no longer written to the inverter (only exposed as metadata at the bottom); if they are not actually used anywhere else, consider either wiring them to the appropriate SoC registers again or dropping them from this template to avoid confusing, no-op configuration options.
- It might be safer to clamp or validate `maxdischargecurrent` and `holdcurrent` against the inverter’s documented min/max current range to avoid writing out-of-spec values to register 109.
- Please double-check whether `writemultiple` with `decode: uint16` is the correct mode for register 109 on this device; if it is a single 16-bit register, `write`/`holding` might be more appropriate and less surprising for future maintainers.
## Individual Comments
### Comment 1
<location> `templates/definition/meter/deye-hybrid-3p.yaml:26-34` </location>
<code_context>
+ - name: maxsoc
+ advanced: true
+ - name: maxacpower
+ - name: maxdischargecurrent
+ default: 100
+ advanced: true
+ description:
+ en: Maximum discharge current (normal mode)
+ de: Maximaler Entladestrom (Normalmodus)
+ help:
+ en: Battery discharge current limit when battery mode is set to normal or charge
+ de: Batterie-Entladestrom-Limit wenn Batteriemodus auf Normal oder Laden steht
+ - name: holdcurrent
+ default: 10
</code_context>
<issue_to_address>
**suggestion:** Consider constraining or documenting expected range/units for the new current parameters to reduce misconfiguration risk.
`maxdischargecurrent` and `holdcurrent` are free numeric parameters with high-ish defaults, but there’s no expressed range, units, or relationship (e.g. `holdcurrent` < `maxdischargecurrent`, register units/scaling). If supported, please add numeric constraints (min/max/step) and clearer units to prevent invalid or surprising configurations (e.g. `holdcurrent` > `maxdischargecurrent`).
Suggested implementation:
```
- name: maxdischargecurrent
default: 100
advanced: true
unit: A
min: 0
max: 200
step: 1
description:
en: Maximum discharge current (normal mode, in amperes)
de: Maximaler Entladestrom (Normalmodus, in Ampere)
help:
en: >
Battery discharge current limit in amperes when battery mode is set to normal or charge.
This value should be greater than or equal to the hold mode discharge current (holdcurrent).
de: >
Batterie-Entladestrom-Limit in Ampere, wenn der Batteriemodus auf Normal oder Laden steht.
Dieser Wert sollte größer oder gleich dem Entladestrom im Haltemodus (holdcurrent) sein.
- name: holdcurrent
default: 10
advanced: true
unit: A
min: 0
max: 100
step: 1
description:
en: Hold mode discharge current (in amperes)
de: Entladestrom im Haltemodus (in Ampere)
help:
en: >
Battery discharge current limit in amperes when battery mode is set to hold.
This should normally be lower than the maximum discharge current (maxdischargecurrent)
and low enough to cover the house baseload only.
de: >
Batterie-Entladestrom-Limit in Ampere, wenn der Batteriemodus auf Halten steht.
Dieser Wert sollte in der Regel niedriger als der maximale Entladestrom (maxdischargecurrent)
und niedrig genug für die Hausgrundlast sein.
```
If the surrounding schema uses different keys for numeric constraints (e.g. `min_value`/`max_value` instead of `min`/`max`, or does not support `step`), you should align `unit`, `min`, `max`, and `step` with the existing convention in this file or project. Also consider adjusting the numeric ranges (`max: 200` / `max: 100`) to match the actual supported current limits of the specific inverter/battery combination.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
andig
changed the title
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider enforcing the relationship between
maxdischargecurrentandholdcurrent(e.g. via validation or at least a note in the template) so users cannot configure aholdcurrenthigher thanmaxdischargecurrent, which would contradict the documented behavior. - The switch cases 1/2/3 now differ only by the constant value written to register 109; if the template language allows it, you could factor out the common Modbus write block to reduce duplication and make future changes to the register configuration less error-prone.
- With the removal of the TOU-based
minsoc/maxsocmanipulation, double-check whether any scenarios relied on those SOC register writes (e.g. for protection or limits) and, if needed, explicitly document that SOC is now controlled solely by the inverter’s own settings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider enforcing the relationship between `maxdischargecurrent` and `holdcurrent` (e.g. via validation or at least a note in the template) so users cannot configure a `holdcurrent` higher than `maxdischargecurrent`, which would contradict the documented behavior.
- The switch cases 1/2/3 now differ only by the constant value written to register 109; if the template language allows it, you could factor out the common Modbus write block to reduce duplication and make future changes to the register configuration less error-prone.
- With the removal of the TOU-based `minsoc`/`maxsoc` manipulation, double-check whether any scenarios relied on those SOC register writes (e.g. for protection or limits) and, if needed, explicitly document that SOC is now controlled solely by the inverter’s own settings.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - name: maxsoc | ||
| advanced: true | ||
| - name: maxacpower | ||
| - name: maxdischargecurrent |
There was a problem hiding this comment.
Both values should be required since they're unconditionally used below.
|
Could we get independent test @deadrabbit87? Would the same apply for |
I think, but I am not completely sure, that this solution should work exactly the same way for ‚deye-hybrid-hp3‘. The current TOU Solution doesn‘t work at all. |
|
I like the idea to use maxdischargecurrent for the HOLD function, I'm also fine with the holdcurrent parameter if users want to have the flexibility. Unfortunately, looking at the template code I don't think the CHARGE function will work. This is the same code as for NORMAL, hence, automatic charging of the battery using the evcc grid charge option for dynamic tariffs will not work, or do I miss something here? |
You are right. This is only to have the ability to choose if the battery should load the car or not. It's not for charging the battery. |
|
Maybe we join forces here - I'm currently working with two other Deye users on updating my previous PR #13142. I like the holdcurrent idea and could try to include your much shorter code for At the same time I'd also like to append the |
|
Genport is a separate thing, please open new pr. |
For later: For grid charging the battery, I don't think, you have to change the TOU timetable. You only need to disable it at all and later restore (or enable) it again. Should be register 0x92. With that, when TOU is disabled and grid charge in the deye is enabled, the battery will be grid charged with the rate of the grid charge limit (total battery charge limit will also be respected) until 100% (or settings are reverted). |
disabling TOU would count as "manipulation of tou" for me :D. Reverting it to the previous state is very important in that case. |
sure, no problem! |
Thanks for the hint, that's really cool! |
During testing I noted that this register also stores whether the days of the week are enabled/disabled (bit 1-7 Mon-Sun). Since the Deye Modbus implementation does not support bitwise operations and we cannot read and save the previous content via templates (afaik), we overwrite the register content when setting or resetting the TOU table (bit 0). This means we have changed the TOU schedule again... This may not be a big problem since I think the vast majority of users will have activated every day anyway. However, some users may need special use cases. To be sure, we would have to query each day individually whether the user wants to enable it or not. That would imho inflate the configuration quite a bit. |
|
I‘ve tested now several options, each with transitions of batterymode from NORMAL<->HOLD, NORMAL<->CHARGE, CHARGE<->HOLD. Basically, all options are working within their design. Result:
Since the missing CHARGE option in 1. is a show stopper for me (and probably many others using this option), I vote to go for option 3. When other Deye users have succesfully tested option 3 (ongoing), I’d push a separate PR for this. Any comments or other suggestions? |
Thank you for your support! Is it possible to have Maybe we can also improve the TOU Table approach later a bit. Not sure if its really needed to change all registers everytime or if we could reduce this. |
Unfortunately not. As soon as the
I currently do not see a way without TOU register modification, at least with the bare template approach, i.e. without writing a separate Go module for these inverters. I tested the idea of @bundi78 above, but this also alters the TOU table in a way and can only be avoided with dynamic reading of table values (not possible with templates). Still, this simple approach could work for many users not needing individual days of week settings. This is actually a decision for the individual Deye user. Hence, we could think to offer two possibilities during configuration:
This all will blow up the template a bit further, but I could live with this :-) |
Yes I am currently working on trying to harmonize both, but like you mentioned its not possible to have a strict split because if the charge mode is changing TOU we need to recover the settings in normal mode. |
|
Yes, there are some changes and bug fixes. You can take the latest template from my fork as base if you want. |
I already tried out something different, looks like a clean approach and works on my deye. |
That's now part of my last commit. For me that looks like a good solution without changing the tou. |
Great, that is the simple option from above. Works fine for me as well! I can perfectly live with the TOU days being overwritten. If someone complains, we still can think to implement the more complicated advanced mode with writing TOU registers. |
|
|
7 participants
Summary
Adds safe battery discharge control to the Deye 3P hybrid inverter template using discharge current limiting instead of TOU manipulation.
Changes
maxdischargecurrent(default 100A): Normal/charge mode discharge limitholdcurrent(default 10A): Hold mode discharge limit (for house baseload only)Testing
Benefits
holdthe battery is not getting drained from the carIssues
Adressing
#23599
#13142
#12333
#25489
#25467