search

MycroftAI / skill-homeassistant

Mycroft Skill/Integration for Homeassistant
GNU Lesser General Public License v3.0
114 stars 62 forks source link

Feature/rgb lights #69

Open xiki808 opened 3 years ago

xiki808 commented 3 years ago

Say: change boxroom light to blue - to trigger the color change.

The 'to' is key, that separates the entity from the color, and so it allows for example: change boxroom light TO light blue.

Tony763 commented 3 years ago

Nice, I will send you a PR with Czech translation tomorrow.

Tony763 commented 3 years ago

@xiki808 PR sended.

stratus-ss commented 3 years ago

I'll take a look at this shortly.

We are trying to encourage people to put more effort into the descriptions of the pull requests. We made a video about it here

stratus-ss commented 3 years ago

This one passes all the tests which is expected.

The all function also works as expected and individual lights also work as expected

However, I dont know if this is a problem with this PR or the skill itself, but when I call an entity that doesn't exist it seems to grab a random light... for example:

change bathroom light to orange                                 --- 0.21
 >> LaundryRoom changed to orange.  
change outside light to green                             
 >> Steve's Bed Light is  unavailable.

Do you have the same results @xiki808 ?

stratus-ss commented 3 years ago

Let's get some tests added for the new functionality. You can find the existing tests under homeassistant.mycroftai/test/intent

Let us know if you need help with that!

Tony763 commented 3 years ago

@xiki808 If you need help with tests, ping me.

xiki808 commented 3 years ago

Thanks guys, will have a look into this early this week.

xiki808 commented 3 years ago

@stratus-ss For me it is working fine, I tried the following:

change test light to blue
>> Changed Test to blue.
change desk light to red
>> Sorry, I can't find the Home Assistant entity "desk light".
change test light to red
>> Changed Test to red.
turn off test
>> turned Test off.
change test to red
>> Test changed to red.
turn off test
>> turned off Test.
change desk to blue
>> Excuse me, I wasn't able to find "desk".

I will try to find some time to try with the same naming.

xiki808 commented 3 years ago

@Tony763 would love it if you can handle the test, I cannot see how to run and check my test intent.

Tony763 commented 3 years ago

Hi @xiki808 , Kris just merged two of my commits so there is a conflict in init.py now, but I can't see it. Please, check it and try to fix or send me a screenshot. Maybe a fetching upstream could be sufficient. If anything, I will gladly help.

xiki808 commented 3 years ago

I don't have the setup currently, been using the Pi for other projects. I will try to reinstall Mycroft next week and check it!

Tony763 commented 2 years ago

Hi @xiki808 were you able to reinstall Mycroft? If not, could I help you?

xiki808 commented 2 years ago

Hi @Tony763, I haven't had the time yet, but will find some time this week

Tony763 commented 2 years ago

Hi @krisgesling, merge issue solved and tested. Seems Ok to me. image

Tony763 commented 2 years ago

Hi @xiki808 and @krisgesling I tested it again with latest commit and it's working.

I have only one thing that could be improved. When I ask Mycroft to change color of light that does not support colors, it still say that color was changed.

Found entity_id contain attributes and for lights there is supported_color_mode. https://github.com/MycroftAI/skill-homeassistant/blob/715bc81e7703a4b572051b8a24c18ff493579eb7/ha_client.py#L66

Without support: image

With support: image

As defined in HA light, color can be changed only when at least one of hs, xy, rgb, rgbw, rgbww modes is present and if not, Mycroft could inform uses that color change is not supported.

https://github.com/home-assistant/core/blob/bc59387437cea91d675584bb9057338aa5992817/homeassistant/components/light/__init__.py#L53-L64

I would suggest to pass whole attributes dictionary to __init__.py as same thing will more-less be happening in future with others intents.

What do You think?

stratus-ss commented 2 years ago

How does this handle rgbww and/or rgbw?

Tony763 commented 2 years ago

Hi @stratus-ss , long not hear from You :). Color is directly passed to HA, so handling is done in side HA not in skill. image

This way with rgbw and rgbww only rgb part is controlled. White and temperature of white is omitted. If its on, it stay on or off. Brightness of color can be controlled by set intensity intent.

image

Although I just realized that this will perfectly work in English. But in other languages, HA will not understood to color names!

stratus-ss commented 2 years ago

Let me see if I can carve out some time this week to test this out now that you folks have been doing some good cleanup work

Extarys commented 2 years ago

For the color issue, is there a way to make a dummy mycroft skill so users can translate the colors name on Mycroft Translate and use those inside other skills?

krisgesling commented 2 years ago

Hey @Extarys - that's not a bad idea

You could make a Color Skill that displays the color along with the RGB and/or hex values. Submit this to the Marketplace and it will get added to translate.mycroft.ai, you can then poach the translated colors back out of it.

Tony763 commented 2 years ago

Or maybe we could try auto translate. I have seen it in some other skill. One word translation should not be big challenge.

stratus-ss commented 2 years ago

The CSS3 color spec has 140(ish) colors that might be hard to translate one at a time

Extarys commented 2 years ago

@stratus-ss I wouldn't mind translating them in my own language, it would not take that much time and the CSS color name has never changed much. so it would be once per language and would last 25+ year :joy: Of course it wouldn't be done in 5 minutes so it still require some efforts.

Ppl can probably use LibreTranslate or Google Translate too as it should be simple for those automated tools - it's not a complicated sentence or something.

Very Mycroft noob so not sure how the auto translate thing would work, but I have no idea how to make a plugin so I would probably leave that to a user who wont take a month to make it :smile: but I won't mind translating it in french and then other languages with automated tools.

Tony763 commented 2 years ago

I have seen auto translate function in one of skills I translated last year. It was quite easy to use. I will try to find it and we can try it and compare results.

stratus-ss commented 2 years ago

Gez and I were discussing this one and we were wondering how non-english speakers would handle most of the colors. For example "light golden rod" or similar words would not translate automatically and would require someone versed in colours to be able to translate them appropriately.

Speaking for myself I dont know what colours most of these are (just reading the names) and I am a native speaker. So the question is, would users learn their native words for these bizarre colours, or would they just learn the english name and call it a day?

krisgesling commented 2 years ago

Also wanted to flag that we've started a Color Picker Skill for an upcoming video series - so we can get an early MVP version of that into the Marketplace and start getting colors translated if it would be helpful.

Currently the colors are included as their exact CSS3 forms eg "lightgoldenrodyellow" rather than "light golden rod yellow" but thinking through the best way to approach this and hence the question above.

If you're a German speaker and want to change your lights to a "lightgoldenrodyellow" color (#fafad2), what would you say?

krisgesling commented 2 years ago

I've updated the color names to have spaces - much easier to remove spaces programmatically.

It's also nearing an MVP so should be able to push it to the Marketplace soonish. It's here if you want to check it out: https://github.com/krisgesling/color-picker-skill

Extarys commented 2 years ago

Nice! I don't see it yet on https://translate.mycroft.ai/ but I'll help translating to french as soon as I see it there :heart:

pfefferle commented 2 years ago

Is there something I can help to get this merged?

krisgesling commented 2 years ago

@pfefferle if you can verify the functionality works as expected that would be great. It didn't seem to be working for @stratus-ss but there might have been updates since that comment

It would be great to add a check to see if the given device supports color as suggested by Tony here: https://github.com/MycroftAI/skill-homeassistant/pull/69#issuecomment-929535979

But I'd be comfortable with that being a follow up PR.

arborealfirecat commented 1 year ago

Hey all, sorry to necro an old PR, but are you still open to merging this functionality? If so I'm happy to pick it up and finish off the last bits of the PR?

stratus-ss commented 1 year ago

Hey all, sorry to necro an old PR, but are you still open to merging this functionality? If so I'm happy to pick it up and finish off the last bits of the PR?

@krisgesling and I are still floating around. Speaking for myself, I don't mind continuing to approve PRs

stratus-ss commented 1 year ago

I re-looked at this. There is a lot of whitespace noise in this PR.

overall I don't have any major concerns. If there is still interest and the original author isn't involved we can get a new PR opened