Skip to content

fix: merge last_service_data across split calls to fix detect_non_ha_changes with separate_turn_on_commands#1426

Merged
basnijholt merged 8 commits intobasnijholt:mainfrom
roeehendel:fix/last-service-data-merge-for-split-commands
Mar 16, 2026
Merged

fix: merge last_service_data across split calls to fix detect_non_ha_changes with separate_turn_on_commands#1426
basnijholt merged 8 commits intobasnijholt:mainfrom
roeehendel:fix/last-service-data-merge-for-split-commands

Conversation

@roeehendel
Copy link
Contributor

@roeehendel roeehendel commented Feb 20, 2026

Bug

separate_turn_on_commands: true causes each adaptation cycle to make two sequential light.turn_on calls — one for brightness, one for color. Each call overwrote last_service_data, so after the color call, brightness was gone.

When detect_non_ha_changes: true checks for manual changes, _attributes_have_changed reads brightness from last_service_data. With brightness absent, the comparison is silently skipped — so brightness changes made directly on the bulb (e.g. via a Zigbee-bound switch bypassing HA) were never detected, and AL kept overriding them.

Fix

Merge into last_service_data instead of overwriting, so attributes accumulate across split calls:

self.manager.last_service_data[light] = {
    **self.manager.last_service_data.get(light, {}),
    **service_data,
}

reset() clears last_service_data when a light turns off, so there is no stale-data concern.

Testing

Added test_detect_non_ha_changes_with_separate_turn_on_commands in tests/test_switch.py, which:

  1. Force-adapts and asserts last_service_data contains both brightness and color_temp_kelvin after the split calls
  2. Simulates a direct Zigbee brightness change
  3. Asserts the light is marked manually_controlled=BRIGHTNESS on the next interval
  4. Asserts AL does not override the manual brightness on the following interval

Live-tested on HAOS 17.0 / HA Core 2026.2.1 with an IKEA TRADFRI bulb + RODRET switch. Before the fix, AL overrode manual brightness every interval. After the fix, the manual change is detected and respected.

@roeehendel roeehendel marked this pull request as draft February 20, 2026 20:55
@roeehendel roeehendel marked this pull request as ready for review February 20, 2026 21:49
@roeehendel roeehendel marked this pull request as draft February 21, 2026 11:04
…eparate_turn_on_commands

End-to-end scenario: user adjusts brightness via a directly-bound Zigbee
switch (e.g. IKEA RODRET).  No HA service call is made; ZHA reports the new
brightness via async_update_entity.  On the next adaptation interval AL must
detect the change and stop overriding the user's brightness.

The test verifies the user-visible symptom: after two adaptation cycles
following a simulated direct-Zigbee brightness change, the light's brightness
must still be the manually set value — not AL's own target.

NOTE: this test FAILS on the current code.  It is committed here to document
the bug before the fix is applied in the next commit.
@roeehendel roeehendel force-pushed the fix/last-service-data-merge-for-split-commands branch from d95a194 to d2a2cde Compare February 21, 2026 11:46
…changes with separate_turn_on_commands

When separate_turn_on_commands=True, each adaptation cycle makes two
light.turn_on calls (brightness, then color_temp).  Previously each call
overwrote last_service_data[light], so after the cycle only the color_temp
key remained.  _attributes_have_changed() then saw old_brightness=None and
silently skipped the brightness comparison, so a manually-set brightness was
never detected and AL kept overriding it.

Fix: merge instead of overwrite so all split-call attributes accumulate:

    self.manager.last_service_data[light] = {
        **self.manager.last_service_data.get(light, {}),
        **service_data,
    }
@roeehendel roeehendel force-pushed the fix/last-service-data-merge-for-split-commands branch from d2a2cde to b26286c Compare February 21, 2026 11:48
root and others added 6 commits February 22, 2026 11:08
Two assertions were promised in the PR description but missing:
1. After the force-adapt, assert that last_service_data contains BOTH
   brightness AND color — directly proving the merge fix works.
2. After the first non-forced update, assert that BRIGHTNESS is in
   manual_control — proving detection fired, not just that the final
   state is right.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@roeehendel roeehendel marked this pull request as ready for review February 22, 2026 09:46
@roeehendel
Copy link
Contributor Author

roeehendel commented Mar 2, 2026

Ping, let me know if you have any feedback or need changes.

Copy link

@homeassilol homeassilol left a comment

Choose a reason for hiding this comment

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

Good catch — the overwrite bug is real and the test proves it. One minor suggestion: consider using dict.update() instead of {**existing, **new} to avoid allocating a new dict on each call:

existing = self.manager.last_service_data.setdefault(light, {})
existing.update(service_data)

Otherwise LGTM — well tested with live verification on HAOS.

@basnijholt basnijholt merged commit 6cebe14 into basnijholt:main Mar 16, 2026
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants