architecture icon indicating copy to clipboard operation
architecture copied to clipboard

Add toggle service to Covers

Open kbickar opened this issue 6 years ago • 25 comments

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 avatar Apr 20 '19 15:04 kbickar

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)

kbickar avatar Apr 20 '19 15:04 kbickar

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.

andrewsayre avatar Apr 20 '19 15:04 andrewsayre

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.

kbickar avatar Apr 20 '19 15:04 kbickar

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

Add cover.toggle_cover service

  • A default implementation uses is_closed to determine if it should call open vs close.

Add cover.toggle_cover_tilt service

  • A default implementation uses current_cover_tilt_position == 0 (meaning closed) to determine if it should open vs close. We don't need to introduce new properties. is_closed (and other related) only exist to drive the value of state, which we don't do for tilt position.

Other items:

andrewsayre avatar Apr 20 '19 16:04 andrewsayre

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

kbickar avatar Apr 20 '19 18:04 kbickar

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.

andrewsayre avatar Apr 20 '19 20:04 andrewsayre

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

balloob avatar Apr 22 '19 15:04 balloob

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

finipini avatar Jul 16 '20 20:07 finipini

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

PopRe avatar Jul 17 '20 01:07 PopRe

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.

finipini avatar Jul 17 '20 02:07 finipini

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.

kbickar avatar Jul 17 '20 04:07 kbickar

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

finipini avatar Jul 17 '20 09:07 finipini

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.

tetienne avatar Jul 27 '20 08:07 tetienne

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

mbaluda avatar Jan 08 '21 14:01 mbaluda

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

  • If stop is supported, implement toggle as open/stop/close/stop
  • If stop is not supported, implement toggle as open/close

emontnemery avatar Jan 27 '21 19:01 emontnemery

stop is supported

mbaluda avatar Jan 27 '21 19:01 mbaluda

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

emontnemery avatar Jan 27 '21 19:01 emontnemery

Ah now I understand

mbaluda avatar Jan 27 '21 19:01 mbaluda

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.

CubieMedia avatar Jan 29 '21 17:01 CubieMedia

@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?

emontnemery avatar Jan 29 '21 19:01 emontnemery

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 avatar Jan 29 '21 20:01 CubieMedia

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 avatar Jan 29 '21 22:01 CubieMedia

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).

CubieMedia avatar Jan 31 '21 10:01 CubieMedia

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.

emontnemery avatar Jan 31 '21 14:01 emontnemery

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

CubieMedia avatar Jan 31 '21 16:01 CubieMedia

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.

Taraman17 avatar Mar 01 '23 22:03 Taraman17

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

CubieMedia avatar Mar 02 '23 06:03 CubieMedia

Well, the actual implementation of the toggle method for the cover does not solved all the cases. See my previous comments. Having a CoverEntityFeature.TOGGLE feature 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.

tetienne avatar Mar 02 '23 09:03 tetienne

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 :-)

CubieMedia avatar Mar 02 '23 10:03 CubieMedia

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.

tetienne avatar Mar 02 '23 13:03 tetienne