Browse Source

fix: multiple bugs for lights with packed/masked dps (e.g. dp 51) (#4986)

* fix(device_config): handle null current value in masked write

Optional masked dps (e.g. Tuya dp 51 packed base64 with sub-field masks)
are only sent by some devices after first being written. Before this
change, the very first write to such a dp raised "Cannot mask unknown
current value" because get_values_to_set tried to OR the new bits onto
a None baseline.

Treat the unknown current value as zero so the masked OR produces a
clean standalone value with only the requested bits set. Once the
device echoes back its updated dp, subsequent writes use the real value
as their baseline.

Repro: Orison Chanfok Neo (orison_chanfok_neo_fan_light) — dp 51 is
optional and not reported until written. Calling light.turn_on on a
fresh-connected device errored out before the integration could send
anything.

* fix(light): always merge switch-on for masked switch dps

When a light's switch and other sub-fields (brightness, color, effect)
share the same dp id with different masks (e.g. Tuya dp 51 packed
base64), the previous `id not in settings` guard incorrectly skipped
the switch-on merge once any other branch had populated `settings[id]`.
The bulb ended up with brightness/color staged but switch byte still
zero — visually, it stayed off.

For masked dps, the merge is always safe — get_values_to_set with
pending_map=settings ORs the new bits onto the existing pending value.
The guard now allows the merge whenever the dp has a mask, falling
back to the original `id not in settings` check for unmasked dps.

Repro: Orison Chanfok Neo (orison_chanfok_neo_fan_light) — calling
light.turn_on(brightness=255, color_temp_kelvin=3868) on a cold-off
bulb wrote brightness/color to dp 51 but skipped the switch-on byte,
so the bulb stayed dark. Manual `light.turn_on` with no params worked
because nothing else populated settings first.

* fix(device): serialize light set operations on shared device

Two HA light entities that are sub-functions of the same physical Tuya
device (e.g. main light + RGB backlight, both writing to dp 51 with
different byte masks) race when invoked concurrently — typically by
HA's scene.turn_on, which calls async_reproduce_state on every entity
in the scene in parallel via asyncio.gather.

Each sub-entity does a read-modify-write: it reads dp 51's current
value, ORs its masked bits in, then writes the result via
async_set_properties. With concurrent calls, both reads see the same
baseline before either updates pending — then the second's pending
update clobbers the first's, and one entity's bits are lost.

Add a per-device asyncio.Lock and acquire it in light.async_turn_on /
async_turn_off so the read-modify-write windows serialize. The second
caller's get_values_to_set then sees the first's pending value via
device.get_property (which already merges cached_state with pending)
and ORs onto it correctly.

Repro: Orison Chanfok Neo (orison_chanfok_neo_fan_light) — a scene
that activates main light ON and backlight OFF in the same call left
the device reporting back as off (or in a wrong state) because the
parallel dp 51 writes raced.

* fix(orison_chanfok_neo): improve dp 51 handling, decouple light availability

Two improvements for the Orison Chanfok Neo ceiling fan/light:

1. Add `force: true` to the dp 51 main-light switch entry. The device
   only sends dp 51 after first being written, so without forcing it,
   `cached_state["51"]` stays absent on first connect and any masked
   write has to rely on the null-current-value fallback. Forcing it
   means dp 51 is requested explicitly each poll cycle.

2. Rename `name: available` to `name: power` on dp 20 for both light
   sub-entities (and update the corresponding `value_mirror`). dp 20
   doubles as the device's master power switch (already exposed as a
   separate `switch` entity named "Lights"). With the previous naming,
   when the device auto-toggled dp 20 off after both lights went off,
   both light entities went unavailable in HA — and the user couldn't
   turn either back on through HA. The light's switch state still
   falls back to dp 20 via value_mirror, but availability no longer
   gates on it. The "Lights" switch entity continues to work as a
   master cut-off if the user wants to use it.

* style(light): reformat masked-dp guards per ruff format

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* revert:  Treat unknown as 0.

Raise exception when current value is unknown instead of treating it as zero. Assuming values has no guarantee of being the correct behaviour in all cases, and may be downright dangerous in some cases.

* Remove test for unknown current value in masked hex

Removed test for reverted change

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Jason Rumney <make-all@users.noreply.github.com>
William Overton 2 days ago
parent
commit
bb6dcb63d9

+ 6 - 0
custom_components/tuya_local/device.py

@@ -157,6 +157,12 @@ class TuyaLocalDevice(object):
         # The number of failures from a working protocol before retrying other protocols.
         self._AUTO_FAILURE_RESET_COUNT = 10
         self._lock = Lock()
+        # Serialises the read-modify-write window for entities that share a
+        # packed dp (e.g. multiple sub-entity lights writing to dp 51 with
+        # different masks). Without this, two concurrent async_turn_on calls
+        # both read the same baseline before either updates pending, then the
+        # second's pending update clobbers the first.
+        self._set_lock = asyncio.Lock()
 
     @property
     def name(self):

+ 4 - 3
custom_components/tuya_local/devices/orison_chanfok_neo_fan_light.yaml

@@ -38,15 +38,16 @@ entities:
     dps:
       - id: 20
         type: boolean
-        name: available
+        name: power
       - id: 51
         type: base64
         mask: "000100000000000000000000"
         optional: true
+        force: true
         name: switch
         mapping:
           - dps_val: null
-            value_mirror: available
+            value_mirror: power
       - id: 21
         type: string
         name: work_mode
@@ -98,7 +99,7 @@ entities:
     dps:
       - id: 20
         type: boolean
-        name: available
+        name: power
       - id: 51
         type: base64
         mask: "000200000000000000000000"

+ 1 - 1
custom_components/tuya_local/helpers/device_config.py

@@ -1109,7 +1109,7 @@ class TuyaDpsConfig:
 
             if decoded_value is None:
                 raise ValueError("Cannot mask unknown current value")
-            elif isinstance(decoded_value, int):
+            if isinstance(decoded_value, int):
                 current_value = decoded_value
                 result = (current_value & ~mask) | (mask & int(result * mask_scale))
                 # Only convert back to bytes if the DP is actually hex/base64

+ 24 - 3
custom_components/tuya_local/light.py

@@ -285,6 +285,10 @@ class TuyaLocalLight(TuyaLocalEntity, LightEntity):
             return best_match
 
     async def async_turn_on(self, **params):
+        async with self._device._set_lock:
+            await self._async_turn_on_locked(**params)
+
+    async def _async_turn_on_locked(self, **params):
         settings = {}
         color_mode = None
         _LOGGER.debug("Light turn_on: %s", params)
@@ -501,11 +505,19 @@ class TuyaLocalLight(TuyaLocalEntity, LightEntity):
                     ),
                 }
 
+        # On packed dps where switch / brightness / effect all share the same
+        # dp id (e.g. dp 51 with different masks), the original `id not in
+        # settings` check incorrectly skipped the switch-on merge once any
+        # other masked sub-field had populated settings. For masked dps it's
+        # always safe to merge — get_values_to_set with pending_map=settings
+        # will OR onto the existing pending value.
         if (
             self._switch_dps
             and not self._switch_dps.readonly
             and not self.is_on
-            and self._switch_dps.id not in settings
+            and (
+                self._switch_dps.mask is not None or self._switch_dps.id not in settings
+            )
         ):
             _LOGGER.info("%s turning light on", self._config.config_id)
             settings = settings | self._switch_dps.get_values_to_set(
@@ -514,7 +526,10 @@ class TuyaLocalLight(TuyaLocalEntity, LightEntity):
         elif (
             self._brightness_dps
             and not self.is_on
-            and self._brightness_dps.id not in settings
+            and (
+                self._brightness_dps.mask is not None
+                or self._brightness_dps.id not in settings
+            )
         ):
             bright = 255
             r = self._brightness_dps.range(self._device)
@@ -532,7 +547,9 @@ class TuyaLocalLight(TuyaLocalEntity, LightEntity):
             self._effect_dps
             and not self.is_on
             and "off" in self._effect_dps.values(self._device)
-            and self._effect_dps.id not in settings
+            and (
+                self._effect_dps.mask is not None or self._effect_dps.id not in settings
+            )
         ):
             # Special case for lights with effect that has off state, but no switch or brightness
             on_value = self._effect_dps.default
@@ -551,6 +568,10 @@ class TuyaLocalLight(TuyaLocalEntity, LightEntity):
             await self._device.async_set_properties(settings)
 
     async def async_turn_off(self):
+        async with self._device._set_lock:
+            await self._async_turn_off_locked()
+
+    async def _async_turn_off_locked(self):
         if self._switch_dps and not self._switch_dps.readonly:
             _LOGGER.info("%s turning light off", self._config.config_id)
             await self._switch_dps.async_set_value(self._device, False)

+ 51 - 0
tests/test_light.py

@@ -152,6 +152,57 @@ async def test_async_turn_on_with_white_param():
     mock_device.async_set_properties.assert_called_once_with({"2": "white", "3": 506})
 
 
+@pytest.mark.asyncio
+async def test_async_turn_on_with_brightness_on_packed_dp():
+    """Switch-on must merge cleanly when the dp is shared across sub-fields.
+
+    On packed dps where switch / brightness / color all share the same dp id
+    (e.g. dp 51 with different masks), the switch-on branch was previously
+    skipped once the brightness branch had populated `settings[dp_id]`,
+    leaving the bulb with brightness staged but not actually on.
+
+    For masked switch dps, the merge is always safe — `get_values_to_set`
+    with `pending_map=settings` ORs onto the existing pending value — so the
+    final write contains the switch byte AND the brightness bytes.
+    """
+    mock_device = AsyncMock()
+    mock_device.get_property = Mock()
+    # Bulb currently off: switch byte (mask 0001) is 0, brightness is 0.
+    dps = {"1": "000000000000"}
+    mock_device.get_property.side_effect = lambda arg: dps[arg]
+    mock_config = Mock()
+    config = TuyaEntityConfig(
+        mock_config,
+        {
+            "entity": "light",
+            "dps": [
+                {
+                    "id": "1",
+                    "name": "switch",
+                    "type": "hex",
+                    "mask": "000100000000",
+                },
+                {
+                    "id": "1",
+                    "name": "brightness",
+                    "type": "hex",
+                    "mask": "0000FFFF0000",
+                    "range": {"min": 0, "max": 1000},
+                },
+            ],
+        },
+    )
+    light = TuyaLocalLight(mock_device, config)
+    await light.async_turn_on(brightness=255)
+    mock_device.async_set_properties.assert_called_once()
+    sent = mock_device.async_set_properties.call_args[0][0]
+    sent_value = int(sent["1"], 16)
+    # brightness bytes set
+    assert sent_value & 0x0000FFFF0000 != 0
+    # switch byte also set (this is the bug — was zero before the fix)
+    assert sent_value & 0x000100000000 != 0
+
+
 @pytest.mark.asyncio
 async def test_is_off_when_off_by_brightness():
     """Test that the light appears off when turned off by brightness."""