search

MycroftAI / skill-homeassistant

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

Basic cover support #80

Closed pfefferle closed 2 years ago

pfefferle commented 2 years ago

Description

Basic implementation of cover_open, cover_close and cover_stop

Type of PR

If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR. Either delete those that do not apply, or add an x between the square brackets like so: - [x]

Testing

Say "Can you close {Entity} please?" or "Can you open {Entity} please?"

Tony763 commented 2 years ago

Will send you a PR with Czech translations, tonight. Also, cover entity state could by read with sensor intent when domain is added, but here should be also intent to get position of cover (and tilt if supported) and set it. 'current_cover_position', 'current_cover_tilt_position' https://developers.home-assistant.io/docs/core/entity/cover/

Tony763 commented 2 years ago

Also we should think about a way to check if cover was really closed/opened as safety can be involed (Garage doors). That fuction should be universal, so we can use ot with other entties like locks.

pfefferle commented 2 years ago

Also we should think about a way to check if cover was really closed/opened as safety can be involed (Garage doors). That fuction should be universal, so we can use ot with other entties like locks.

hmmm... I am not sure about that, because none of my covers has a valid way to check if it really is open/closed...

Tony763 commented 2 years ago

Each entity that is somehow activated return its state. For example switch. You can turn it on/off but also You can retrieve it's state: https://github.com/MycroftAI/skill-homeassistant/blob/2f8b7a452d22fe7abd9b53b89cb38af42c90837f/__init__.py#L458

Now we just execute turn on/off action and say it's done if no error occur. My option: For covers and locks I would rater execute service, inform user we executed command and spawn some type of timeout which after done would read state of entity and confirm that command was un/successfully done.

This way it would be only up to HA component, how they handle security manner.

pfefferle commented 2 years ago

@Tony763 @krisgesling With the last commit, you can check the state of a cover. Is it possible to translate the open/close returned by home assistant?

Tony763 commented 2 years ago

It would be fine to report open/opening/close/closing in selected language. I think I have seen a function for this somewhere. I will try to find it.

Tony763 commented 2 years ago

Didn't find, I think, ti should be done as voc_match, but in reverse.

We would create 4 voc files with name of status and put translates states into them. Then when state is returned, we try to check if returned state match name of voc file and if yes, take random word from it (in case multiple words in voc). What do you think?

krisgesling commented 2 years ago

I think the preferred way would be to have an open.dialog and close.dialog then you could call:

self.translate(action)

docs for that method here

Alternatively you could have a comma separated listed of actions in an actions.value file then create a map of those values:

action_vocab = self.translate_namedvalues('actions')
close = action_vocab['close']

But yeah, the first option I think is better.

pfefferle commented 2 years ago

thanks @krisgesling !

what if there is no matching file? does it return the text param? I ask, because there are sensors, that return for example a string, that should not be translated (for example thermostats).

Tony763 commented 2 years ago

You could limit it by entity domain.

pfefferle commented 2 years ago

@Tony763 makes sense!

pfefferle commented 2 years ago

@Tony763 @krisgesling I decided to use different dialogues for each state, because the sentence syntax differs between the states (at least in german), so the response is much nicer.

Tony763 commented 2 years ago

Yes, that's usual. Dialogues in Czech are often different as direct 1:1 translation makes no sense.

pfefferle commented 2 years ago

@Tony763 what do you think? is this ready to merge or should the points you mentioned be part of this (first) pull request?

Tony763 commented 2 years ago

Merge my PR with Czech translation and this could be done. :) Added security can by added later, I would just write it to README.md as TODO:.

pfefferle commented 2 years ago

@krisgesling any feedback from your side?

krisgesling commented 2 years ago

what if there is no matching file? does it return the text param? I ask, because there are sensors, that return for example a string, that should not be translated (for example thermostats).

yeah self.translate('kangawallafox') should just return "kangawallafox"

In terms of checking it would be better if there was a response from HA that the action had been completed, but I'm not opposed to this being merged as is with the TODO item you've added. Better to have a chance of doing something than not :)

pfefferle commented 2 years ago

@krisgesling

yeah self.translate('kangawallafox') should just return "kangawallafox"

Thanks for the info :)

In terms of checking it would be better if there was a response from HA that the action had been completed, but I'm not opposed to this being merged as is with the TODO item you've added. Better to have a chance of doing something than not :)

I would vote for merging it as is and to work on the feedback in a different PR, because it might be not that trivial.

Not every cover reports the real state it is in. If I, for example, open my covers using Home Assistant, they simply change the state to open, if they receive the signal or not. So we have to be sure that the cover supports state reportings or not. Otherwise it can cause massive problems, at least if we handle it as a security feature.

Tony763 commented 2 years ago

@pfefferle Give me time till midnight for translations, before merge, please.

pfefferle commented 2 years ago

@Tony763 you have to tell @krisgesling and/or @stratus-ss ! It is not in my hands ☺️

Tony763 commented 2 years ago

Actually, I wrote comment to wrong PR, this one is already translated :slightly_smiling_face:

pfefferle commented 2 years ago

@stratus-ss done!

stratus-ss commented 2 years ago

LGTM, pending testing

pfefferle commented 2 years ago

@Tony763 can you help? what do I have to do to fix the Resource not accessible by integration issues?

Tony763 commented 2 years ago

Hi @pfefferle, sure, I have to go to work, now so I will check it tonight.

Tony763 commented 2 years ago

@pfefferle create empty branch gh-pages and enable github pages.

Tony763 commented 2 years ago

also change line 556 in __init__.py from self.speak_dialog('homeassistant.sensor.cover.%s' % sensor_state, data={ to self.speak_dialog('homeassistant.sensor.cover.{sensor_state}', data={ to fix pylint.

@pfefferle Faster than me :D I edited fixed line as there was whitespace before , and not after. With that, run in your fork will be ok, as all checks will pass. Tonight I will check why it is not working on PR as it should. If not, we can disable them on PRs and just check outcome on fork from where PR is.

pfefferle commented 2 years ago

;)

I fixed all lint errors in the cover and binary_sensor PRs. I will rework the GUI PR if/when the first two are merged.

pfefferle commented 2 years ago

@Tony763 on my "local" fork, it also works on branches. I think it breaks if you send PRs from remote branches. Maybe a security thingy.

stratus-ss commented 2 years ago

@krisgesling do you have any insight into the check that is failing?

Tony763 commented 2 years ago

Hi @stratus-ss, please check PR #87. I covered there whats going on and fixed it.

stratus-ss commented 2 years ago

I have a cover defined

cover.double_garage_door

I don't seem to get any output from the skill because the cover is dumb and does not report its' state

Here is an entity test

turn on steve_s_desk_fan                                                                                                          
 >> turned on Steve's Desk Fan.                                                                                                 
 turn off steve desk fan                                                                                                          
 >> Steve's Desk Fan is now off.

here is the cover test output

can you close double garage door                                                                                                                    
 can you close double garage door         
 close the double garage door                                                                                                               
 open the double garage door

The cover does indeed open and close, but there is no output from the skill

I am using a binary_sensor on the cover to determine its' open/close status since the cover itself does not report state

pfefferle commented 2 years ago

@stratus-ss thanks for testing!

You miss an output, like "{Entity} is going to open"?

The missing "real" state is a problem of the cover and/or Home Assistant (config). I mentioned it here: https://github.com/MycroftAI/skill-homeassistant/pull/80#issuecomment-941997401

Not every cover reports the real state it is in. If I, for example, open my covers using Home Assistant, they simply change the state to open, if they receive the signal or not. So we have to be sure that the cover supports state reportings or not. Otherwise it can cause massive problems, at least if we handle it as a security feature.

stratus-ss commented 2 years ago

@stratus-ss thanks for testing!

You miss an output, like "{Entity} is going to open"?

Yes, some sort of feedback is what I expect. If I wasn't watching my garage door, I wouldn't know if Mycroft heard me, had a problem, was waiting on a slow internet connection, had a bad TTS problem or otherwise.

Consistent interaction is important. As existing commands give some sort of feedback, so should this PR

The missing "real" state is a problem of the cover and/or Home Assistant (config). I mentioned it here: #80 (comment)

Yes, I understand, I was confirming this. I was adding context in case someone reads this in the future

pfefferle commented 2 years ago

Should I keep the output vague? Something like: "trying to open {Entity}?"

stratus-ss commented 2 years ago

Should I keep the output vague? Something like: "trying to open {Entity}?"

There is the ideal and then the practical.

The ideal is to attempt to check the status of the cover. If, like in my case, the status comes back with nothing, is empty or throws an error then the response would be general. If there is some sort of status then that would cause a different response.

Having said that, if you just want to push this through, the practical or MVP for this PR is just a vague response

pfefferle commented 2 years ago

I would start with the simple and vague solution, to keep the PR simple.

The problem is, that it always return a state and you never can get sure if it is really the state. At least I have not found a valid metric! So the only metric that I can think of is time. If the cover instantaneous changes the state it seems to be a "stateless" cover. Maybe this is something to report on the home assistant repo.