Repository navigation
Base valve platform - #909
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical primary-entity election issue remains unresolved.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds an abstract ZHA valve platform API for future device-handler implementations.
Changes:
- Defines valve classes and feature flags.
- Adds base valve state and control interfaces.
- Adds tests for valve behavior and capabilities.
| File | Summary |
|---|---|
zha/application/platforms/valve/const.py |
Valve constants and feature flags |
zha/application/platforms/valve/__init__.py |
Abstract valve entity implementation |
tests/test_platform_valve.py |
Valve state and capability tests |
Blocking issue: BaseValve needs a positive primary weight so valve-only devices can elect a primary entity.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #909 +/- ##
==========================================
+ Coverage 97.19% 97.21% +0.01%
==========================================
Files 57 59 +2
Lines 10561 10617 +56
==========================================
+ Hits 10265 10321 +56
Misses 296 296 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Looks good — a clean abstract base whose state and constants line up with HA's ValveEntity, following the same shape as the event base from #864. Nothing blocking; a few optional suggestions for the subclass API below.
Optional
- Position-reporting valves have to re-implement things HA derives from the position.
is_closed,async_open_valveandasync_close_valveare all abstract, but HA computes the closed state fromcurrent_valve_position == 0wheneverreports_positionis set, and routes open/close toasync_set_valve_position(100/0)wheneverSET_POSITIONis supported. HA'sasync_togglestill readsis_closed, though, so a positional subclass that leaves itNone(asFakePositionValvedoes —current_position=40alongsideis_closed=None) gets a toggle that ignores the actual position. Consider concrete defaults inBaseValvefor those cases (is_closed→Noneif the position is unknown, elseposition == 0, whenreports_positionis set; open/close →async_set_valve_position(100/0)whenSET_POSITIONis supported), still overridable. Anis_closedassertion intest_position_valve_statewould cover it. - Nothing ties the advertised capabilities to the implementation: a subclass can set
SET_POSITION/STOPwithout overriding the matching method (surfaces asNotImplementedErroron the first service call), orreports_position = Truewithout overridingcurrent_valve_position(the valve staysunknownin HA). This matches HA's own base class, so it's a judgment call, but a constructor check — like the doorbellringguardBaseEvent.__init__got in #864 — would catch quirk mistakes at entity creation instead of at runtime. - Question:
_attr_primary_weight = 10ties with the defaultSwitchon theOnOffcluster (also 10), and a tie elects no primary entity (Device._compute_primary_entity). Is the expectation that valve quirks always suppress the default switch (or set_attr_primary/ a different weight)? If so, that's worth a line in the base class docstring for quirk authors. - Nit: #864 also added
eventto the platform imports andPlatform.EVENTtoPLATFORMSinzha/application/discovery.py.PLATFORMShas no consumer in zha or HA core and the base registers no entities, so this is consistency only.
Note: HA's zha integration has no valve platform yet, so valve entities created by quirks won't show up in HA until a paired core change adds a valve platform (an adapter like the existing cover.py) to the ZHA integration. This is expected for a base-only PR, as with event (home-assistant/core#183109).
Verified (5 checks)
ValveDeviceClass/ValveEntityFeaturevalues match HA'shomeassistant/components/valve/const.pyexactly (water/gas; 1/2/4/8);ValveEntityStatefields map ontoValveEntity's properties.Platform.VALVEalready exists inzha/application/__init__.py; the base registers no entities, so there's no discovery or diagnostics-snapshot impact.tests/test_platform_valve.pypasses locally; mypy (worktree venv with real deps) and ruff are clean; CI is green, and Codecov reports all modified lines as covered.- Copilot's primary-weight thread is addressed by 9e9ea95 (
_attr_primary_weight = 10, same as cover, fan, light, switch and climate). - No overlapping open zha PRs; #673 (superseded) is closed.

Supersedes #673, which has gotten stale and needed a rewrite to account for the current ZHA APIs.
As with the other new platforms, this is only an abstract base. Device support will need to be implemented in https://github.com/zigpy/zha-device-handlers/. The ZCL spec doesn't have a concept of valves (only
OnOffandLevelControlclusters without a distinguishing device type) so this platform cannot participate in ZCL discovery.Architecturally, the ZCL OnOff cluster has a safety feature that I think HA Core would really benefit from:
on_with_timed_off. Feel free to express your support in my architecture proposal, the more the better 😄: home-assistant/architecture#1491