Skip to content

Clean up number platform #442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 2, 2025
Merged

Clean up number platform #442

merged 9 commits into from
May 2, 2025

Conversation

puddly
Copy link
Contributor

@puddly puddly commented Apr 30, 2025

Should resolve zigpy/zha-device-handlers#3821 (comment).

  • I've reorganized the platform code to use a BaseNumber entity, with subclasses implementing native_value and async_set_native_value.
  • All entity metadata is set in recompute_capabilities().
  • _init_from_quirks_metadata is implemented on NumberConfigurationEntity. Should we move it up in the hierarchy?
  • We use fallback_name instead of description for standardization.

@puddly
Copy link
Contributor Author

puddly commented Apr 30, 2025

For Core, this should get rid of the name override by setting fallback_name:

diff --git a/homeassistant/components/zha/number.py b/homeassistant/components/zha/number.py
index 567e2a5b37a..401ffc39651 100644
--- a/homeassistant/components/zha/number.py
+++ b/homeassistant/components/zha/number.py
@@ -46,17 +46,6 @@ async def async_setup_entry(
 class ZhaNumber(ZHAEntity, RestoreNumber):
     """Representation of a ZHA Number entity."""
 
-    @property
-    def name(self) -> str | UndefinedType | None:
-        """Return the name of the number entity."""
-        if (description := self.entity_data.entity.description) is None:
-            return super().name
-
-        # The name of this entity is reported by the device itself.
-        # For backwards compatibility, we keep the same format as before. This
-        # should probably be changed in the future to omit the prefix.
-        return f"{super().name} {description}"
-
     @property
     def native_value(self) -> float | None:
         """Return the current value."""

Copy link

codecov bot commented Apr 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.92%. Comparing base (37778b0) to head (eb15e11).
Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #442      +/-   ##
==========================================
+ Coverage   96.88%   96.92%   +0.03%     
==========================================
  Files          63       63              
  Lines       10404    10362      -42     
==========================================
- Hits        10080    10043      -37     
+ Misses        324      319       -5     

☔ View full report in Codecov by Sentry.
📢 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.

@puddly puddly marked this pull request as ready for review May 1, 2025 15:55
Comment on lines 271 to 277
@property
def state(self) -> dict[str, Any]:
"""Return the state of the entity."""
Copy link
Contributor

Choose a reason for hiding this comment

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

This can now be removed from NumberConfigurationEntity, as it's already implemented in BaseNumber.

@@ -119,59 +96,92 @@ def state(self) -> dict[str, Any]:
response["state"] = self.native_value
return response
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda related: Should we include the min/max value attributes (and possibly others) in state?
state is only really important for maybe_emit_state_changed_event. If only the min/max values were to change, the method would prevent an update from going through, unless native_value is also changed at the same time.
This would matter if we were to "dynamically" call recompute_capabilities (i.e. after sw updates) in the future.

For the analog input entity, if min_present_value/max_present_value ZCL attributes were updated at runtime before these changes (whilst native_value is also changed due to what I mentioned above), the update of the min/max values would populate to HA as well.
Looking at this now, it would only happen after recompute_capabilities is called, but that's not done for a simple attribute change, so we always prevent min/max changes for analog input entities going to HA now, I think.

In reality, I doubt any devices/quirks exposing an AnalogInput cluster did this before, so it might not be an issue.

The same would also apply for the entities that useZCLHeatSetpointLimitEntity (i.e. MaxHeatSetpointLimit and MinHeatSetpointLimit), as they overrode the methods HA called for min/max values, when HA state is written. Now it's also only done in recompute_capabilities, so not possible "at runtime" (after the device is initialized).
Doubt that matters as well though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's true. I think we maybe could emit a new event when capabilities change. I'd like to eventually figure out
info_object in preparation for the client/server split to allow devices to emit both types of updates in a more type-checked manner.

Copy link
Contributor

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

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

Seems good, but note the comment below about changing the discovery for v2 quirks to NumberConfigurationEntity.

@TheJulianJES
Copy link
Contributor

TheJulianJES commented May 2, 2025

_init_from_quirks_metadata is implemented on NumberConfigurationEntity. Should we move it up in the hierarchy?

To BaseNumber? Not really, as we should never need to initialize the AnalogInput only Number class with quirks v2 metadata.

But we should update the discovery for quirks v2 to always initialize NumberConfigurationEntity.
Since we'll never need to initialize the Number class for quirks v2 (as that's now only for AnalogInput entities).

Platform.NUMBER,
NumberMetadata,
EntityType.CONFIG,
): number.NumberConfigurationEntity,
(
Platform.NUMBER,
NumberMetadata,
EntityType.DIAGNOSTIC,
): number.Number,
(
Platform.NUMBER,
NumberMetadata,
EntityType.STANDARD,
): number.Number,

Maybe we should also rename Number class to AnalogInputNumber?

Actually, we should be able to completely remove the STANDARD/CONFIG/DIAGNOSTIC type from the discovery schema IMO.

@puddly puddly force-pushed the puddly/clean-up-number branch from bd10579 to 947fdca Compare May 2, 2025 19:02
@puddly
Copy link
Contributor Author

puddly commented May 2, 2025

Good point. I've implemented the above change and will make a follow-up to remove the unnecessary entity type handling in the mapping.

@puddly puddly merged commit 758d8b1 into zigpy:dev May 2, 2025
9 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.

2 participants