Fix changing brightness when effect is active (#1019)

This PR changes the behavior of `brightness` module if an effect is
active.
Currently, changing the brightness disables the effect when the
brightness is changed, this fixes that.
This will also improve the `set_effect` interface to use the current
brightness when an effect is activated.

* light_strip_effect: passing `bAdjusted` with the changed properties
changes the brightness.
* light_effect: the brightness is stored only in the rule, so we modify
it when adjusting the brightness. This is also done during the initial
effect activation.

---------

Co-authored-by: Steven B <51370195+sdb9696@users.noreply.github.com>

kasa/tests/fakeprotocol_smart.py

@@ -250,18 +250,31 @@ class FakeSmartTransport(BaseTransport):
         info["get_dynamic_light_effect_rules"]["enable"] = params["enable"]
         if params["enable"]:
             info["get_device_info"]["dynamic_light_effect_id"] = params["id"]
-            info["get_dynamic_light_effect_rules"]["current_rule_id"] = params["enable"]
+            info["get_dynamic_light_effect_rules"]["current_rule_id"] = params["id"]
         else:
             if "dynamic_light_effect_id" in info["get_device_info"]:
                 del info["get_device_info"]["dynamic_light_effect_id"]
             if "current_rule_id" in info["get_dynamic_light_effect_rules"]:
                 del info["get_dynamic_light_effect_rules"]["current_rule_id"]
 
+    def _set_edit_dynamic_light_effect_rule(self, info, params):
+        """Edit dynamic light effect rule."""
+        rules = info["get_dynamic_light_effect_rules"]["rule_list"]
+        for rule in rules:
+            if rule["id"] == params["id"]:
+                rule.update(params)
+                return
+
+        raise Exception("Unable to find rule with id")
+
     def _set_light_strip_effect(self, info, params):
         """Set or remove values as per the device behaviour."""
         info["get_device_info"]["lighting_effect"]["enable"] = params["enable"]
         info["get_device_info"]["lighting_effect"]["name"] = params["name"]
         info["get_device_info"]["lighting_effect"]["id"] = params["id"]
+        # Brightness is not always available
+        if (brightness := params.get("brightness")) is not None:
+            info["get_device_info"]["lighting_effect"]["brightness"] = brightness
         info["get_lighting_effect"] = copy.deepcopy(params)
 
     def _set_led_info(self, info, params):
@@ -365,6 +378,9 @@ class FakeSmartTransport(BaseTransport):
         elif method == "set_dynamic_light_effect_rule_enable":
             self._set_dynamic_light_effect(info, params)
             return {"error_code": 0}
+        elif method == "edit_dynamic_light_effect_rule":
+            self._set_edit_dynamic_light_effect_rule(info, params)
+            return {"error_code": 0}
         elif method == "set_lighting_effect":
             self._set_light_strip_effect(info, params)
             return {"error_code": 0}

kasa/tests/smart/modules/test_light_effect.py

@@ -39,3 +39,45 @@ async def test_light_effect(dev: Device, mocker: MockerFixture):
 
     with pytest.raises(ValueError):
         await light_effect.set_effect("foobar")
+
+
+@light_effect
+@pytest.mark.parametrize("effect_active", [True, False])
+async def test_light_effect_brightness(
+    dev: Device, effect_active: bool, mocker: MockerFixture
+):
+    """Test that light module uses light_effect for brightness when active."""
+    light_module = dev.modules[Module.Light]
+
+    light_effect = dev.modules[Module.SmartLightEffect]
+    light_effect_set_brightness = mocker.spy(light_effect, "set_brightness")
+    mock_light_effect_call = mocker.patch.object(light_effect, "call")
+
+    brightness = dev.modules[Module.Brightness]
+    brightness_set_brightness = mocker.spy(brightness, "set_brightness")
+    mock_brightness_call = mocker.patch.object(brightness, "call")
+
+    mocker.patch.object(
+        type(light_effect),
+        "is_active",
+        new_callable=mocker.PropertyMock,
+        return_value=effect_active,
+    )
+    if effect_active:  # Set the rule L1 active for testing
+        light_effect.data["current_rule_id"] = "L1"
+
+    await light_module.set_brightness(10)
+
+    if effect_active:
+        assert light_effect.is_active
+        assert light_effect.brightness == dev.brightness
+
+        light_effect_set_brightness.assert_called_with(10)
+        mock_light_effect_call.assert_called_with(
+            "edit_dynamic_light_effect_rule", mocker.ANY
+        )
+    else:
+        assert not light_effect.is_active
+
+        brightness_set_brightness.assert_called_with(10)
+        mock_brightness_call.assert_called_with("set_device_info", {"brightness": 10})

kasa/tests/smart/modules/test_light_strip_effect.py

@@ -0,0 +1,101 @@
+from __future__ import annotations
+
+from itertools import chain
+
+import pytest
+from pytest_mock import MockerFixture
+
+from kasa import Device, Feature, Module
+from kasa.smart.modules import LightEffect, LightStripEffect
+from kasa.tests.device_fixtures import parametrize
+
+light_strip_effect = parametrize(
+    "has light strip effect",
+    component_filter="light_strip_lighting_effect",
+    protocol_filter={"SMART"},
+)
+
+
+@light_strip_effect
+async def test_light_strip_effect(dev: Device, mocker: MockerFixture):
+    """Test light strip effect."""
+    light_effect = dev.modules.get(Module.LightEffect)
+
+    assert isinstance(light_effect, LightStripEffect)
+
+    brightness = dev.modules[Module.Brightness]
+
+    feature = dev.features["light_effect"]
+    assert feature.type == Feature.Type.Choice
+
+    call = mocker.spy(light_effect, "call")
+
+    light = dev.modules[Module.Light]
+    light_call = mocker.spy(light, "call")
+
+    assert feature.choices == light_effect.effect_list
+    assert feature.choices
+    for effect in chain(reversed(feature.choices), feature.choices):
+        await light_effect.set_effect(effect)
+
+        if effect == LightEffect.LIGHT_EFFECTS_OFF:
+            light_call.assert_called()
+            continue
+
+        # Start with the current effect data
+        params = light_effect.data["lighting_effect"]
+        enable = effect != LightEffect.LIGHT_EFFECTS_OFF
+        params["enable"] = enable
+        if enable:
+            params = light_effect._effect_mapping[effect]
+            params["enable"] = enable
+            params["brightness"] = brightness.brightness  # use the existing brightness
+
+        call.assert_called_with("set_lighting_effect", params)
+
+        await dev.update()
+        assert light_effect.effect == effect
+        assert feature.value == effect
+
+    with pytest.raises(ValueError):
+        await light_effect.set_effect("foobar")
+
+
+@light_strip_effect
+@pytest.mark.parametrize("effect_active", [True, False])
+async def test_light_effect_brightness(
+    dev: Device, effect_active: bool, mocker: MockerFixture
+):
+    """Test that light module uses light_effect for brightness when active."""
+    light_module = dev.modules[Module.Light]
+
+    light_effect = dev.modules[Module.SmartLightEffect]
+    light_effect_set_brightness = mocker.spy(light_effect, "set_brightness")
+    mock_light_effect_call = mocker.patch.object(light_effect, "call")
+
+    brightness = dev.modules[Module.Brightness]
+    brightness_set_brightness = mocker.spy(brightness, "set_brightness")
+    mock_brightness_call = mocker.patch.object(brightness, "call")
+
+    mocker.patch.object(
+        type(light_effect),
+        "is_active",
+        new_callable=mocker.PropertyMock,
+        return_value=effect_active,
+    )
+
+    await light_module.set_brightness(10)
+
+    if effect_active:
+        assert light_effect.is_active
+        assert light_effect.brightness == dev.brightness
+
+        light_effect_set_brightness.assert_called_with(10)
+        mock_light_effect_call.assert_called_with(
+            "set_lighting_effect", {"brightness": 10, "bAdjusted": True}
+        )
+    else:
+        assert not light_effect.is_active
+
+        brightness_set_brightness.assert_called_with(10)
+        mock_brightness_call.assert_called_with("set_device_info", {"brightness": 10})

kasa/tests/test_common_modules.py

@@ -89,35 +89,39 @@ async def test_light_effect_module(dev: Device, mocker: MockerFixture):
     assert light_effect_module.has_custom_effects is not None
 
     await light_effect_module.set_effect("Off")
-    assert call.call_count == 1
+    call.assert_called()
     await dev.update()
     assert light_effect_module.effect == "Off"
     assert feat.value == "Off"
+    call.reset_mock()
 
     second_effect = effect_list[1]
     await light_effect_module.set_effect(second_effect)
-    assert call.call_count == 2
+    call.assert_called()
     await dev.update()
     assert light_effect_module.effect == second_effect
     assert feat.value == second_effect
+    call.reset_mock()
 
     last_effect = effect_list[len(effect_list) - 1]
     await light_effect_module.set_effect(last_effect)
-    assert call.call_count == 3
+    call.assert_called()
     await dev.update()
     assert light_effect_module.effect == last_effect
     assert feat.value == last_effect
+    call.reset_mock()
 
     # Test feature set
     await feat.set_value(second_effect)
-    assert call.call_count == 4
+    call.assert_called()
     await dev.update()
     assert light_effect_module.effect == second_effect
     assert feat.value == second_effect
+    call.reset_mock()
 
     with pytest.raises(ValueError):
         await light_effect_module.set_effect("foobar")
-        assert call.call_count == 4
+        call.assert_not_called()
 
 
 @dimmable