home-assistant / architecture

Repo to discuss Home Assistant architecture
317 stars 100 forks source link

Add toggle service to Covers #205

Closed kbickar closed 1 year ago

kbickar commented 5 years ago

The cover component currently does not support the 'toggle' service because it is not a descendant of ToggleEntity. ToggleEntitys have an on or off state while covers have open or close.

As covers do have an open_cover and close_cover service (as well as similar open_cover_tilt/close_cover_tilt), a toggle service and toggle_cover_tilt service would be appropriate.

Having a single button toggle is also very common for covers such as garage doors.

There is currently an issue requesting this and a PR to address it.

kbickar commented 5 years ago

For the toggle service, the base cover component can toggle using the is_closed property

For the toggle_tilt service, a new property is_tilt_closed would have to be added OR the toggle function would need to be overridden by some platforms. All platforms except the MQTT platform define a tilt of 0 as closed, but the MQTT platform allows a configurable closed value (e.g. closed could be 50 on a scale of 0-100)

andrewsayre commented 5 years ago

Thanks for opening the issue! Can you clarify your use case(s) -- for example, are you looking to solve this problem for a device that doesn't support open and close (the garage door button example) or are you looking to add toggle support for existing devices that open and close? This distinction is important because I don't think we support covers today that only toggle (and store state optimistically). More changes would needed to be considered in that case (like introducing a new SUPPORT_COVER_TOGGLE constant) than if we are just adding a cover_toggle service that is part of SUPPORT_COVER_OPEN and SUPPORT_COVER_CLOSED.

kbickar commented 5 years ago

The main use case would be adding toggle support for devices that open and close. Anything that has SUPPORT_COVER_OPEN and SUPPORT_COVER_CLOSED would be able to toggle.

Side note: I know some people have been using devices like garage doors that open and close using the same command and currently use a work around of calling the same toggle script in both open and close in template covers.

andrewsayre commented 5 years ago

Cool. So I think your proposal falls down into two major parts:

Add cover.toggle_cover service

Add cover.toggle_cover_tilt service

Other items:

kbickar commented 5 years ago

Any reason to name it toggle_cover rather than use the existing "toggle" service?

andrewsayre commented 5 years ago

toggler_cover would be consistent with the existing naming convention of services within the cover entity. I believe the existing toggle service is in relation turn_on and turn_off. I'm open to other's opinions.

balloob commented 5 years ago

When using the name "toggle", the generic service homeassistant.toggle will pick it up too.

finipini commented 4 years ago

Hello. I think that the cover.toggle doesnt work very well. In my mind this service should never be directely comparated with a light.toggle or switch.toggle, it is a total diferent thing.

And it shouldn,t also call a service depending of the current posicion of the cover (because if it is not totally closed it,s always open) because the actual service cover.toggle is almost always the same has calling the service cover.close.

In my opinion the cover.toggle should "cover" 3 diferent services, by this position: cover.open; cover.stop ; cover.close; cover.stop; cover.open...... and so on. It should remember Last call.service of the cover to know what is the next step it should take in the cover.toggle

PopRe commented 4 years ago

But lights can also be different brightnesses and be toggled, this would work the same way.

finipini commented 4 years ago

But lights can also be different brightnesses and be toggled, this would work the same way.

Not same think. We should see the real life porpose of that 2 very diferent thinks.But if you want to compare..... Light.toggle acts Just in state of Lights, not in states of their attributtes, so if you have a light on ( no matter whatt bri.and color) the toggle service Turn it off; if you have a light off the toggle service takes that light to on.

Lets see what happens with cover: if you have the state of the cover to close ( again the toggle service dont care about attributtes) and you press cover.toggle the cover starts to open, and then press cover.toggle again and cover starts to close, press again cover.toggle and cover stops for an instant and then continue to close.

Until cover completly closes again you can press has many times you want the cover.toggle, that it always go closing.

For me this makes no sense, i understand why it happens but shouldnt act like that. Should be ; closing;stop;opening;stop;closing;stop;opening..... etc....

Just like we can do with tasmota, if we want to control a shutter with Just one button. Even original firmware from shelly2.5 allows us to do that.

If this doesnt makes sense, maybe should be created a new service like cover.step_by_step.

kbickar commented 4 years ago

I think I see what you're saying - it's a bit different from a light in that it can be in a transition state. I guess it should take into account the state it's heading to, so if it's opening then toggle would close it and if it's closing, toggle would open it.

finipini commented 4 years ago

Yes that is a way to do it, i think, to me that should also include the stop action - close, stop, open.

tetienne commented 4 years ago

Hi, let me join also this interesting conversation. We have currently an issue on our custom component where an end-user has a garage door that only exposes a toggle command (no open, no close dedicated command): https://github.com/iMicknl/ha-tahoma/issues/146. The SUPPORT_COVER_TOGGLE suggested by @andrewsayre will totally make sense in our case.

mbaluda commented 3 years ago

Usually toggle is implemented as a 4 states loop: open stop close stop, this requires to store the last direction

emontnemery commented 3 years ago

So to summarize, toggle for covers should depend on what's supported by the entity:

mbaluda commented 3 years ago

stop is supported

emontnemery commented 3 years ago

supporting stop is not mandatory, so for platforms that don't support it, toggle needs to be implemented as open/close.

mbaluda commented 3 years ago

Ah now I understand

CubieMedia commented 3 years ago

My Python skills are not on the homeassistant team lvl. I am having problems to get the inheritance (mixins) working within CoverEntity. Any help would be appriciated ...

Anyways i am on it to make a new suggestion just need to understand more of the structure.

emontnemery commented 3 years ago

@CubieMedia I started working on it in parallel too, do you want to open a PR with what you have so far, or should I open one?

CubieMedia commented 3 years ago

Since i'm still struggling with the starting point (getting stuff into CoverEntity) i will be glad to join you. I will cancle my PR and would join yours if you share it ... we can discuss stuff there if you want to.

CubieMedia commented 3 years ago

PR is closed!

@emontnemery Please share your PR when you opened it. Or link the Branch then it is documented for the others what is happening.

CubieMedia commented 3 years ago

I got something working now!

@emontnemery https://github.com/CubieMedia/core/tree/CubieMedia-MQTT-Toggle-Cover

But im still not satisfied with the use of async and sync methodes ... there is redundant code and i think for me the concept is not really clear (open_cover is still not implemented).

emontnemery commented 3 years ago

I think it looks pretty good, and it's very similar to what I started out on, please go ahead and open a PR.

However, I don't think _is_last_toggle_direction_open should be set when async_open_cover is set because things will get weird if the cover is controlled by something else than HA. Instead, set it when the state is updated (for example by overriding async_write_ha_state for CoverEntity).

Also, maybe you want to initialize the _is_last_toggle_direction_open to None since we don't know it?

For the async vs sync, maybe do a helper method which can return the correct function to call based on a map of functions passed to it, then the same code can be reused also for toggle_tilt.

CubieMedia commented 3 years ago

Thanks to your advice it looks really clean.

I also got more confident and changed some methods definition in MqttCover, the redundant code is also solved with this.

My first tests were successfull and i will create a new PR

Taraman17 commented 1 year ago

What is the state of this Issue? I'm trying to implement a Garage Door Impulse Switch, that reports open/closed/intermediate as well as the last state (opening/closing). So the state of _is_last_toggle_direction_open would always be known. So additional to the toggle function I should also be able to enable/disable the up/down/stop controls to make it feel like a full featured control.

CubieMedia commented 1 year ago

This issue has been closed and the mentioned issue has a successfull merge. This feature is in the main branch of HA and ready to use.

It seems you missed something, the toggle function is the open/close (open/stop/close/stop) function.

Since this is all explained here (and connected issues) you could reread what we tried to achieve here. Summerized

tetienne commented 1 year ago

Well, the actual implementation of the toggle method for the cover does not solved all the cases. See my previous comments. Having a CoverEntityFeature.TOGGLEfeature flag would greatly help.

By default, this flag would be True if CoverEntityFeature.OPEN and CoverEntityFeature.CLOSE. And in the case I just gave, the device can set to True also, even if there is no open or close method available.

CubieMedia commented 1 year ago

Thank you for clarifying this. I get your Point now and i dont know your product. I also have my garage doors included into HA (with a Shelly 1). And even if you can see a garage door as a cover with this functionality it is not a cover.

This is a simple button which is only enabled for a short time and with each activation it toggles through one of the steps (open/stop/close/stop). This means your toggle function is inside your garage door and is used by external button which is your smart product.

What you are suggesting is to create a button function inside the cover field. With what i currently understand, this does not make sense to me.

Maybe we should have a direct talk or you draw me a picture :-)

tetienne commented 1 year ago

This means your toggle function is inside your garage door and is used by external button which is your smart product.

This is almost this indeed. But you make me think about the button integration. At the time, it was not yet a thing. So for my use case, we can just map the toggle (named cycle by Somfy) function of this garage door to a button within HA.

CubieMedia commented 1 year ago

I think so, yes. I will explain my setup, maybe it can help you.

Using Shelly 1 (Relay Actor is parallel to the button, Sensor is information if garage is closed or opened). In HA i have a button in Lovelace to trigger an action that closes the relay and opens it again after one second. This way it is the same as if i push the button in my garage.

With the sensor i can see the status of my garage.

With the configuration i mean something like this: https://github.com/bieniu/ha-shellies-discovery

Here all different Shelly Devices are configured (end of script). Hope this helps to get a good start.

A garage door should only be a cover if you control the motor itself.

Taraman17 commented 1 year ago

I was hoping I could use the up/stop/down controls of the cover for my garage. Since I can definitely tell from the information I get if the last movement was open or close, it should be possible to deactivate the up or down controls as needed. But maybe I'll try to build this into a custom control then...

frenck commented 1 year ago

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck