Add support for setting RGB and RGBW values for Twinkly lights

[RobBie1221]

Breaking change

Proposed change

Change library from twinkly-client to ttls. It covers same functionality and more and has required functions to set RGBW values.
Add support in the light entity for setting RGBW values.
Update all tests.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[probot-home-assistant]

Hey there @dr1rrb, mind taking a look at this pull request as it has been labeled with an integration (twinkly) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[pvizeli]

Nice. Is also possible to read effect and let them choose with the library?

[RobBie1221]

Maybe...to set fixed color mode "movie" is used with 1 single frame with a fixed color. There is a mode "effect", but I didn't test that yet.

I did forget one thing by the way when submitting this PR, there is twinkly RGB, RGBW and AWW. I need to add that it reads the available mode and set color_mode accordingly.

[dr1rrb]

Change library from twinkly-client to ttls. It covers same functionality and more and has required functions to set RGBW values.
Add support in the light entity for setting RGBW values.

@RobBie1221 😍😍😍

Nice. Is also possible to read effect and let them choose with the library?

@pvizeli From my last investigations last year, unfortunately not really: effects are not stored on the device, they are sent by the app each time. And when you "group" and "map" multiple devices together, effects are "computed" by the app before being sent, so it's not a mater of capturing all of them statically and embed them in the component.

The single option I see would be to have a "listening mode" which would somehow act as a "an in the middle" to capture effects sent by the app, store them on HA and then expose them as "light effects" in HA ...

[dr1rrb]

Hey @RobBie1221 does the ttls.client supports the "Playlist" mode? It's a feature I completely missed by the time which would be great to support (cf. dr1rrb/py-twinkly-client#1 and #62370).

(BTW when ready I do have RGB and RGBW devices, I will be more than happy to test it)

[RobBie1221]

Hey @RobBie1221 does the ttls.client supports the "Playlist" mode? It's a feature I completely missed by the time which would be great to support (cf. dr1rrb/py-twinkly-client#1 and #62370).

Don't know, need to check that.

(BTW when ready I do have RGB and RGBW devices, I will be more than happy to test it)

I do have RGBW, but don't have RGB, so it would be good to have someone who can test that.

[matejdro]

Yeah, twinkly integration with home assistant presents at least two difficult challenges:

1. There is no way to enter playlist mode. Twinkly lights have (at least) three modes of operation: movie, playlist and off. When you just select an effect from the app, it switches to the movie mode. But when you start a playlist, it switches to playlist mode. However, turning lights off and on in home assistant will first change its mode to off and then back to movie, which disables the playlist. I'm not sure what would be a good way to map these three modes to two-state home assistant switches. Maybe some kind of config option for default mode and/or parameter on turn_on service?

2. Twinkly lights can only be connected to one client at a time. When a client connects, it kicks all others out. This presents a problem when attempting to upload a playlist onto device from the app. This process is very slow, so it can take up to a minute. But home assistant polls more frequently than that, killing your upload. It's essentially impossible to upload any nontrivial playlist when home assistant entity is active, since its polling keep disconnecting the app. Maybe we could work around that by adding a switch to twinkly lights that disables polling?

I'm not saying these should be resolved right now (this PR has already improved twinkly situation significantly as it resolves the brightness changing killing playlist), I just want to bring them to your attention since we appear to have a new Twinkly maintainer.

[dr1rrb]

Yeah, twinkly integration with home assistant presents at least two difficult challenges:

1. [...] Maybe some kind of config option for default mode and/or parameter on `turn_on` service?

Great idea

2. Twinkly lights can only be connected to one client at a time. When a client connects, it kicks all others out. This presents a problem when attempting to upload a playlist onto device from the app. This process is very slow, so it can take up to a minute. But home assistant polls more frequently than that, killing your upload. It's essentially impossible to upload any nontrivial playlist when home assistant entity is active, since its polling keep disconnecting the app. Maybe we could work around that by adding a switch to twinkly lights that disables polling?

My idea for this was to add a basic flag/switch to disable the polling for a given time (like 20 min). Then we could also add some basic logic like when we get a 401 we automatically enable that flag, and when we have a "real" command from HA we auto-disable it.

But this year there is a new setting in the Twinkly App which sounds promising: you can make your device discoverable to homekit. I would assume that we would be able to avoid the "auth conflict" issue.

[wicol]

Maybe...to set fixed color mode "movie" is used with 1 single frame with a fixed color.

Can I ask why it'd be preferable doing this over using the "color" mode?

[RobBie1221]

Maybe...to set fixed color mode "movie" is used with 1 single frame with a fixed color.

Can I ask why it'd be preferable doing this over using the "color" mode?

What color mode? These I know off:

https://github.com/jschlyter/ttls/blob/707e860109f7aacc613e8d9aa98038498d9e37ff/ttls/client.py#L47

[wicol]

What color mode? These I know off:

https://github.com/jschlyter/ttls/blob/707e860109f7aacc613e8d9aa98038498d9e37ff/ttls/client.py#L47

I got it from here:
https://xled-docs.readthedocs.io/en/latest/rest_api.html#get-led-operation-mode
https://xled-docs.readthedocs.io/en/latest/rest_api.html#set-led-color

I used these successfully with my RGBW and ended up creating a PR for twinkly-client. Then I found this PR haha "oops".

[RobBie1221]

I think that API is new. Anyway it would take major redesign to get it working like that. The integration works at the moment that "on" means set mode "movie". If we are going to support multiple modes, we need a design to incorporate that.

The simplicity of this solution is that nothing changes to which modes are set. I think in the end it may be nicer to use the "color" mode, but that needs more work. Also it needs to be in a library. Perhaps you can make a PR against the ttls library, that one seems to get regular updates.

[wicol]

I think that API is new. Anyway it would take major redesign to get it working like that. The integration works at the moment that "on" means set mode "movie". If we are going to support multiple modes, we need a design to incorporate that.

I see. I just got my twinkly last week, so that's when I started to look around for options and libs. I saw some ppl playing with xled and xled_plus, and then focused on twinkly-client since that's the one used by HA. I completely missed ttls though 🙄

The simplicity of this solution is that nothing changes to which modes are set. I think in the end it may be nicer to use the "color" mode, but that needs more work. Also it needs to be in a library. Perhaps you can make a PR against the ttls library, that one seems to get regular updates.

I agree, this should get merged, given that it's a major improvement over the current implementation and ppl (including myself) want this stuff asap 😄 . Meanwhile I'll try to play around with ttls and see if I can add color mode to it 👍

[pvizeli]

Well not all support RGBW, we should handle it dynamic and only show the support if they are present over an property

[RobBie1221]

Yep, you're right. It got burried a bit over the numerous comments. It's on my list to adjust.

[pvizeli]

All comments have to stop now, by side of the code review. Feel free to open a forum topic or an issue for future discussion.

[pvizeli]

You can also remove line 132 - 141 - That is wrong extra_state_attributes

[RobBie1221]

You can also remove line 132 - 141 - That is wrong extra_state_attributes

I noticed that there is still a lot of review comments open on the initial PR for the integration, which I think also adresses this. Maybe make a separate PR to cleanup those comments?

[RobBie1221]

You can also remove line 132 - 141 - That is wrong extra_state_attributes

@pvizeli No problem with removing that, but isn't that a breaking change for people using that? Shouldn't we do that in a separate pull?

[RobBie1221]

I reworked it a bit:

• It now defaults to COLOR_MODE_BRIGHTNESS. For devices reporting rgb or rgbw, it will change color mode to the matching mode.
• I cleaned up some bits and pieces as mentioned here: Add twinkly integration #42103 (review) , mostly things that I touched anyway, but not everything (I would propose to do this in a follow-up PR)
• Brightness now uses attribute, but there is more cleaning up to do (I would propose in a follow-up PR, not to make this any bigger than it's already is)

[pvizeli]

Looks a lot better as that code before :D

[MartinHjelmare]

Nice! All my comments pertain to code that was already there from earlier contributions. Just noting them again, to help with possible future clean up efforts. ❤️

[RobBie1221]

I'll make a follow up PR :)