Add nobo_hub dynamic temperature controls #145157
10000Conversation
As implemented upstream in pynobo.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @echoromeo, @oyvindwe, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Implemented and tested now! Also added a very basic unit test. |
There was a problem hiding this comment.
Nice! Only some nitpicking from me.
I also have tested the feature. Unfortunately, I my thermostats are either only manual or support both eco and comfort, so I am not able to test the case for only one temperature to be controlled.
My entities shows up as expected
|
I am not sure if this should be considered a breaking change or not, as any automation setting the Personally, I'm in favour if the proposed change in the current form! |
|
Made all requested changes now! I agree this is not very likely to be breaking in practice.
All my heaters have NCU-1R recievers that only support eco, so then we've at least covered the known cases. |
Use bitwise-and to check feature flags.
| self._supports_comfort = any( | ||
| component["model"].supports_comfort | ||
| for component in hub.components.values() | ||
| if component["zone_id"] == zone_id | ||
| ) | ||
| self._supports_eco = any( | ||
| component["model"].supports_eco | ||
| for component in hub.components.values() | ||
| if component["zone_id"] == zone_id | ||
| ) |
There was a problem hiding this comment.
Instead, create a property that returns this, instead of iterating twice
There was a problem hiding this comment.
Thanks for the review! Pushed a new implementation with properties, though please let me know if you had something else in mind.
| if self._supports_comfort and self._supports_eco: | ||
| self._attr_supported_features |= ( | ||
| ClimateEntityFeature.TARGET_TEMPERATURE_RANGE | ||
| ) | ||
| elif self._supports_comfort or self._supports_eco: | ||
| self._attr_supported_features |= ClimateEntityFeature.TARGET_TEMPERATURE |
There was a problem hiding this comment.
Changed these, but whether the new version is simpler depends a lot on what you mean by "simplified". Shorter? More verbose? More efficient?
| } | ||
|
|
||
| # Both comfort and eco supported | ||
| zone = NoboZone("zone_1", mock_hub, "override_type") |
There was a problem hiding this comment.
We don't want to create entities in the tests, we want to mock out the library and have HA create the entities for us and then check their state through the state machine
There was a problem hiding this comment.
Makes sense, working on it.
There was a problem hiding this comment.
This will take time. There is no way to do what you're asking without writing tests way beyond the scope of my PR.
Proposed change
Add dynamic temperature controls to Nobø Ecohub integration to properly support recievers like NCU-1R, where COMFORT temperature can only be set on the heater while ECO is set from the hub.
Type of change
Additional information
Checklist
ruff format 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.To help with the load of incoming pull requests: