Rewrite BulbDevice and rework set_multiple_values() #525

uzlonewolf · 2024-07-30T15:14:37Z

This is a pretty big rewrite to BulbDevice. The fact that it connects to the bulb and calls status() as soon as the version is set has always bugged me, so now it delays that until a command is sent. If a user calls status() themselves before they send a command then it uses the result from that status call instead of calling it again.

Also included are modifications to core: a caching mechanism has been added so devices can try to use cached DP values instead of making calls to the device, and set_multiple_values() has been modified to try and detect devices which fall over if they get sent multiple DPs in a single request. The caching only works when an active persistent connection is open; closing the connection clears the cache since we won't get notified when DPs change.

To access the cache, call d.cached_status(nowait). When nowait=True then the cached value is returned if it is available or None if it is not. When nowait=False then the cached value is returned if it is available or a call to d.status() is made if it is not.

BulbDevice now leans on the cache to try and minimize the number of DPs set in set_white() and set_colour(). This combined with the set_multiple_values() change is to help with bulbs that only accept 1 DP at a time, such as @Anonymouse83's in #504.

Todo: update documentation.

… building the payload directly

… handle 1 DP at a time

jasonacox · 2024-08-01T03:38:39Z

tinytuya/BulbDevice.py

+        result = super(BulbDevice, self).status(nowait=nowait)
+        if result and (not self.bulb_type) and (self.DPS in result):
+            self.detect_bulb(result)
+        return result


I like this! clean.

jasonacox · 2024-08-01T03:43:40Z

tinytuya/BulbDevice.py

+            for rgb_value in rgb:
+                value = int(rgb_value)
+                if value < 0 or value > 255:
+                    raise ValueError(f"Bulb type {bulb} must have RGB values 0-255.")


This is needed, but it occurs to me that we have a real mix of using python exceptions versus returning error payloads, or actually not doing any validation. This is more of a commentary on some of our global style/design we could address rather than specific to this PR. :)

jasonacox · 2024-08-01T03:45:15Z

tinytuya/BulbDevice.py

-            if len(hexvalue_hsv) == 7:
-                hexvalue = hexvalue + "0" + hexvalue_hsv
-            else:
-                hexvalue = hexvalue + "00" + hexvalue_hsv


I love that we are addressing this. This is original pytuya code and more complicated than it needs to be. Thank you.

jasonacox · 2024-08-01T03:47:07Z

tinytuya/BulbDevice.py

+            self.has_colour = has_colour
+        elif self.has_colour is None:
+            self.has_colour = bool(self.DPS_INDEX_COLOUR[self.bulb_type])
+


❤️ this

jasonacox · 2024-08-01T03:52:02Z

tinytuya/BulbDevice.py

-        data = self._send_receive(payload, getresponse=(not nowait))
-        return data
+
+        return self.set_colour( r * 255.0, g * 255.0, b * 255.0, nowait=nowait )


Also, clean API change. I think there is enough API change here we need to signal breaking change with the version number we pick.

I don't think there are any API changes? Most of this is function de-duplication and code cleanup. I'll double-check but I think the only user-visible changes are 1) manually reading d.bulb_type will read None until either d.status() is called or a command is sent 2) unknown bulbs now default to type 'B' instead of 'A' and 3) a force argument has been added to set_colour() and set_white() to disable some of the new optimizations, however this feels like a dirty hack to me and I'm probably going to rework it to remove this.

Fair. Those are minor. Thinking of old code out there that may be impacted by those two things is very edge case. I'm good with a minor update. I had a stronger impression of the change when I was originally doing the review, but feel less concerned now.

jasonacox · 2024-08-01T03:57:28Z

tinytuya/BulbDevice.py

+        payload = {
+            self.DPS_INDEX_BRIGHTNESS[self.bulb_type]: brightness,
+            self.DPS_INDEX_COLOURTEMP[self.bulb_type]: colourtemp,
+        }


Instead of payload should we move to dps_values or something like that to not confuse this with raw "payloads"?

jasonacox · 2024-08-01T04:01:41Z

tinytuya/BulbDevice.py

                # for colour mode use hsv to increase brightness
                if self.bulb_type == "A":
                    value = brightness / 255.0
                else:
                    value = brightness / 1000.0
                (h, s, v) = self.colour_hsv()
                data = self.set_hsv(h, s, value, nowait=nowait)
+                msg += 'No repsonse from bulb.'
+            else:
+                msg += "Device mode is not 'white' or 'colour', cannot set brightness."


What about?...

f"Unable to set brightness, device mode needs to be 'white' or 'colour' but reports {state["mode"]}"

jasonacox · 2024-08-01T04:03:33Z

tinytuya/BulbDevice.py


        if data is not None or nowait is True:
            return data
        else:
-            return error_json(ERR_STATE, "set_brightness: Unknown bulb state.")
+            log.debug( msg )
+            return error_json(ERR_STATE, msg)


I mentioned how we use python exceptions in some cases and error responses in other place. But thinking about it, it seems we do this:

If user provides invalid parameters (values) - raise exception

If buld responds with error or invalid conditions - respond with error_json()

I think that makes sense. We may need to do scan the rest of our code to ensure that is consistent.

jasonacox · 2024-08-01T04:08:15Z

tinytuya/BulbDevice.py

        return BulbDevice._hexvalue_to_hsv(hexvalue, self.bulb_type)

    def state(self):
        """Return state of Bulb"""
-        status = self.status()
+        status = self.cached_status()


I like the idea of caching. But what if the user is making changes to the device using the SmartLife app or other tool?

We should definitely cache the device type and capabilities.

Status or current state of device should be live (unless we provide a force=False)?

Thoughts?

But what if the user is making changes to the device using the SmartLife app or other tool?

Every device I've seen sends asynchronous updates when that happens, so as long as the user calls .receive() every now and then or sends a command with nowait=False then those changes will be picked up and cached. This is also why caching is only enabled when persist=True and an active connection to the device is open.

jasonacox · 2024-08-01T04:19:09Z

tinytuya/core.py

@@ -820,7 +820,7 @@ def assign_dp_mappings( tuyadevices, mappings ):

 class XenonDevice(object):
    def __init__(
-            self, dev_id, address=None, local_key="", dev_type="default", connection_timeout=5, version=3.1, persist=False, cid=None, node_id=None, parent=None, connection_retry_limit=5, connection_retry_delay=5, port=TCPPORT # pylint: disable=W0621
+            self, dev_id, address=None, local_key="", dev_type="default", connection_timeout=5, version=3.1, persist=False, cid=None, node_id=None, parent=None, connection_retry_limit=5, connection_retry_delay=5, port=TCPPORT, max_simultaneous_dps=0 # pylint: disable=W0621


Should caching be an option (that can be turned off) in addition to persist=True?

Also, we should break up the super long line with some \n love.

I don't see a point in disabling it. Cached data is only returned with .cached_status() so if you don't want cached data then just call .status(). Enabling it without persist=True is going to cause the cached state to not match the device state if someone presses a button on the device or makes a change in the SmartLife app.

jasonacox · 2024-08-03T15:28:05Z

I think this is a good update @uzlonewolf - ready for non-draft PR IMHO.

uzlonewolf · 2024-08-13T22:13:49Z

@jasonacox Do you happen to have an example status() response for a Type C bulb? I want to check some of my logic for that bulb type before committing this.

jasonacox · 2024-08-14T03:57:48Z

Hi @uzlonewolf , makes sense. However, I don't seem to have one. The Type C Bulb addition came from #54 by @davidlie (who may be able to test for us?) and I recall it was for the Feit Electric Smart Light Bulb. I see some on Amazon but not sure if that would be the C type. I thought I picked one up but scanned my devices and no Type C's. 😞

uzlonewolf · 2024-08-14T17:08:20Z

Looking at that PR it looks to be a dimmer switch and not a bulb at all. Using the Internet Archive I found some that look like they're probably it and ordered a couple new-old-stock off ebay. We'll see in a few days.

jasonacox · 2024-08-15T03:00:44Z

Yes, I can't recall why it said dimmer switch but PR was for a smart bulb. It wasn't that many years ago, but long enough fo rme to completely forget. 🤷 Thanks for doing the research. 🙏

uzlonewolf · 2024-08-19T16:18:59Z

My dimmer switch came in Saturday and it does identify as a Type C bulb. When it comes down to it, is there really a difference between a dimmer and a single-color (non-CCT) bulb? Anyway, I also have some single-color as well as CCT bulbs arriving today.

The more I think about it the less happy I am with the entire "Type X" thing and am probably going to rewrite it to just use "has feature" (mode, brightness, colortemp, etc). I may or may not try and use the DPS mapping (if it's available) instead of blindly guessing based off DP ID.

jasonacox · 2024-08-25T15:24:06Z

Yes, I haven't been fond of the Type X thing either, but I don't know of a better way to represent both the DPS mapping as well as the range differences.

tinytuya/tinytuya/BulbDevice.py

Lines 117 to 127 in 7d8922e

    
                   Convert an RGB value to the hex representation expected by Tuya Bulb. 
        
                   Index (DPS_INDEX_COLOUR) is assumed to be in the format: 
        
                       (Type A) Index: 5 in hex format: rrggbb0hhhssvv 
        
                       (Type B) Index: 24 in hex format: hhhhssssvvvv 
        
                   While r, g and b are just hexadecimal values of the corresponding 
        
                   Red, Green and Blue values, the h, s and v values (which are values 
        
                   between 0 and 1) are scaled: 
        
                       Type A: 360 (h) and 255 (s and v) 
        
                       Type B: 360 (h) and 1000 (s and v)

uzlonewolf · 2024-08-26T03:10:59Z

Last week I ended up rewriting BulbDevice and almost completely eliminated the Type X thing. I left it in a few places for backwards compatibility and as a quick way to manually set the DP range, but it's otherwise gone. The RGB/HSV range is now selected by a boolean, and the "has X capability" is set by whether a DP is defined or not. I'll push an update once I have the DP detection finished.

I picked up an assortment of bulbs to test with. The new Feit single-color white bulbs are v3.5 and act like what was formerly known as Type B but with a few DPs missing. The older Feit dimmer uses DPs 1-3 with 3 being the "minimum dim" setting, while Geeni bi-color/CCT white only bulbs use DPs 1-3 with 3 being CCT.

uzlonewolf added 6 commits July 13, 2024 17:35

Rework BulbDevice to use set_value()/set_multiple_values() instead of…

1f07086

… building the payload directly

Add status caching to core

059625c

Make BulbDevice prefer cached status when available

cacb519

Rewrite BulbDevice

f9c14fe

Add force flag to set_colour() and set_white()

edad81d

Rework set_multiple_values() to try and detect devices which can only…

c1864f4

… handle 1 DP at a time

jasonacox reviewed Aug 1, 2024

View reviewed changes

uzlonewolf added 4 commits August 5, 2024 02:31

Cache tweaks and pylint cleanup

1da9322

Documentation update

d78e86e

Cache tweaks, documentation, and cleanup

17fd66a

Merge branch 'master' into BulbFix

447ea78

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite BulbDevice and rework set_multiple_values() #525

Rewrite BulbDevice and rework set_multiple_values() #525

uzlonewolf commented Jul 30, 2024

jasonacox Aug 1, 2024

jasonacox Aug 1, 2024

jasonacox Aug 1, 2024

jasonacox Aug 1, 2024

jasonacox Aug 1, 2024

uzlonewolf Aug 1, 2024

jasonacox Aug 3, 2024

jasonacox Aug 1, 2024

jasonacox Aug 1, 2024

jasonacox Aug 1, 2024 •

edited

Loading

jasonacox Aug 1, 2024

uzlonewolf Aug 1, 2024

jasonacox Aug 3, 2024

jasonacox Aug 1, 2024

uzlonewolf Aug 1, 2024

jasonacox commented Aug 3, 2024

uzlonewolf commented Aug 13, 2024

jasonacox commented Aug 14, 2024

uzlonewolf commented Aug 14, 2024

jasonacox commented Aug 15, 2024

uzlonewolf commented Aug 19, 2024

jasonacox commented Aug 25, 2024

uzlonewolf commented Aug 26, 2024

Rewrite BulbDevice and rework set_multiple_values() #525

Are you sure you want to change the base?

Rewrite BulbDevice and rework set_multiple_values() #525

Conversation

uzlonewolf commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonacox Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonacox commented Aug 3, 2024

uzlonewolf commented Aug 13, 2024

jasonacox commented Aug 14, 2024

uzlonewolf commented Aug 14, 2024

jasonacox commented Aug 15, 2024

uzlonewolf commented Aug 19, 2024

jasonacox commented Aug 25, 2024

uzlonewolf commented Aug 26, 2024

jasonacox Aug 1, 2024 •

edited

Loading