Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Set min, max, and step for ViCare number entities #104593

Merged
merged 9 commits into from
Nov 28, 2023
Merged

Set min, max, and step for ViCare number entities #104593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cd4cc18
add constraints handling
CFenner Nov 27, 2023
2bae121
Merge branch 'dev' into constraints
CFenner Nov 27, 2023
74c870f
use correct function name
CFenner Nov 27, 2023
2df65af
apply suggestions
CFenner Nov 27, 2023
193d816
Apply suggestions from code review
CFenner Nov 27, 2023
e8a3cc1
refactor
CFenner Nov 27, 2023
e0a6c0e
Apply suggestions from code review
CFenner Nov 28, 2023
82666e3
Revert "Apply suggestions from code review"
CFenner Nov 28, 2023
f709269
add helper
CFenner Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions homeassistant/components/vicare/binary_sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ async def _entities_from_descriptions(
hass.data[DOMAIN][config_entry.entry_id][VICARE_DEVICE_CONFIG],
description,
)
if entity is not None:
if entity:
entities.append(entity)


Expand All @@ -156,7 +156,7 @@ async def async_setup_entry(
hass.data[DOMAIN][config_entry.entry_id][VICARE_DEVICE_CONFIG],
description,
)
if entity is not None:
if entity:
entities.append(entity)

circuits = await hass.async_add_executor_job(get_circuits, api)
Expand Down
2 changes: 1 addition & 1 deletion homeassistant/components/vicare/button.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ async def async_setup_entry(
hass.data[DOMAIN][config_entry.entry_id][VICARE_DEVICE_CONFIG],
description,
)
if entity is not None:
if entity:
entities.append(entity)

async_add_entities(entities)
Expand Down
66 changes: 47 additions & 19 deletions homeassistant/components/vicare/number.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from PyViCare.PyViCareDevice import Device as PyViCareDevice
from PyViCare.PyViCareDeviceConfig import PyViCareDeviceConfig
from PyViCare.PyViCareHeatingDevice import (
HeatingDeviceWithComponent as PyViCareHeatingDeviceWithComponent,
HeatingDeviceWithComponent as PyViCareHeatingDeviceComponent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the reason to rename the import? Why aren't we using the original name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me this naming makes more sense. We deal here with burners, compressors or heating circuits which are components of a heating device and not heating devices as the name heating device with components suggest.

)
from PyViCare.PyViCareUtils import (
PyViCareInvalidDataError,
Expand Down Expand Up @@ -38,6 +38,9 @@ class ViCareNumberEntityDescription(NumberEntityDescription, ViCareRequiredKeysM
"""Describes ViCare number entity."""

value_setter: Callable[[PyViCareDevice, float], Any] | None = None
min_value_getter: Callable[[PyViCareDevice], float | None] | None = None
max_value_getter: Callable[[PyViCareDevice], float | None] | None = None
stepping_getter: Callable[[PyViCareDevice], float | None] | None = None


CIRCUIT_ENTITY_DESCRIPTIONS: tuple[ViCareNumberEntityDescription, ...] = (
Expand All @@ -46,11 +49,14 @@ class ViCareNumberEntityDescription(NumberEntityDescription, ViCareRequiredKeysM
translation_key="heating_curve_shift",
icon="mdi:plus-minus-variant",
entity_category=EntityCategory.CONFIG,
native_unit_of_measurement=UnitOfTemperature.CELSIUS,
value_getter=lambda api: api.getHeatingCurveShift(),
value_setter=lambda api, shift: (
api.setHeatingCurve(shift, api.getHeatingCurveSlope())
),
native_unit_of_measurement=UnitOfTemperature.CELSIUS,
min_value_getter=lambda api: api.getHeatingCurveShiftMin(),
max_value_getter=lambda api: api.getHeatingCurveShiftMax(),
stepping_getter=lambda api: api.getHeatingCurveShiftStepping(),
native_min_value=-13,
native_max_value=40,
native_step=1,
Expand All @@ -64,6 +70,9 @@ class ViCareNumberEntityDescription(NumberEntityDescription, ViCareRequiredKeysM
value_setter=lambda api, slope: (
api.setHeatingCurve(api.getHeatingCurveShift(), slope)
),
min_value_getter=lambda api: api.getHeatingCurveSlopeMin(),
max_value_getter=lambda api: api.getHeatingCurveSlopeMax(),
stepping_getter=lambda api: api.getHeatingCurveSlopeStepping(),
native_min_value=0.2,
native_max_value=3.5,
native_step=0.1,
Expand All @@ -72,7 +81,7 @@ class ViCareNumberEntityDescription(NumberEntityDescription, ViCareRequiredKeysM


def _build_entity(
vicare_api: PyViCareHeatingDeviceWithComponent,
vicare_api: PyViCareHeatingDeviceComponent,
device_config: PyViCareDeviceConfig,
entity_description: ViCareNumberEntityDescription,
) -> ViCareNumber | None:
Expand All @@ -92,23 +101,21 @@ async def async_setup_entry(
async_add_entities: AddEntitiesCallback,
) -> None:
"""Create the ViCare number devices."""
entities: list[ViCareNumber] = []
api = hass.data[DOMAIN][config_entry.entry_id][VICARE_API]
circuits = await hass.async_add_executor_job(get_circuits, api)
device_config = hass.data[DOMAIN][config_entry.entry_id][VICARE_DEVICE_CONFIG]

entities: list[ViCareNumber] = []
try:
for circuit in circuits:
for description in CIRCUIT_ENTITY_DESCRIPTIONS:
entity = await hass.async_add_executor_job(
_build_entity,
circuit,
hass.data[DOMAIN][config_entry.entry_id][VICARE_DEVICE_CONFIG],
description,
)
if entity is not None:
entities.append(entity)
except PyViCareNotSupportedFeatureError:
_LOGGER.debug("No circuits found")
circuits = await hass.async_add_executor_job(get_circuits, api)
for circuit in circuits:
for description in CIRCUIT_ENTITY_DESCRIPTIONS:
entity = await hass.async_add_executor_job(
Copy link
Member

Choose a reason for hiding this comment

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

We should collect all the sync work in a function and schedule that work once on the executor. The context switch from event loop to the executor is expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to do that once in __init__ and store on hass.data[DOMAIN]?

Copy link
Member

Choose a reason for hiding this comment

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

Entities are a platform concern. It would be ok to collect device information in a central place if it's relevant for more than one entity. But the entities themselves should not be stored anywhere outside their platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I misunderstood, I thought this is about circuits = await hass.async_add_executor_job(get_circuits, api) which is done for all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that the library caches the API data for 60 seconds and each access one of its objects can result in an update of that cache.
Probably an UpdateCoordinator would be better to have, but my knowledge neither in python nor in home assistant is deep enough to do that.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what the cache has to do with if we make one or multiple sequential executor jobs? It doesn't change how we access the library objects. It only changes how that work is scheduled from the event loop on the executor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think about something like this where the async executor is moved outside the loops?
https://github.com/home-assistant/core/compare/dev...CFenner:sync-4?expand=1

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but use a list comprehension. We don't need any local variable for entity then.

_build_entity,
circuit,
device_config,
description,
)
if entity:
entities.append(entity)

async_add_entities(entities)

Expand All @@ -120,7 +127,7 @@ class ViCareNumber(ViCareEntity, NumberEntity):

def __init__(
self,
api: PyViCareHeatingDeviceWithComponent,
api: PyViCareHeatingDeviceComponent,
device_config: PyViCareDeviceConfig,
description: ViCareNumberEntityDescription,
) -> None:
Expand All @@ -146,6 +153,20 @@ def update(self) -> None:
self._attr_native_value = self.entity_description.value_getter(
self._api
)
if min_value := _get_value(
self.entity_description.min_value_getter, self._api
):
self._attr_native_min_value = min_value

if max_value := _get_value(
self.entity_description.max_value_getter, self._api
):
self._attr_native_max_value = max_value

if stepping_value := _get_value(
self.entity_description.stepping_getter, self._api
):
self._attr_native_step = stepping_value
except RequestConnectionError:
_LOGGER.error("Unable to retrieve data from ViCare server")
except ValueError:
Expand All @@ -154,3 +175,10 @@ def update(self) -> None:
_LOGGER.error("Vicare API rate limit exceeded: %s", limit_exception)
except PyViCareInvalidDataError as invalid_data_exception:
_LOGGER.error("Invalid data from Vicare server: %s", invalid_data_exception)


def _get_value(
fn: Callable[[PyViCareDevice], float | None] | None,
api: PyViCareHeatingDeviceComponent,
) -> float | None:
return None if fn is None else fn(api)
4 changes: 2 additions & 2 deletions homeassistant/components/vicare/sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ async def _entities_from_descriptions(
hass.data[DOMAIN][config_entry.entry_id][VICARE_DEVICE_CONFIG],
description,
)
if entity is not None:
if entity:
entities.append(entity)


Expand All @@ -647,7 +647,7 @@ async def async_setup_entry(
device_config,
description,
)
if entity is not None:
if entity:
entities.append(entity)

circuits = await hass.async_add_executor_job(get_circuits, api)
Expand Down