Repository navigation
Fix/1p ac power scaling - #123
Conversation
|
O.O nice. I think we need to do revalidation for new modbus version protocol. |
| register_type: holding | ||
| address: 167 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
There was a problem hiding this comment.
this hole if statement looks wrong. The documentation is garbage but from my understanding register 54 hold binary 1 (decimal 1) or binary 10 (decimal 2)(I may be wrong). So this if is always true and you return "x". Also even if you change if condition, then you still have error in x * id(ac_power_ratio).state. You are multiplying X by decimal 2 or 1. So you will return x or 2x.
Also try to use substitutions: to hold this lambda expression.
There was a problem hiding this comment.
Oh only std::isnan(id(ac_power_ratio).state) is a condition. that change everything. x will never be use and multiplication will be always use. IF register 54 is not use then it will be probably set to 0 or other random value.
There was a problem hiding this comment.
this hole if statement looks wrong. The documentation is garbage but from my understanding register 54 hold binary 1 (decimal 1) or binary 10 (decimal 2)(I may be wrong). So this if is always true and you return "x". Also even if you change if condition, then you still have error in x * id(ac_power_ratio).state. You are multiplying X by decimal 2 or 1. So you will return x or 2x.
Also try to use substitutions: to hold this lambda expression.
Good catch on the single-condition logic, you're right that once reg 54 reads, the multiply always applies, so a bad value there would corrupt every AC power sensor. But I raw-polled reg 54 on my SG02LP1 to check the value:
Request 01 03 0036 0001 9404 → Response 01 03 02 000A 3843 = 0x000A = 10 decimal.
So it's decimal 10, not binary, the AC power registers really are 10 W/LSB on this unit, and the ×10 is needed (without it grid/load power reads 1/10th).
Worth noting sunsynk hardcodes ×1 for single-phase, which would under-report 10× on these, so reading the ratio is actually more correct than the static map.
To cover the risk you flagged for firmwares where reg 54 isn't a valid ratio, I'll guard it so it only scales on a known value and passes through otherwise, and hoist it into a substitution as you suggested:
option 1:
substitutions:
ac_power_scale: >-
const float r = id(ac_power_ratio).state;
return (r == 1.0f || r == 10.0f) ? x * r : x;
or
option 2:
ac_power_scale: >-
const float r = id(ac_power_ratio).state;
return (r == 10.0f) ? x * 10.0f : x;
i think option 2 is better.
I'll also add a protocol-version sensor (reg 2, decoded high.low) so we can tie reg-54 behaviour to firmware versions across reporters.
Out of curiosity, I checked the same register on another inverter. A noark Ex9H-DH-6KS-AU which is just an australian rebranded deye infact its a Deye SUN-6K-SG04LP1-AU, and that one returned 0 for register 54.
Request 01 03 0036 0001 9404 → Response 01 03 02 0000 B844 = 0x0000 = 0 decimal.
So ill need to do some testing on that inverter.
There was a problem hiding this comment.
Second option look fine but make it oneline: return (id(ac_power_ratio).state == 10.0f) ? x * 10.0f : x;.
If this register is equal to 0 it will use X (1W). it is correct right?
There was a problem hiding this comment.
Correct and now confirmed on hardware rather than assumed. I tested a second inverter (Noark Ex9H-DH-6KS-AU, a rebranded Deye SUN-6K-SG04LP1-AU): register 54 returns 0 on it, and I compared its AC power registers against the inverter display, they're already in 1 W units. So falling through to the raw value on anything other than 10 is the right behaviour.
That gives us both cases verified with real data:
Inverter Reg 54 Power units Lambda result
SG02 10 kW 10 10 W/LSB × 10
SG04 6 kW 0 1 W/LSB pass through
Updated in 0456277:
One-line guarded lambda as you suggested: return (id(ac_power_ratio).state == 10.0f) ? x * 10.0f : x;
this also covers the not-yet-read startup state without a separate isnan check, since NaN == 10.0f is false.
Hoisted into a substitutions: entry (same pattern as tou.yaml) so the nine sensors share one definition.
| register_type: holding | ||
| address: 168 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| register_type: holding | ||
| address: 169 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| register_type: holding | ||
| address: 170 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| register_type: holding | ||
| address: 171 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| register_type: holding | ||
| address: 172 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| register_type: holding | ||
| address: 176 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| register_type: holding | ||
| address: 177 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| register_type: holding | ||
| address: 178 | ||
| filters: | ||
| - lambda: 'return std::isnan(id(ac_power_ratio).state) ? x : x * id(ac_power_ratio).state;' |
| modbus_controller_id: ${modbus_controller_id} | ||
| name: "AC Power Ratio" | ||
| id: ac_power_ratio | ||
| register_type: holding |
There was a problem hiding this comment.
change register type to read only
There was a problem hiding this comment.
Isn't it a holding register?
modbus 0x03? is to read from the holding register
There was a problem hiding this comment.
In ESPHome's modbus_controller, register_type: read doesn't mean "read-only", it means input registers, read with Modbus function 0x04. Deye exposes everything as holding registers (function 0x03): my raw poll 01 03 0036 0001 used 0x03 and got an answer.
Plus in the entire repo, every single sensor in both the 1P and 3P packages uses register_type: holding. Switching register 54 to read would actually break it (the inverter likely won't answer 0x04).
A sensor never writes regardless, so there's no write-safety concern.
Register_type: read = FC 0x04 input registers, which Deye doesn't serve (atleast according to the documentation), and that it would be inconsistent with every other sensor in the repo.
There was a problem hiding this comment.
Ok you are right about register type. Instead make it: internal: true
There was a problem hiding this comment.
Ok you are right about register type. Instead make it: internal: true
I think it would be useful to keep it shown. It would be useful in the diagnostic section like this:
Setting it to internal: true will hide it, which doesnt really provide any benefit that i can see.
There was a problem hiding this comment.
It never change and it is related to modbus version. In older release this register address is use as a different parameter. It may confuse other people. Also it is use to change values of other sensor. So it role is done and nothing more is needed.
There was a problem hiding this comment.
Ahh okay I understand. I will update it accordingly.
523eb88 to
5f1ec1e
Compare
…ubstitution Per PR Lewa-Reka#123 review: guard the register 54 multiply so it only applies on a verified 10W/LSB value and passes raw watts through otherwise. Confirmed on hardware: SG02 10kW reports ratio 10 (10W units), SG04 6kW reports 0 (1W units). NaN == 10.0f is false, so the not-yet-read state needs no isnan check. Lambda hoisted into a substitution following the tou.yaml pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ubstitution Per PR Lewa-Reka#123 review: guard the register 54 multiply so it only applies on a verified 10W/LSB value and passes raw watts through otherwise. Confirmed on hardware: SG02 10kW reports ratio 10 (10W units), SG04 6kW reports 0 (1W units). NaN == 10.0f is false, so the not-yet-read state needs no isnan check. Lambda hoisted into a substitution following the tou.yaml pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
We need protocol validation here: I just looked in to the protocol version pdf for v117 and v119 and i see differences that are not acceptable: 
|
| modbus_controller_id: ${modbus_controller_id} | ||
| name: "AC Power Ratio" | ||
| id: ac_power_ratio | ||
| register_type: holding |
There was a problem hiding this comment.
Ok you are right about register type. Instead make it: internal: true
Per the V119 protocol, the AC power registers 167-169 (grid), 170-172 (external CT) and 176-178 (load) are reported in units of register 54 (1W or 10W per LSB). Read register 54 as an "AC Power Ratio" sensor and multiply those power sensors by it, so values match the inverter display (e.g. External CT Power 875 -> 8750 when the ratio is 10). Falls back to the raw value while the ratio is unread. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ubstitution Per PR Lewa-Reka#123 review: guard the register 54 multiply so it only applies on a verified 10W/LSB value and passes raw watts through otherwise. Confirmed on hardware: SG02 10kW reports ratio 10 (10W units), SG04 6kW reports 0 (1W units). NaN == 10.0f is false, so the not-yet-read state needs no isnan check. Lambda hoisted into a substitution following the tou.yaml pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a13f488 to
8ffd334
Compare
…ubstitution Per PR Lewa-Reka#123 review: guard the register 54 multiply so it only applies on a verified 10W/LSB value and passes raw watts through otherwise. Confirmed on hardware: SG02 10kW reports ratio 10 (10W units), SG04 6kW reports 0 (1W units). NaN == 10.0f is false, so the not-yet-read state needs no isnan check. Lambda hoisted into a substitution following the tou.yaml pattern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@sourcery-ai review |
Reviewer's GuideCorrects 1P grid, external CT, and load power sensors by reading register 54 and scaling values by 10 only when the inverter reports 10 W per register unit; the ratio is exposed diagnostically and the protocol reference is added. Sequence diagram for 1P AC power scalingsequenceDiagram
participant Inverter
participant ModbusController
participant ACPowerRatio
participant PowerSensors
ModbusController->>Inverter: Read holding register 54
Inverter-->>ModbusController: ACpowerratio
ModbusController->>ACPowerRatio: Update ac_power_ratio
ModbusController->>Inverter: Read registers 167-172 and 176-178
Inverter-->>ModbusController: Raw AC power values
PowerSensors->>ACPowerRatio: Read state
alt ac_power_ratio == 10
PowerSensors->>PowerSensors: Multiply value by 10
else ratio is 1, 0, unsupported, or not yet read
PowerSensors->>PowerSensors: Keep raw value
end
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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pv_inverter/packages/deye_hybrid_1p/grid.yaml" line_range="19" />
<code_context>
+ # Register 54 = "AC power ratio". Only scale when it reads a known 10W/LSB
+ # value; pass raw watts through otherwise (covers 1, 0/unsupported and the
+ # not-yet-read NaN state).
+ func_scale_ac_power: "return (id(ac_power_ratio).state == 10.0f) ? x * 10.0f : x;"
+
globals:
</code_context>
<issue_to_address>
**AC power reads are under-scaled**
When register 54 fails or its initial read completes after AC power reads succeed on a 10 W/LSB inverter, `func_scale_ac_power` returns raw `x` while `ac_power_ratio` is unread or invalid, so affected power sensors publish values 10× too low until a valid ratio read and power poll succeed.
Handle an unread or failed ratio separately from a genuine non-10 ratio so 10 W/LSB power is not published unscaled.
Also at `pv_inverter/packages/deye_hybrid_1p/load.yaml:19`.
</issue_to_address>
### Comment 2
<location path="pv_inverter/packages/deye_hybrid_1p/grid.yaml" line_range="44" />
<code_context>
+ accuracy_decimals: 0
+ entity_category: diagnostic
+ icon: "mdi:multiplication"
+ internal: true
+
- platform: modbus_controller
</code_context>
<issue_to_address>
**Ratio is hidden from Home Assistant**
When a Home Assistant user or automation tries to inspect the documented AC Power Ratio entity, `internal: true` keeps ESPHome from exposing the sensor to Home Assistant, so users and automations cannot inspect the ratio or applied factor.
Remove `internal: true` from the AC Power Ratio sensor.
Also at `pv_inverter/packages/deye_hybrid_1p/grid.yaml:45`.
</issue_to_address>Sourcery assessment
Approval pending. 2 findings to address first.
Blocking findings: pv_inverter/packages/deye_hybrid_1p/grid.yaml:19, pv_inverter/packages/deye_hybrid_1p/grid.yaml:44
| # Register 54 = "AC power ratio". Only scale when it reads a known 10W/LSB | ||
| # value; pass raw watts through otherwise (covers 1, 0/unsupported and the | ||
| # not-yet-read NaN state). | ||
| func_scale_ac_power: "return (id(ac_power_ratio).state == 10.0f) ? x * 10.0f : x;" |
There was a problem hiding this comment.
🟡 Medium · AC power reads are under-scaled
When register 54 fails or its initial read completes after AC power reads succeed on a 10 W/LSB inverter, func_scale_ac_power returns raw x while ac_power_ratio is unread or invalid, so affected power sensors publish values 10× too low until a valid ratio read and power poll succeed.
Handle an unread or failed ratio separately from a genuine non-10 ratio so 10 W/LSB power is not published unscaled.
Also at pv_inverter/packages/deye_hybrid_1p/load.yaml:19.
Prompt for AI agents
In `pv_inverter/packages/deye_hybrid_1p/grid.yaml` at line 19:
**AC power reads are under-scaled**
When register 54 fails or its initial read completes after AC power reads succeed on a 10 W/LSB inverter, `func_scale_ac_power` returns raw `x` while `ac_power_ratio` is unread or invalid, so affected power sensors publish values 10× too low until a valid ratio read and power poll succeed.
Handle an unread or failed ratio separately from a genuine non-10 ratio so 10 W/LSB power is not published unscaled.
Also at `pv_inverter/packages/deye_hybrid_1p/load.yaml:19`.| accuracy_decimals: 0 | ||
| entity_category: diagnostic | ||
| icon: "mdi:multiplication" | ||
| internal: true |
There was a problem hiding this comment.
🟡 Medium · Ratio is hidden from Home Assistant
When a Home Assistant user or automation tries to inspect the documented AC Power Ratio entity, internal: true keeps ESPHome from exposing the sensor to Home Assistant, so users and automations cannot inspect the ratio or applied factor.
Remove internal: true from the AC Power Ratio sensor.
Also at pv_inverter/packages/deye_hybrid_1p/grid.yaml:45.
Prompt for AI agents
In `pv_inverter/packages/deye_hybrid_1p/grid.yaml` at line 44:
**Ratio is hidden from Home Assistant**
When a Home Assistant user or automation tries to inspect the documented AC Power Ratio entity, `internal: true` keeps ESPHome from exposing the sensor to Home Assistant, so users and automations cannot inspect the ratio or applied factor.
Remove `internal: true` from the AC Power Ratio sensor.
Also at `pv_inverter/packages/deye_hybrid_1p/grid.yaml:45`.
… 3389-1515) New entry point pv_inverter/deye_hybrid_1p_lv_v118.yaml next to the existing 1P one, so users on older firmwares are unaffected. Reuses inverter/load/pv from deye_hybrid_1p; includes the AC power ratio scaling (Lewa-Reka#123), External CT Power state_class (Lewa-Reka#152) and Battery Current scale (Lewa-Reka#128) fixes. Shared-register switches (248, 274-279, 280, 326) write via read-modify-write so toggling one bit no longer clears the others.
3 participants
Description
Fixes incorrect AC power readings on the single-phase (1P) integration.
On inverters where the "AC power ratio" register (register 54) is set to 10, the inverter reports all AC power values in 10 W units instead of 1 W. The integration read these registers as raw watts, so every AC power sensor was 10× too low — e.g. External CT Power showed 875 W while the inverter's own screen showed 8750 W.
Per the V119 Modbus protocol, register 54 (ACpowerratio, value 1 or 10) sets the unit (1 W or 10 W per LSB) for power registers 167,168,169,170,171,172,176,177,178. This PR reads register 54 and multiplies those sensors by it.
Changes relate to
Changes
New diagnostic sensor "AC Power Ratio" (register 54).
Multiply the affected 1P power sensors by that ratio (filter falls back to the raw value until the ratio is first read):
Grid L1 / L2 / Total Power (167 / 168 / 169) — grid.yaml
External CT L1 / L2 / Total Power (170 / 171 / 172) — grid.yaml
Load L1 / L2 / Total Power (176 / 177 / 178) — load.yaml
Added the Deye 1P V119 Modbus protocol document under docs/modbus_communication_protocols/ as the source for this behaviour.
Notes
Backward compatible: inverters with register 54 = 1 multiply by 1 (no change).
Registers not flagged as 10 W in the protocol (inverter output power 173–175, battery power 190, etc.) are intentionally left unscaled.
1P only
How it was verified
Hardware with register 54 = 10:
External CT Power now reads 8750 W, matching the inverter display (esphome was saying 875 W before); the other Grid/Load power sensors scale accordingly.
The new AC Power Ratio entity reads 10 (or 1), so the applied factor is visible.
Summary by Sourcery
Scale affected Deye 1P AC power readings according to the inverter’s configured power ratio while preserving existing readings for standard units.
New Features:
Bug Fixes: