rstrouse / ESPSomfy-RTS

A controller for Somfy RTS shades and blinds
The Unlicense
442 stars 32 forks source link

The `PUT /tiltCommand` endpoint isn't idempotent #239

Closed joneshf closed 5 months ago

joneshf commented 6 months ago

I've noticed that the PUT /tiltCommand endpoint isn't idempotent. In less jargon-y terms, it seems that if this endpoint receives the same target value twice, it will send the right command to the shades the first time, but actually send the "my" command the second time. That "my" command does not necessarily have the same behavior on the shades as the target value had. In my case, it means that using the PUT /tiltCommand endpoint twice, will first close the blinds, then open them. I have tilt-only blinds, so maybe that's part of the issue. But this is what I'm doing to reproduce this behavior:

  1. $ curl --request PUT http://<esp-somfy-host>/tiltCommand?shadeId=1&target=0

    The blinds tilt to 0%.

  2. $ curl --request PUT http://<esp-somfy-host>/tiltCommand?shadeId=1&target=0

    The blinds tilt to the "my" position (which in this case happens to be 50%).

For more transparency, I'm not actually using the PUT /tiltCommand endpoint day-to-day. I'm using the set_tilt_position service in the Home Assistant integration. And near as I can tell, it ends up calling the PUT /tiltCommand endpoint. In any case, the behavior is reproducible directly through the REST API, so I figured I'd open the issue here.

I can think of at least two ways to work around this particular issue, so I'm not really stuck:

Even though I have workarounds, I think the fact that PUT /tiltCommand isn't idempotent is surprising both from the theoretical perspective that PUT endpoints should be idempotent (and this one isn't); and also surprising from the practical perspective that if my blinds are closed, and I tell them to close again, they will open instead. So I figured I'd open this issue in case you weren't aware of it, and had thoughts on how to fix it in ESPSomfy-RTS. If this is intentional behavior, that's fine too (things don't have to follow what I think in my head). But it might be worth noting that down somewhere, to save future people some debugging time.

rstrouse commented 6 months ago

The real issue here seems to be that sending a target when the target is already the position is the problem. It should not command the movement in this case and I see where the moveToTiltTarget method is not properly performing this check when it is integrated tilt. For tilt motors the code is correct but what it is doing now is initiating a my when it should be checking to make sure it is not idle before seeking.

rstrouse commented 6 months ago

Are your blinds tilt only?

joneshf commented 6 months ago

That's correct, they are tilt-only blinds.

rstrouse commented 6 months ago

v2.3.1 is in pre-release and it should resolve the issue of re-entrant targets on the endpoint when there is not a separate tilt and lift motor. Essentially, it needed to check if the target = the current position then do nothing.

rstrouse commented 5 months ago

v2.3.1 has been released and the tiltCommand now verifies the current position before acting.

joneshf commented 5 months ago

Had a chance to update to 2.3.1 and tried this out. I can confirm that this now works as expected for the PUT /tiltCommand endpoint, and more importantly for the set_tilt_position service.

Thanks for fixing this!