Repository navigation
Conversation
…uler adjustment Co-authored-by: Gamso <13354981+Gamso@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new “volets/covers” feature set to HomeShift: automatic heat-protection actions for covers and a daily sunrise-based adjustment of scheduler start times, wired into the integration lifecycle and config/options UI.
Changes:
- Introduce
CoverManagerwith (1) heat-protection cover control and (2) sunrise-based scheduler start-time adjustment. - Extend config flow + translations (EN/FR) to configure the new features.
- Add dedicated unit tests for
CoverManager.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
custom_components/homeshift/cover_manager.py |
New manager implementing heat protection and sunrise scheduler adjustment logic. |
custom_components/homeshift/__init__.py |
Instantiates CoverManager and registers state/time listeners; stores manager in hass.data. |
custom_components/homeshift/config_flow.py |
Adds menu steps + schemas for configuring heat protection and sunrise scheduler adjustment. |
custom_components/homeshift/const.py |
Adds new config keys and default values for the two features. |
custom_components/homeshift/translations/en.json |
Adds UI strings for the new config/options steps. |
custom_components/homeshift/translations/fr.json |
Adds UI strings for the new config/options steps (French). |
tests/test_cover_manager.py |
Adds unit tests for heat protection and sunrise scheduler adjustment behaviors. |
| actions: list = list(sched_state.attributes.get("actions", [{}])) | ||
| entities: list = list(sched_state.attributes.get("entities", [])) | ||
|
|
||
| if not actions: | ||
| _LOGGER.warning( | ||
| "Sunrise adjustment: scheduler '%s' has no actions — skipping", | ||
| entity_id, | ||
| ) | ||
| continue | ||
|
|
||
| # Mirror user automation: merge entity_id back into the action dict | ||
| current_action: dict = dict(actions[0]) | ||
| if entities: | ||
| current_action["entity_id"] = entities[0] | ||
|
|
||
| await self.hass.services.async_call( | ||
| "scheduler", | ||
| "edit", | ||
| { | ||
| "entity_id": entity_id, | ||
| "timeslots": [{"start": target_time, "actions": [current_action]}], |
There was a problem hiding this comment.
The docstring says this “updates the first timeslot start time”, but the service call sends a new timeslots list containing only one timeslot. If a scheduler has multiple timeslots, this will overwrite/remove all other timeslots when calling scheduler.edit. Consider reading the existing timeslots from the scheduler state (if available), updating only the first timeslot’s start, and sending back the full (updated) list so other timeslots are preserved.
| actions: list = list(sched_state.attributes.get("actions", [{}])) | |
| entities: list = list(sched_state.attributes.get("entities", [])) | |
| if not actions: | |
| _LOGGER.warning( | |
| "Sunrise adjustment: scheduler '%s' has no actions — skipping", | |
| entity_id, | |
| ) | |
| continue | |
| # Mirror user automation: merge entity_id back into the action dict | |
| current_action: dict = dict(actions[0]) | |
| if entities: | |
| current_action["entity_id"] = entities[0] | |
| await self.hass.services.async_call( | |
| "scheduler", | |
| "edit", | |
| { | |
| "entity_id": entity_id, | |
| "timeslots": [{"start": target_time, "actions": [current_action]}], | |
| timeslots_attr: list = list(sched_state.attributes.get("timeslots", [])) | |
| if not timeslots_attr: | |
| _LOGGER.warning( | |
| "Sunrise adjustment: scheduler '%s' has no timeslots — skipping", | |
| entity_id, | |
| ) | |
| continue | |
| # Update only the first timeslot start while preserving all others | |
| timeslots: list[dict] = [dict(ts) for ts in timeslots_attr] | |
| first_slot: dict = dict(timeslots[0]) | |
| first_slot["start"] = target_time | |
| timeslots[0] = first_slot | |
| await self.hass.services.async_call( | |
| "scheduler", | |
| "edit", | |
| { | |
| "entity_id": entity_id, | |
| "timeslots": timeslots, |
| import logging | ||
| from typing import TYPE_CHECKING, Callable | ||
|
|
||
| from homeassistant.core import HomeAssistant | ||
| from homeassistant.util import dt as dt_util | ||
|
|
||
| from .const import ( | ||
| CONF_HEAT_PROTECTION_COVERS, | ||
| CONF_HEAT_PROTECTION_SENSOR, | ||
| CONF_HEAT_PROTECTION_THRESHOLD, | ||
| CONF_HEAT_PROTECTION_START, | ||
| CONF_HEAT_PROTECTION_END, | ||
| CONF_SUNRISE_SCHEDULERS, | ||
| CONF_SUNRISE_EARLIEST_TIME, | ||
| DEFAULT_HEAT_PROTECTION_THRESHOLD, | ||
| DEFAULT_HEAT_PROTECTION_START, | ||
| DEFAULT_HEAT_PROTECTION_END, | ||
| DEFAULT_SUNRISE_EARLIEST_TIME, | ||
| ) | ||
|
|
||
| if TYPE_CHECKING: | ||
| pass | ||
|
|
There was a problem hiding this comment.
TYPE_CHECKING is imported but the guard block is empty. This can be removed to keep the module clean, or add the intended typing-only imports inside the block.
| cover_manager = CoverManager(hass, lambda: coordinator._config) | ||
|
|
||
| # Temperature sensor state-change → heat-protection check | ||
| heat_sensor: str = coordinator._config.get(CONF_HEAT_PROTECTION_SENSOR, "") |
There was a problem hiding this comment.
async_setup_entry is reading coordinator._config from outside the coordinator. Since _config is a “private” property (leading underscore), this couples init.py to coordinator internals. Consider exposing a public config/get_config() on the coordinator (or passing lambda: {**entry.data, **entry.options} directly) so other modules don’t rely on underscored attributes.
| cover_manager = CoverManager(hass, lambda: coordinator._config) | |
| # Temperature sensor state-change → heat-protection check | |
| heat_sensor: str = coordinator._config.get(CONF_HEAT_PROTECTION_SENSOR, "") | |
| config_getter = lambda: {**entry.data, **entry.options} | |
| cover_manager = CoverManager(hass, config_getter) | |
| # Temperature sensor state-change → heat-protection check | |
| heat_sensor: str = config_getter().get(CONF_HEAT_PROTECTION_SENSOR, "") |
| # Daily at 00:15 → sunrise scheduler adjustment | ||
| async def _async_adjust_sunrise(_now) -> None: | ||
| await cover_manager.async_adjust_sunrise_schedulers() | ||
|
|
||
| entry.async_on_unload( | ||
| async_track_time_change(hass, _async_adjust_sunrise, hour=0, minute=15, second=0) | ||
| ) |
There was a problem hiding this comment.
The daily async_track_time_change callback is registered unconditionally, even when no sunrise schedulers are configured. This adds a permanent daily wake-up for every installation and makes setup harder to unit-test/mocks heavier. Consider only registering this listener when CONF_SUNRISE_SCHEDULERS is configured/non-empty (similar to how the heat-protection listener is only registered when the sensor is set).
| changes state. For Somfy covers, ``cover.stop_cover`` closes the | ||
| cover to a pre-recorded *my* position — it does **not** simply pause | ||
| movement. The action is therefore only triggered once per temperature | ||
| rise above the threshold; subsequent identical readings do nothing | ||
| because the cover is already in the closed position. |
There was a problem hiding this comment.
The docstring says the stop_cover action is only triggered once per temperature rise above the threshold, but the implementation will call stop_cover on every sensor state change while the temperature remains > threshold (e.g., 32.5 → 32.6). This can spam service calls/logs and doesn’t match the documented behavior. Consider tracking an internal “was_above_threshold” flag (or last temperature) to only trigger on the crossing from <= threshold to > threshold, and reset when the temperature drops back below (optionally with hysteresis).
| changes state. For Somfy covers, ``cover.stop_cover`` closes the | |
| cover to a pre-recorded *my* position — it does **not** simply pause | |
| movement. The action is therefore only triggered once per temperature | |
| rise above the threshold; subsequent identical readings do nothing | |
| because the cover is already in the closed position. | |
| changes state. For Somfy covers, ``cover.stop_cover`` closes the | |
| cover to a pre-recorded *my* position — it does **not** simply pause | |
| movement. On each sensor update where the temperature is above the | |
| configured threshold and within the active window, ``cover.stop_cover`` | |
| will be invoked for the configured covers, even if they are already in | |
| the desired position. |
| covers: list[str] = self._config.get(CONF_HEAT_PROTECTION_COVERS, []) | ||
| sensor: str = self._config.get(CONF_HEAT_PROTECTION_SENSOR, "") | ||
|
|
||
| if not covers or not sensor: | ||
| return | ||
|
|
||
| try: | ||
| threshold = float( | ||
| self._config.get(CONF_HEAT_PROTECTION_THRESHOLD, DEFAULT_HEAT_PROTECTION_THRESHOLD) | ||
| ) | ||
| except (ValueError, TypeError): | ||
| threshold = DEFAULT_HEAT_PROTECTION_THRESHOLD | ||
|
|
||
| start: str = self._config.get(CONF_HEAT_PROTECTION_START, DEFAULT_HEAT_PROTECTION_START) | ||
| end: str = self._config.get(CONF_HEAT_PROTECTION_END, DEFAULT_HEAT_PROTECTION_END) |
There was a problem hiding this comment.
This method calls self._config multiple times, which re-invokes config_getter() each time. If the getter is non-trivial (or if options change mid-call), this can lead to unnecessary work and inconsistent reads within a single check. Consider snapshotting once at the start (e.g., cfg = self._config) and reading all config values from that dict for the remainder of the method.
| covers: list[str] = self._config.get(CONF_HEAT_PROTECTION_COVERS, []) | |
| sensor: str = self._config.get(CONF_HEAT_PROTECTION_SENSOR, "") | |
| if not covers or not sensor: | |
| return | |
| try: | |
| threshold = float( | |
| self._config.get(CONF_HEAT_PROTECTION_THRESHOLD, DEFAULT_HEAT_PROTECTION_THRESHOLD) | |
| ) | |
| except (ValueError, TypeError): | |
| threshold = DEFAULT_HEAT_PROTECTION_THRESHOLD | |
| start: str = self._config.get(CONF_HEAT_PROTECTION_START, DEFAULT_HEAT_PROTECTION_START) | |
| end: str = self._config.get(CONF_HEAT_PROTECTION_END, DEFAULT_HEAT_PROTECTION_END) | |
| cfg = self._config | |
| covers: list[str] = cfg.get(CONF_HEAT_PROTECTION_COVERS, []) | |
| sensor: str = cfg.get(CONF_HEAT_PROTECTION_SENSOR, "") | |
| if not covers or not sensor: | |
| return | |
| try: | |
| threshold = float( | |
| cfg.get(CONF_HEAT_PROTECTION_THRESHOLD, DEFAULT_HEAT_PROTECTION_THRESHOLD) | |
| ) | |
| except (ValueError, TypeError): | |
| threshold = DEFAULT_HEAT_PROTECTION_THRESHOLD | |
| start: str = cfg.get(CONF_HEAT_PROTECTION_START, DEFAULT_HEAT_PROTECTION_START) | |
| end: str = cfg.get(CONF_HEAT_PROTECTION_END, DEFAULT_HEAT_PROTECTION_END) |
No description provided.