Repository navigation
Conversation
|
First of all, thanks for your PR. Good job! But to be honest, I didn't add this before on purpose. Are you aware of all the drawbacks of the remote mode? I think we can try to merge this, but it might be a good idea to add a dedicated documentation file and explain how it works. The most important part is describing all the risks for users. |
|
No, what are these risks? It seems to work fine.
…On Sun, 4 Oct 2026, 14:27 Lewa-Reka, ***@***.***> wrote:
*Lewa-Reka* left a comment (Lewa-Reka/esphome-deye-inverter#148)
<#148 (comment)>
First of all, thanks for your PR. Good job!
But to be honest, I didn't add this before on purpose. Are you aware of
all the drawbacks of the remote mode?
I think we can try to merge this, but it might be a good idea to add a
dedicated documentation file and explain how it works. The most important
part is describing all the risks for users.
—
Reply to this email directly, view it on GitHub
<#148?email_source=notifications&email_token=ABJLYGGPJSNFWL3LI4J3EJD5SI6Z5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXHE4TANZVGE4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5979907518>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJLYGD4VBHKMDNRJDXZW3T5SI6Z5AVCNFSNUABFKJSXA33TNF2G64TZHM4TSNJUGIZTCNJRHNEXG43VMU5TKNRQGE4DCMZZGA22C5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ABJLYGAEVMYXKS5LVFQKMFD5SI6Z5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXHE4TANZVGE4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/ABJLYGHR5B7472GICHGVGAD5SI6Z5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJXHE4TANZVGE4KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@sourcery-ai review |
Reviewer's GuideIntroduces Modbus-backed controls for operating the inverter in remote mode, including control-mode selection and scaled battery, phase-power, watchdog, and solar-power parameters, and wires them into the 3-phase inverter configuration. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pv_inverter/packages/deye_hybrid_3p/remote.yaml" line_range="5" />
<code_context>
+ - platform: modbus_controller
+ use_write_multiple: true
+ modbus_controller_id: ${modbus_controller_id}
+ id: remote_mode
+ name: "Remote mode"
+ address: 1100
</code_context>
<issue_to_address>
**Remote setpoints survive disconnect**
When home Assistant disconnects while remote mode is enabled and the watchdog has not expired, `set_safe_modbus_registers` writes the existing safe settings but never disables `remote_mode`, so the inverter continues applying its last remote setpoints instead of those safe settings until the watchdog expires.
Disable `remote_mode` in the safe-mode sequence.
</issue_to_address>
### Comment 2
<location path="pv_inverter/packages/deye_hybrid_3p/remote.yaml" line_range="18" />
<code_context>
+ - platform: modbus_controller
+ use_write_multiple: true
+ modbus_controller_id: ${modbus_controller_id}
+ id: inverter_output_power_control_mode
+ name: "Inverter output power control mode"
+ address: 1104
</code_context>
<issue_to_address>
**Mode switches use stale setpoints**
When a client switches to per-phase control while changing its phase setpoints, or a Modbus write fails partway through those changes, `inverter_output_power_control_mode` can enable per-phase control before the separate phase setpoint writes complete, so the inverter applies previously stored phase values in the meantime; a dropped write leaves the new mode using a mixed set of values. Update the setpoints before switching modes, or provide a grouped update that switches modes last.
Apply all setpoints first and switch the active control mode last, ideally through one coordinated update.
Also at `pv_inverter/packages/deye_hybrid_3p/remote.yaml:117-164`.
</issue_to_address>
### Comment 3
<location path="pv_inverter/packages/deye_hybrid_3p/remote.yaml" line_range="68" />
<code_context>
+ id: battery_constant_voltage_mode
+ name: "Battery constant voltage mode"
+ min_value: 0
+ max_value: 1000
+ step: 0.1
+ unit_of_measurement: "V"
</code_context>
<issue_to_address>
**Unsupported battery targets are accepted**
When a caller sets the remote battery voltage above the applicable battery limit, the 0–1000 V range permits writing unsupported targets to register 1107; 3P LV controls are bounded to 38–61 V and HV controls to 710 V, so the inverter cannot apply an out-of-range remote setpoint reliably.
Constrain the accepted maximum to the applicable LV or HV battery-voltage limit.
Also at `pv_inverter/deye_hybrid_3p_lv.yaml:40`.
</issue_to_address>
### Comment 4
<location path="pv_inverter/packages/deye_hybrid_3p/remote.yaml" line_range="55-61" />
<code_context>
+ - platform: modbus_controller
+ use_write_multiple: true
+ modbus_controller_id: ${modbus_controller_id}
+ id: remote_mode_watchdog_time
+ name: "Remote mode watchdog time"
+ min_value: 10
+ step: 1
+ address: 1101
+ value_type: U_WORD
+
+ - platform: modbus_controller
+ use_write_multiple: true
</code_context>
<issue_to_address>
**Remote control times out**
When remote mode remains enabled past the configured watchdog interval without another write, the inverter's watchdog expires the remote-control session, so the configured remote mode stops applying and the inverter no longer follows the caller's setpoints.
Add a periodic refresh of the remote-control session before the watchdog expires, or make the expected refresh responsibility explicit to callers.
</issue_to_address>
### Comment 5
<location path="pv_inverter/packages/deye_hybrid_3p/remote.yaml" line_range="57" />
<code_context>
+ modbus_controller_id: ${modbus_controller_id}
+ id: remote_mode_watchdog_time
+ name: "Remote mode watchdog time"
+ min_value: 10
+ step: 1
+ address: 1101
</code_context>
<issue_to_address>
**Watchdog timeout is capped at 100 seconds**
When a caller needs a watchdog duration above 100 seconds, with `max_value` omitted, the number entity defaults to 100 even though register 1101 is a `U_WORD`, so callers cannot configure a longer watchdog timeout.
Set `max_value` to permit the longer watchdog durations supported by the register.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and this adds direct Modbus writes for inverter operating mode, battery voltage/current/SOC, and phase power limits. An incorrect register mapping or value could cause unwanted charging, discharging, power output, or an outage, and reverting the configuration would not necessarily restore values already written to the inverter.
Blocking findings: pv_inverter/packages/deye_hybrid_3p/remote.yaml:5, pv_inverter/packages/deye_hybrid_3p/remote.yaml:18, pv_inverter/packages/deye_hybrid_3p/remote.yaml:68, pv_inverter/packages/deye_hybrid_3p/remote.yaml:61, pv_inverter/packages/deye_hybrid_3p/remote.yaml:57
| - platform: modbus_controller | ||
| use_write_multiple: true | ||
| modbus_controller_id: ${modbus_controller_id} | ||
| id: inverter_output_power_control_mode |
There was a problem hiding this comment.
🟡 Medium · Mode switches use stale setpoints
When a client switches to per-phase control while changing its phase setpoints, or a Modbus write fails partway through those changes, inverter_output_power_control_mode can enable per-phase control before the separate phase setpoint writes complete, so the inverter applies previously stored phase values in the meantime; a dropped write leaves the new mode using a mixed set of values. Update the setpoints before switching modes, or provide a grouped update that switches modes last.
Apply all setpoints first and switch the active control mode last, ideally through one coordinated update.
Also at pv_inverter/packages/deye_hybrid_3p/remote.yaml:117-164.
Prompt for AI agents
In `pv_inverter/packages/deye_hybrid_3p/remote.yaml` at line 18:
**Mode switches use stale setpoints**
When a client switches to per-phase control while changing its phase setpoints, or a Modbus write fails partway through those changes, `inverter_output_power_control_mode` can enable per-phase control before the separate phase setpoint writes complete, so the inverter applies previously stored phase values in the meantime; a dropped write leaves the new mode using a mixed set of values. Update the setpoints before switching modes, or provide a grouped update that switches modes last.
Apply all setpoints first and switch the active control mode last, ideally through one coordinated update.
Also at `pv_inverter/packages/deye_hybrid_3p/remote.yaml:117-164`.There was a problem hiding this comment.
Yes, switching the control mode should be done such that the setpoints are written first and the mode change last. I'm not sure that's the job of this package. The question of a modbus write fail is interesting though, is there a way to block several register updates such that they all succeed or fail together?
There was a problem hiding this comment.
Not with the normal number and select entities. In the current file, use_write_multiple: true makes each individual entity use Modbus function 16, but it does not combine writes made to separate entities into one transaction. ESPHome documents this as selecting the multiple-register function for that component’s write, not as providing transaction or rollback semantics. (ESPHome documentation)
A custom ESPHome lambda/script can send one function-16 command containing the contiguous registers. This repository already uses that approach in time_sync.yaml. For this case, the safest sequence would be:
- Send registers
1105–1115together. - Only after that succeeds, write register
1104to change the active control mode.
That prevents a failed setpoint write from activating the new mode, although it still cannot guarantee rollback if the inverter applies the write and the response is lost. Modbus defines request/reply transactions and function 16, but does not provide a general multi-request commit/rollback mechanism; whether a single function-16 write is internally atomic is device-specific. (Modbus Application Protocol Specification)
For EMHASS, I would expose a single coordinated action/script for changing the mode and its setpoints, rather than relying on clients to update the independent entities in the correct order.
| id: battery_constant_voltage_mode | ||
| name: "Battery constant voltage mode" | ||
| min_value: 0 | ||
| max_value: 1000 |
There was a problem hiding this comment.
🟡 Medium · Unsupported battery targets are accepted
When a caller sets the remote battery voltage above the applicable battery limit, the 0–1000 V range permits writing unsupported targets to register 1107; 3P LV controls are bounded to 38–61 V and HV controls to 710 V, so the inverter cannot apply an out-of-range remote setpoint reliably.
Constrain the accepted maximum to the applicable LV or HV battery-voltage limit.
Also at pv_inverter/deye_hybrid_3p_lv.yaml:40.
Prompt for AI agents
In `pv_inverter/packages/deye_hybrid_3p/remote.yaml` at line 68:
**Unsupported battery targets are accepted**
When a caller sets the remote battery voltage above the applicable battery limit, the 0–1000 V range permits writing unsupported targets to register 1107; 3P LV controls are bounded to 38–61 V and HV controls to 710 V, so the inverter cannot apply an out-of-range remote setpoint reliably.
Constrain the accepted maximum to the applicable LV or HV battery-voltage limit.
Also at `pv_inverter/deye_hybrid_3p_lv.yaml:40`.
This branch has not been deployed
2 participants
Description
Add inverter remote mode modbus registers. I have tested this on my SUN-15K-SG01HP3-EU-AM2 and I'm now using it to control the inverter from my EMHASS optimization.
Changes relate to
I only have a 3P HV inverter but I suspect this is common for all inverter types.
Summary by Sourcery
New Features: