Use DataUpdateCoordinator in Vallox #56966
Conversation
|
Hi @bdraco, are you planning to look at this PR or was your comment contained to the startup issue only? Just trying to understand if I should request somebody else for review of the whole thing 👍 |
|
I have ~45 review requests ahead of this one. |
|
Failing in unrelated test. |
|
Hi @bdraco, thank you for your review, really appreciated!
These are in |
Maybe something like this will work (untested) bdraco@7c1c05f |
| def check_and_convert(value: StateType) -> int | None: | ||
| if isinstance(value, (int, float)): | ||
| return int(value) |
There was a problem hiding this comment.
Please pull this out into a utility function. We don't want to embed functions if we can avoid it as it makes them more expensive to execute.
Also please name it to show intent
Looks a lot better. Small comment above to reduce the overhead and avoid the embedded function. |
👍 |
|
Addressed both 👍 Out of curiosity: Why did you opt for the type annotation with the subclassing, instead of using the generic type parameter? |
For consistency: We don't use the original method anywhere else in the codebase for the coordinator, and I think its better to be more explicit that the type is for the data |
| # Hence, wait for the started event before doing a first data refresh and loading the platforms, | ||
| # because it usually means the system is less busy after the event and can now meet the | ||
| # websocket timing requirements. | ||
| hass.bus.async_listen_once( |
There was a problem hiding this comment.
In the future by converting this to a config entry, you can do await coordinator.async_config_entry_first_refresh() which will automatically retry later if the device isn't ready
| """Set up the client and boot the platforms.""" | ||
| conf = config[DOMAIN] | ||
| host = conf.get(CONF_HOST) | ||
| name = conf.get(CONF_NAME) |
There was a problem hiding this comment.
Host is required and name has a default value. Please use dict[key] when we know the key is in the dict.
| if metric_key not in vlxDevConstants.__dict__: | ||
| raise KeyError(f"Unknown metric key: {metric_key}") | ||
| _LOGGER.debug("Metric key invalid: %s", metric_key) |
There was a problem hiding this comment.
We don't want side effects like logging in entity state properties. get_metric is called from is_on eg.
| description="fan_speed_away", metric_key=METRIC_KEY_PROFILE_FAN_SPEED_AWAY | ||
| ), | ||
| ExtraStateAttributeDetails( | ||
| description="fan_speed_boost", metric_key=METRIC_KEY_PROFILE_FAN_SPEED_BOOST |
There was a problem hiding this comment.
What's the set value of these extra attributes?
There was a problem hiding this comment.
Not sure I understand the question. This Tulpe is iterated over down in the extra state attributes member function.
There was a problem hiding this comment.
Yes. What's the measurement type?
There was a problem hiding this comment.
Integer, representing percent speed.
| def native_value(self) -> StateType: | ||
| """Return the value reported by the sensor.""" | ||
| if (metric_key := self.entity_description.metric_key) is None: | ||
| _LOGGER.debug("Error updating sensor. Empty metric key") |
There was a problem hiding this comment.
Please move or remove the logging.
Breaking change
Proposed change
This PR replaces the
State Proxywith aDataUpdateCoordinator, which allows to reduce the integration's code size by ~90 lines.By staying true to the intended design of the
DataUpdateCoordinator, we get a slight change in how a certain error case is handled:If a state query to the Vallox device was successful, but one of the many metrics, or the fan profile (aka preset), isn't present at all or in an unexpected form, the integration now returns
Nonefor the respective entity's value or attribute, instead of setting the whole of theentity.availableto false.However, I haven't seen this corner case happening yet once. Usually, the Vallox device either responds to websocket requests, which means we get updates for all the metrics and the profile, or it doesn't respond. In the latter case, even with the
DataUpdateCoordinator, we are still setting any dependent entity's availability to false.Therefore, I didn't declare this PR as breaking, since I am not sure if this corner case of an error ever happens in reality. Even if it does, I don't think any existing installation will suffer from this change.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: