Skip to content

Relax voltage/current reporting for Sonoff S60 plugs - #885

Merged
TheJulianJES merged 4 commits into
zigpy:devfrom
TheJulianJES:tjj/sonoff-s60-em-reporting
Sep 2, 2026
Merged

Relax voltage/current reporting for Sonoff S60 plugs#885
TheJulianJES merged 4 commits into
zigpy:devfrom
TheJulianJES:tjj/sonoff-s60-em-reporting

Conversation

@TheJulianJES

@TheJulianJES TheJulianJES commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Proposed change

Quick, temporary ZHA-side workaround for the SONOFF S60ZBTPF / S60ZBTPG smart plugs spamming voltage (and current) reports.

These plugs report rms_voltage and rms_current with ac_voltage_divisor / ac_current_divisor of 100 (rms_voltage = 23384 is 233.84 V, verified on S60ZBTPG firmware 0x00001002). ZHA configures all electrical measurement attributes with a fixed raw reportable_change=1, which for this device is a 0.01 V / 0.01 A threshold. Mains voltage always drifts more than that, so the plug reports at every 5 second minimum interval.

This adds a model-matched VirtualEntity on the Electrical Measurement cluster, SonoffS60RelaxedElectricalMeasurementReporting, that uses the reporting override from #884 to configure rms_voltage with a raw change of 100 (1 V) and rms_current with 5 (0.05 A). Everything else (intervals, other attributes, other devices) is unchanged. It is a virtual entity, so nothing shows up in Home Assistant, no unique_id changes, and the device snapshots are untouched. It lives in zha/application/platforms/virtual.py next to the other manufacturer-specific bind/init virtual entities (SonoffManufacturerBind, TuyaPlugManufacturerInit, ...).

The new reporting config is applied on the next reconfigure or re-pair of the plug, as with any reporting change.

Additional information

This is deliberately the smallest possible fix so it can go into a patch release. It is a hardcoded device entry in ZHA, which is what quirks normally exist to avoid, so it is marked temporary and should be removed again once either of these lands. Note that overriding configs merge with each other (tightest wins), so when the quirks-side override lands this entry has to be removed in the same release rather than just being outvoted; with the divisor-aware route it merely becomes redundant.

Both need coordinated releases across ZHA and zha-quirks, which is why this ZHA-only patch comes first.

Unrelated to this change: the S60 is not in the ElectricalMeasurementReportingDevice model list, so the EM poller still polls it every 30 to 45 seconds. Whether to add it there is a separate decision.

Checklist

  • The changes are tested and work correctly
  • pre-commit checks pass
  • Tests have been added to verify that the new code works

The S60ZBTPF/S60ZBTPG report rms_voltage and rms_current with a divisor of
100, so ZHA's raw reportable change of 1 is 0.01 V / 0.01 A and the plug
reports at every 5 second minimum interval. Add a model-matched virtual
entity that overrides the reporting config with 1 V / 0.05 A.

Temporary until reportable changes are divisor-aware for all devices.
…sumption

Also rename the entity so it is not confused with the EM polling slot entity
above it.
Copilot AI lite review requested due to automatic review settings September 2, 2026 04:59
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.19%. Comparing base (8b7565b) to head (92f4194).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #885   +/-   ##
=======================================
  Coverage   97.19%   97.19%           
=======================================
  Files          57       57           
  Lines       10554    10560    +6     
=======================================
+ Hits        10258    10264    +6     
  Misses        296      296           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is narrowly scoped, uses the existing reporting override mechanism correctly, and includes a targeted test to prevent regressions.

Pull request overview

Implements a ZHA-side, model-specific reporting override to reduce excessive Electrical Measurement (EM) voltage/current reports from SONOFF S60 smart plugs that use a divisor of 100, preventing the default raw reportable_change=1 from causing near-continuous reporting at the 5-second minimum interval.

Changes:

  • Add a SONOFF S60–matched VirtualEntity that overrides EM rms_voltage/rms_current reporting thresholds to coarser raw deltas (100 and 5 respectively).
  • Add a regression test validating the relaxed reporting config is applied to S60 devices while other devices keep the default behavior.
File summaries
File Description
zha/application/platforms/sensor/init.py Adds a model/manufacturer-matched EM virtual entity to override reporting thresholds for SONOFF S60 voltage/current.
tests/test_sensor.py Adds a parameterized test asserting the S60 override is applied and defaults remain unchanged for other devices.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread zha/application/platforms/sensor/__init__.py Outdated

@zigpy-review-bot zigpy-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the change does what it says, and both magic numbers check out against the device snapshot.

Verified (11 checks)
  • Divisors: tests/data/devices/sonoff-s60zbtpg-0x00001002.json has ac_voltage_divisor = 100 and ac_current_divisor = 100 (ac_power_divisor = 1), so raw 100 = 1 V and raw 5 = 0.05 A. Snapshot values rms_voltage = 23903 / rms_current = 8 line up with 239.03 V / 0.08 A.
  • Only the threshold changes: the defaults being replaced are min_interval=5, max_interval=900, reportable_change=1 for both rms_voltage (sensor/init.py:1585) and rms_current (:1437), so the override keeps the intervals byte-identical and moves only reportable_change.
  • Merge semantics: AggregatedAttrConfig.merge (zha/zigbee/cluster_config.py:47-69) makes the first overriding config discard the merged defaults, and a later non-overriding config can no longer tighten it — the intended path here.
  • read_on_startup=False is harmless: it merges with or (cluster_config.py:45), so the default sensors' read_on_startup=True still wins and startup reads are unchanged.
  • bind=True is free: agg.bind = agg.bind or config.bind, and the EM sensor entities already request the bind.
  • No polling-slot contention: no feature_priority, so the new entity doesn't compete for PlatformFeatureGroup.EM_POLLING and ElectricalMeasurementPoller keeps the slot as before.
  • Not polled itself: AggregatedClusterPoller.async_update only walks Sensor siblings (sensor/init.py:962-964), and a VirtualEntity isn't one — so this entity's configs don't leak into the poll list.
  • Matches through the quirk: the test device (firmware 0x00001002) lands on the zhaquirks.sonoff.s60zbtpf variant that .replaces(SonoffS60ElectricalMeasurement), and the relaxed config still applies — the replaced cluster keeps the electrical_measurement ep_attribute, so the default match_renamed_clusters=False isn't a problem here.
  • Model list is complete: S60ZBTPF and S60ZBTPG are the only two S60 plug models in zigbee-herdsman-converters, and they are exactly the pair the quirk registers under manufacturer SONOFF — so the manufacturer filter can't desync from the quirk.
  • Snapshots really are untouched: no virtual entity appears anywhere under tests/data/devices (grep for em_poller / em_reporting_device returns nothing), so no regen is needed.
  • Local checks: in a worktree venv (i.e. with real zigpy types, not pre-commit's dependency-less run) mypy zha/ is clean, ruff check / ruff format --check are clean, and the new test plus all electrical-measurement/poller tests pass. CI is green on 3.12/3.13/3.14.

Corroborating from the other stack: Z2M doesn't use the standard EM cluster on the S60 at all — it reads voltage/current from the eWeLink manufacturer cluster 0xFC11, and the only reporting it configures there is current with a change of 2 at a 10 s minimum. So it independently steers away from fine-grained EM reporting on this device.

On whether to also add the S60 to ElectricalMeasurementReportingDevice (the open question at the end of the description): I'd leave the poller on. With the thresholds now at 1 V / 0.05 A and max_interval=900, a plug sitting at a steady load would only push a voltage/current update every 15 minutes once the poll went away — the 30-45 s poll is what keeps those two sensors looking alive. The traffic saved (one read plus one response per ~37 s) is smaller than that freshness cost, so the two changes are not really a package deal.

One thing that argues for the generic branch landing sooner rather than later: the control device in the new test, shelly-1pm.json, itself has ac_voltage_divisor = 100 — so it has the identical 0.01 V threshold, and asserting reportable_change == 1 for it is asserting the very behaviour this PR calls a bug. Not something to fix here, but it does mean the hardcoded list would otherwise grow device by device.

Two optional notes inline; nothing blocking.

Comment thread zha/application/platforms/sensor/__init__.py Outdated
Comment thread tests/test_sensor.py Outdated
…s in the test

It is device-specific cluster setup like the other manufacturer virtual
entities, not tied to the sensor platform. Note in the docstring that it has
to be removed when the quirk declares its own override, since overriding
configs merge with each other, and assert the divisors the raw values assume.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The scoped override is correctly model-matched, preserves other reporting settings, and has focused regression coverage.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@zigpy-review-bot zigpy-review-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at e9f44fe. Both notes from the previous round are properly addressed, and the move to virtual.py is behaviour-neutral — approving again.

Verified (9 checks)
  • The delta is a pure move plus the two fixes: diffing 981c610..e9f44fe, the class body is byte-identical apart from the docstring, so nothing about the matching or the config changed with the relocation.
  • The docstring now matches merge's actual semantics: AggregatedAttrConfig.merge (zha/zigbee/cluster_config.py:50-70) takes the replace path only for the first reporting_override=True config; a second one falls through to the min() branch. "Overriding configs are merged with each other (tightest wins), so this must be removed in the same release the S60 quirk starts declaring its own reporting override" is exactly right.
  • The test now pins the divisors: the parametrisation carries (100, 100) for the S60 and (100, 1000) for the Shelly and asserts both against the cluster, so raw 100 / 5 can no longer silently stop meaning 1 V / 0.05 A.
  • Discovery still finds it in virtual.py: register_entity writes into the same ENTITY_REGISTRY regardless of module, and both parametrised cases pass — the S60 gets the relaxed config, the Shelly keeps reportable_change=1.
  • The match survives the quirk: the test device (firmware 0x00001002) takes the zhaquirks.sonoff.s60zbtpf variant that .replaces(SonoffS60ElectricalMeasurement); manufacturer/model stay SONOFF / S60ZBTPG in the snapshot, and the replaced cluster keeps the electrical_measurement ep_attribute, so the default match_renamed_clusters=False isn't a problem here.
  • The model list still matches the quirk exactly: the quirk registers QuirkBuilder("SONOFF", "S60ZBTPF").applies_to("SONOFF", "S60ZBTPG") — the same pair under the same manufacturer, so the two can't desync.
  • No conflict today: the quirk declares no reporting config of its own yet (no configures_reporting / reporting_override in zhaquirks/sonoff/s60zbtpf.py), so the removal note is forward-looking rather than describing a live clash.
  • Snapshots really are still untouched: no virtual entity appears anywhere under tests/data/devices, and git status on that tree is clean at this head — no regen needed.
  • Local checks: in a worktree venv (i.e. with real zigpy types, not pre-commit's dependency-less run) mypy zha/ is clean, ruff check and ruff format --check are clean, and tests/test_discover.py plus tests/test_sensor.py pass in full (998 passed).

One thing the new divisor assertions make nicely visible: the control case asserts (100, 1000), so the Shelly 1PM carries the same ac_voltage_divisor of 100 — the identical 0.01 V threshold — and the test now records that in the very place it asserts reportable_change == 1 for it. That is a fair argument for the divisor-aware branch landing sooner rather than later; nothing to change here.

My answer to the ElectricalMeasurementReportingDevice question from the last round stands unchanged: I'd leave the poller on for the S60, since with max_interval=900 and the relaxed thresholds the 30-45 s poll is what keeps those two sensors looking alive.

One optional note inline; nothing blocking.

Comment thread zha/application/platforms/virtual.py Outdated
… section

Only the aggregated cluster poller reads _cluster_id; a plain VirtualEntity
matches via _cluster_match alone. Electrical Measurement is a standard ZCL
cluster, so the entity does not belong under the manufacturer-specific
clusters header.
@TheJulianJES
TheJulianJES requested a balanced review from Copilot September 2, 2026 05:35
@TheJulianJES TheJulianJES changed the title Relax voltage/current reporting for SONOFF S60 plugs Relax voltage/current reporting for Sonoff S60 plugs Sep 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The narrowly scoped override is correctly model-matched and adequately tested without affecting unrelated attributes or devices.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@TheJulianJES
TheJulianJES merged commit 6660343 into zigpy:dev Sep 2, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants