From aa9f69a7bbcfb471094778ee3e4a9ab369c233bf Mon Sep 17 00:00:00 2001 From: Bas Nijholt Date: Mon, 31 Jul 2023 17:52:53 -0700 Subject: [PATCH 1/3] Set last_service_data in the right place (#652) * Set last_service_data in the right place * Do not get but access key * rename --- custom_components/adaptive_lighting/switch.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/custom_components/adaptive_lighting/switch.py b/custom_components/adaptive_lighting/switch.py index 4229f0ac..7bcd78bb 100644 --- a/custom_components/adaptive_lighting/switch.py +++ b/custom_components/adaptive_lighting/switch.py @@ -1262,8 +1262,6 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): context = context or self.create_context("adapt_lights") - self.manager.last_service_data[light] = service_data - return prepare_adaptation_data( self.hass, light, @@ -1337,6 +1335,8 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): service_data, data.context.id, ) + light = service_data[ATTR_ENTITY_ID] + self.manager.last_service_data[light] = service_data await self.hass.services.async_call( LIGHT_DOMAIN, SERVICE_TURN_ON, @@ -2134,10 +2134,7 @@ class AdaptiveLightingManager: def start_transition_timer(self, light: str) -> None: """Mark a light as manually controlled.""" - last_service_data = self.last_service_data.get(light) - if not last_service_data: - _LOGGER.debug("This should not ever happen. Please report to the devs.") - return + last_service_data = self.last_service_data[light] last_transition = last_service_data.get(ATTR_TRANSITION) if not last_transition: _LOGGER.debug( From da2e27d7381b12e9362daa2a6eb76e0cb6d97277 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 31 Jul 2023 18:15:36 -0700 Subject: [PATCH 2/3] [pre-commit.ci] pre-commit autoupdate (#691) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updates: - [github.com/astral-sh/ruff-pre-commit: v0.0.280 → v0.0.281](https://github.com/astral-sh/ruff-pre-commit/compare/v0.0.280...v0.0.281) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index dc82bbe7..6e27d970 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -8,7 +8,7 @@ repos: - id: mixed-line-ending args: ["--fix=lf"] - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.0.280 + rev: v0.0.281 hooks: - id: ruff args: ["--fix"] From e5133936536cb02d6e8122c1d7dc5fde5810214c Mon Sep 17 00:00:00 2001 From: Bas Nijholt Date: Mon, 31 Jul 2023 18:40:32 -0700 Subject: [PATCH 3/3] Keep on and off state tracking in Manager and listen to toggle (#689) * Keep on and off state tracking in Manager * Move manual_control code * add todo * check is_on * Track toggle * handle toggle * fix debug * test * add comment * better log * Fix comment * log * test * get * check * cancel early * rephrase * add assert * Remove check --- custom_components/adaptive_lighting/switch.py | 312 +++++++++--------- tests/test_switch.py | 14 +- 2 files changed, 166 insertions(+), 160 deletions(-) diff --git a/custom_components/adaptive_lighting/switch.py b/custom_components/adaptive_lighting/switch.py index 7bcd78bb..772aa1e0 100644 --- a/custom_components/adaptive_lighting/switch.py +++ b/custom_components/adaptive_lighting/switch.py @@ -859,12 +859,6 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): self._icon = ICON_MAIN self._state: bool | None = None - # Tracks 'on' → 'off' state changes - self._on_to_off_event: dict[str, Event] = {} - # Tracks 'off' → 'on' state changes - self._off_to_on_event: dict[str, Event] = {} - # Locks that prevent light adjusting when waiting for a light to 'turn_off' - self._locks: dict[str, asyncio.Lock] = {} # To count the number of `Context` instances self._context_cnt: int = 0 @@ -1041,15 +1035,7 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): ) self.remove_listeners.append(remove_sleep) - - if self.lights: - self._expand_light_groups() - remove_state = async_track_state_change_event( - self.hass, - entity_ids=self.lights, - action=self._light_state_event_action, - ) - self.remove_listeners.append(remove_state) + self._expand_light_groups() def _update_time_interval_listener(self) -> None: """Create or recreate the adaptation interval listener. @@ -1282,19 +1268,13 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): adapt_color: bool | None = None, prefer_rgb_color: bool | None = None, ) -> None: - if (lock := self._locks.get(light)) is not None and lock.locked(): - _LOGGER.debug("%s: '%s' is locked", self._name, light) - return + # This should never happen if it's been proactively adapted. + # The context.parent_id is the context.id of the service call that was intercepted + # and context.id here is from the resulting "light_event" event. + assert not self.manager.is_proactively_adapting(context.parent_id) - if context.parent_id is not None and self.manager.is_proactively_adapting( - context.parent_id, - ): - # Skip if adaptation was already executed by the service call interceptor - _LOGGER.debug( - "%s: Skipping reactive adaptation of %s", - self._name, - context.parent_id, - ) + if (lock := self.manager.turn_off_locks.get(light)) and lock.locked(): + _LOGGER.debug("%s: '%s' is locked", self._name, light) return data = await self.prepare_adaptation_data( @@ -1482,6 +1462,39 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): ) await self._adapt_light(light, context, transition) + async def _respond_to_off_to_on_event(self, entity_id: str, event: Event) -> None: + assert not self.manager.is_proactively_adapting(event.context.id) + if ( + not self._detect_non_ha_changes + and not self.manager._off_to_on_state_event_is_from_turn_on( + entity_id, + event, + ) + ): + # There is an edge case where 2 switches control the same light, e.g., + # one for brightness and one for color. Now we will mark both switches + # as manually controlled, which is not 100% correct. + _LOGGER.debug( + "%s: Ignoring 'off' → 'on' event for '%s' with context.id='%s'" + " because 'light.turn_on' was not called by HA and" + " 'detect_non_ha_changes' is False", + self._name, + entity_id, + event.context.id, + ) + self.manager.mark_as_manual_control(entity_id) + return + + if self._adapt_delay > 0: + await asyncio.sleep(self._adapt_delay) + + await self._update_attrs_and_maybe_adapt_lights( + context=self.create_context("light_event", parent=event.context), + lights=[entity_id], + transition=self.initial_transition, + force=True, + ) + async def _sleep_mode_switch_state_event_action(self, event: Event) -> None: if not _is_state_event(event, (STATE_ON, STATE_OFF)): _LOGGER.debug("%s: Ignoring sleep event %s", self._name, event) @@ -1499,102 +1512,6 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): force=True, ) - async def _light_state_event_action(self, event: Event) -> None: - old_state = event.data.get("old_state") - new_state = event.data.get("new_state") - entity_id: str = event.data["entity_id"] - - if old_state is None or new_state is None: - return - - if old_state.state == STATE_ON and new_state.state == STATE_OFF: - # Tracks 'on' → 'off' state changes - self._on_to_off_event[entity_id] = event - self.manager.reset(entity_id) - _LOGGER.debug( - "%s: Detected an 'on' → 'off' event for '%s' with context.id='%s'", - self._name, - entity_id, - event.context.id, - ) - - if old_state.state == STATE_OFF and new_state.state == STATE_ON: - # Tracks 'off' → 'on' state changes - self._off_to_on_event[entity_id] = event - _LOGGER.debug( - "%s: Detected an 'off' → 'on' event for '%s' with context.id='%s'", - self._name, - entity_id, - event.context.id, - ) - - if ( - not self._detect_non_ha_changes - and not self.manager.is_proactively_adapting(event.context.id) - and not self.manager._off_to_on_state_event_is_from_turn_on( - entity_id, - event, - ) - ): - # If we don't detect non-HA changes, we're only adjusting lights that - # were turned on by HA. If the light was turned on by something else, - # we don't adjust it (e.g., when HA suddenly reports it as on). - # Sometimes the light incorrectly reports itself as on when it's - # actually off. This code path will ensure that the light is - # not controlled by Adaptive Lighting. - _LOGGER.debug( - "%s: Ignoring 'off' → 'on' event for '%s' with context.id='%s'" - " because 'light.turn_on' was not called by HA and" - " 'detect_non_ha_changes' is False", - self._name, - entity_id, - event.context.id, - ) - self.manager.mark_as_manual_control(entity_id) - return - - if event.context.parent_id and not self.manager.is_proactively_adapting( - event.context.id, - ): - self.manager.reset(entity_id, reset_manual_control=False) - - lock = self._locks.setdefault(entity_id, asyncio.Lock()) - async with lock: - if await self.manager.just_turned_off( - entity_id, - off_to_on_event=event, - on_to_off_event=self._on_to_off_event.get(entity_id), - ): - # Stop if a rapid 'off' → 'on' → 'off' happens. - _LOGGER.debug( - "%s: Cancelling adjusting lights for %s", - self._name, - entity_id, - ) - return - - if self._adapt_delay > 0: - _LOGGER.debug( - "%s: sleep started for '%s' with context.id='%s'", - self._name, - entity_id, - event.context.id, - ) - await asyncio.sleep(self._adapt_delay) - _LOGGER.debug( - "%s: sleep ended for '%s' with context.id='%s'", - self._name, - entity_id, - event.context.id, - ) - - await self._update_attrs_and_maybe_adapt_lights( - context=self.create_context("light_event", parent=event.context), - lights=[entity_id], - transition=self.initial_transition, - force=True, - ) - class SimpleSwitch(SwitchEntity, RestoreEntity): """Representation of a Adaptive Lighting switch.""" @@ -1908,12 +1825,20 @@ class AdaptiveLightingManager: self.turn_off_event: dict[str, Event] = {} # Tracks 'light.turn_on' service calls self.turn_on_event: dict[str, Event] = {} + # Tracks 'light.toggle' service calls + self.toggle_event: dict[str, Event] = {} + # Tracks 'on' → 'off' state changes + self.on_to_off_event: dict[str, Event] = {} + # Tracks 'off' → 'on' state changes + self.off_to_on_event: dict[str, Event] = {} # Keep 'asyncio.sleep' tasks that can be cancelled by 'light.turn_on' events self.sleep_tasks: dict[str, asyncio.Task] = {} + # Locks that prevent light adjusting when waiting for a light to 'turn_off' + self.turn_off_locks: dict[str, asyncio.Lock] = {} # Tracks which lights are manually controlled self.manual_control: dict[str, bool] = {} # Track 'state_changed' events of self.lights resulting from this integration - self.last_state_change: dict[str, list[State]] = {} + self.our_last_state_on_change: dict[str, list[State]] = {} # Track last 'service_data' to 'light.turn_on' resulting from this integration self.last_service_data: dict[str, dict[str, Any]] = {} # Track ongoing split adaptations to be able to cancel them @@ -2069,7 +1994,11 @@ class AdaptiveLightingManager: call.context.id, ) + # Reset because turning on the light, this also happens in + # `turn_on_off_event_listener`, however, this function is called + # before that one. self.reset(entity_id, reset_manual_control=False) + self.clear_proactively_adapting(entity_id) transition = data[CONF_PARAMS].get( @@ -2243,7 +2172,7 @@ class AdaptiveLightingManager: timer = self.auto_reset_manual_control_timers.pop(light, None) if timer is not None: timer.cancel() - self.last_state_change.pop(light, None) + self.our_last_state_on_change.pop(light, None) self.last_service_data.pop(light, None) self.cancel_ongoing_adaptation_calls(light) @@ -2286,6 +2215,24 @@ class AdaptiveLightingManager: if not any(eid in self.lights for eid in entity_ids): return + def off(eid: str, event: Event): + self.turn_off_event[eid] = event + self.reset(eid) + + def on(eid: str, event: Event): + task = self.sleep_tasks.get(eid) + if task is not None: + task.cancel() + self.turn_on_event[eid] = event + timer = self.auto_reset_manual_control_timers.get(eid) + if ( + timer is not None + and timer.is_running() + and event.time_fired > timer.start_time # type: ignore[operator] + ): + # Restart the auto reset timer + timer.start() + if service == SERVICE_TURN_OFF: transition = service_data.get(ATTR_TRANSITION) _LOGGER.debug( @@ -2295,8 +2242,7 @@ class AdaptiveLightingManager: event.context.id, ) for eid in entity_ids: - self.turn_off_event[eid] = event - self.reset(eid) + off(eid, event) elif service == SERVICE_TURN_ON: _LOGGER.debug( @@ -2305,18 +2251,21 @@ class AdaptiveLightingManager: event.context.id, ) for eid in entity_ids: - task = self.sleep_tasks.get(eid) - if task is not None: - task.cancel() - self.turn_on_event[eid] = event - timer = self.auto_reset_manual_control_timers.get(eid) - if ( - timer is not None - and timer.is_running() - and event.time_fired > timer.start_time # type: ignore[operator] - ): - # Restart the auto reset timer - timer.start() + on(eid, event) + + elif service == SERVICE_TOGGLE: + _LOGGER.debug( + "Detected an 'light.toggle('%s')' event with context.id='%s'", + entity_ids, + event.context.id, + ) + for eid in entity_ids: + state = self.hass.states.get(eid).state + self.toggle_event[eid] = event + if state == STATE_ON: # is turning off + off(eid, event) + elif state == STATE_OFF: # is turning on + on(eid, event) async def state_changed_event_listener(self, event: Event) -> None: """Track 'state_changed' events.""" @@ -2324,16 +2273,21 @@ class AdaptiveLightingManager: if entity_id not in self.lights: return + old_state = event.data.get("old_state") new_state = event.data.get("new_state") - if new_state is not None and new_state.state == STATE_ON: + + new_on = new_state is not None and new_state.state == STATE_ON + new_off = new_state is not None and new_state.state == STATE_OFF + old_on = old_state is not None and old_state.state == STATE_ON + old_off = old_state is not None and old_state.state == STATE_OFF + + if new_on: _LOGGER.debug( "Detected a '%s' 'state_changed' event: '%s' with context.id='%s'", entity_id, new_state.attributes, new_state.context.id, ) - - if new_state is not None and new_state.state == STATE_ON: # It is possible to have multiple state change events with the same context. # This can happen because a `turn_on.light(brightness_pct=100, transition=30)` # event leads to an instant state change of @@ -2345,30 +2299,78 @@ class AdaptiveLightingManager: # called with a color_temp outside of its range (and HA reports the # incorrect 'min_kelvin' and 'max_kelvin', which happens e.g., for # Philips Hue White GU10 Bluetooth lights). - old_state: list[State] | None = self.last_state_change.get(entity_id) + last_state: list[State] | None = self.our_last_state_on_change.get( + entity_id, + ) if is_our_context(new_state.context): if ( - old_state is not None - and old_state[0].context.id == new_state.context.id + last_state is not None + and last_state[0].context.id == new_state.context.id ): _LOGGER.debug( "AdaptiveLightingManager: State change event of '%s' is already" - " in 'self.last_state_change' (%s)" + " in 'self.our_last_state_on_change' (%s)" " adding this state also", entity_id, new_state.context.id, ) - self.last_state_change[entity_id].append(new_state) + self.our_last_state_on_change[entity_id].append(new_state) else: _LOGGER.debug( "AdaptiveLightingManager: New adapt '%s' found for %s", new_state, entity_id, ) - self.last_state_change[entity_id] = [new_state] + self.our_last_state_on_change[entity_id] = [new_state] self.start_transition_timer(entity_id) - elif old_state is not None: - self.last_state_change[entity_id].append(new_state) + elif last_state is not None: + self.our_last_state_on_change[entity_id].append(new_state) + + if old_on and new_off: + # Tracks 'on' → 'off' state changes + self.on_to_off_event[entity_id] = event + self.reset(entity_id) + _LOGGER.debug( + "Detected an 'on' → 'off' event for '%s' with context.id='%s'", + entity_id, + event.context.id, + ) + elif old_off and new_on: + # Tracks 'off' → 'on' state changes + self.off_to_on_event[entity_id] = event + _LOGGER.debug( + "Detected an 'off' → 'on' event for '%s' with context.id='%s'", + entity_id, + event.context.id, + ) + + if self.is_proactively_adapting(event.context.id): + _LOGGER.debug( + "Skipping responding to 'off' → 'on' event for '%s' with context.id='%s' because" + " we are already proactively adapting", + entity_id, + event.context.id, + ) + return + + self.reset(entity_id, reset_manual_control=False) + lock = self.turn_off_locks.setdefault(entity_id, asyncio.Lock()) + async with lock: + if await self.just_turned_off(entity_id): + # Stop if a rapid 'off' → 'on' → 'off' happens. + _LOGGER.debug( + "Cancelling adjusting lights for %s", + entity_id, + ) + return + + switches = _switches_with_lights(self.hass, [entity_id]) + for switch in switches: + if switch.is_on: + await switch._respond_to_off_to_on_event( + entity_id, + event, + ) def is_manually_controlled( self, @@ -2497,8 +2499,6 @@ class AdaptiveLightingManager: async def just_turned_off( # noqa: PLR0911 self, entity_id: str, - off_to_on_event: Event, - on_to_off_event: Event | None, ) -> bool: """Cancel the adjusting of a light if it has just been turned off. @@ -2512,6 +2512,9 @@ class AdaptiveLightingManager: if the brightness is still decreasing. Only if it is the case we adjust the lights. """ + off_to_on_event = self.off_to_on_event[entity_id] + on_to_off_event = self.on_to_off_event.get(entity_id) + if on_to_off_event is None: _LOGGER.debug( "just_turned_off: No 'on' → 'off' state change has been registered before for '%s'." @@ -2529,8 +2532,11 @@ class AdaptiveLightingManager: transition = None if self._off_to_on_state_event_is_from_turn_on(entity_id, off_to_on_event): + is_toggle = off_to_on_event == self.toggle_event.get(entity_id) + from_service = "light.toggle" if is_toggle else "light.turn_on" _LOGGER.debug( - "just_turned_off: State change 'off' → 'on' triggered by 'light.turn_on'", + "just_turned_off: State change 'off' → 'on' triggered by '%s'", + from_service, ) return False diff --git a/tests/test_switch.py b/tests/test_switch.py index 43da74a2..c7220ee4 100644 --- a/tests/test_switch.py +++ b/tests/test_switch.py @@ -990,8 +990,8 @@ async def test_state_change_handlers(hass): blocking=True, ) await hass.async_block_till_done() - assert switch.manager.last_state_change.get(ENTITY_LIGHT_1) - assert len(switch.manager.last_state_change[ENTITY_LIGHT_1]) == 1 + assert switch.manager.our_last_state_on_change.get(ENTITY_LIGHT_1) + assert len(switch.manager.our_last_state_on_change[ENTITY_LIGHT_1]) == 1 assert not switch.manager.transition_timers.get(ENTITY_LIGHT_1) last_service_data = deepcopy(switch.manager.last_service_data) assert last_service_data.get(ENTITY_LIGHT_1) @@ -1060,8 +1060,8 @@ async def test_state_change_handlers(hass): # asyncio.sleep(3) # 4. Assert the transition timer started and everything was filled. listener = switch.manager - assert listener.last_state_change.get(ENTITY_LIGHT_1) - assert len(listener.last_state_change[ENTITY_LIGHT_1]) == total_events + assert listener.our_last_state_on_change.get(ENTITY_LIGHT_1) + assert len(listener.our_last_state_on_change[ENTITY_LIGHT_1]) == total_events assert listener.transition_timers.get(ENTITY_LIGHT_1) # 5. Execute some checks during a transition @@ -1086,8 +1086,8 @@ async def test_state_change_handlers(hass): # 6. Assert everything after the transition finishes. await asyncio.sleep(transition_used) - assert listener.last_state_change.get(ENTITY_LIGHT_1) - assert len(listener.last_state_change[ENTITY_LIGHT_1]) == total_events + assert listener.our_last_state_on_change.get(ENTITY_LIGHT_1) + assert len(listener.our_last_state_on_change[ENTITY_LIGHT_1]) == total_events # Timer should be done and reset now. # This is the assert that I can't fix. timer = listener.transition_timers.get(ENTITY_LIGHT_1) @@ -1115,7 +1115,7 @@ async def test_state_change_handlers(hass): # On next update ENTITY_LIGHT_1 should be marked as manually controlled await update(force=False) assert switch.manager.last_service_data.get(ENTITY_LIGHT_1) is not None - assert switch.manager.last_state_change.get(ENTITY_LIGHT_1) is not None + assert switch.manager.our_last_state_on_change.get(ENTITY_LIGHT_1) is not None assert switch.manager.manual_control[ENTITY_LIGHT_1]