Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[daikin] Update channels immediately after a successful API command #17149

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Jul 25, 2024

This was requested in #16479 (comment)

In summary, when sending an API call to change something, the device will return a ret=OK if it accepted the values that we've set. Therefore, we should know the updated value already, without having to wait for the next poll. We therefore update the relevant channel immediately.

This helps for when changing something requires immediate feedback, such as increasing/decreasing the set temperature. Previously, the only way to get a smooth user experience is by relying on autoupdate.

@jimtng
Copy link
Contributor Author

jimtng commented Jul 25, 2024

@Jogobo - FYI

@jimtng jimtng force-pushed the daikin-channel-update branch 2 times, most recently from 30b7e6d to 70f1adb Compare July 26, 2024 09:50
@jimtng jimtng force-pushed the daikin-channel-update branch from 70f1adb to 0038bf3 Compare July 26, 2024 10:03
@jimtng
Copy link
Contributor Author

jimtng commented Jul 26, 2024

@psmedley if you're still using Daikin binding, WDYT?

@psmedley
Copy link
Contributor

LGTM - I would hope this might fix issues where if you send two commands in short succession - the second command can end up reverting the 1st command - presumably as the autorefresh sometimes hasn't completed.

@jimtng
Copy link
Contributor Author

jimtng commented Jul 26, 2024

I would hope this might fix issues where if you send two commands in short succession - the second command can end up reverting the 1st command - presumably as the autorefresh sometimes hasn't completed.

Could you elaborate / give an example please?

@Jogobo
Copy link

Jogobo commented Jul 26, 2024

As, in most cases, you cannot set a single value via HTTP set command I guess it depends on the parameter combinations chosen in the binding.
Let's say i.e. you cannot set fan speed without setting the temperature too, but you can set the temperature without setting the fan speed. Actual desired temperature is 20°C.

First you set the new desired temperature with your temp item to 21°C which results in set_control_info?stemp=21 being sent by the binding. As long as the next poll did not happen the binding channel is still at 20°C.
Now you change the fan speed with your fan item. As the temperature has to be sent together with the new fan speed the binding sends set_control_info?stemp=20&f_rate=4, thus overwriting the previously sent 21°C.

@jimtng
Copy link
Contributor Author

jimtng commented Jul 26, 2024

Let's say i.e. you cannot set fan speed without setting the temperature too, but you can set the temperature without setting the fan speed. Actual desired temperature is 20°C.

First you set the new desired temperature with your temp item to 21°C which results in set_control_info?stemp=21 being sent by the binding. As long as the next poll did not happen the binding channel is still at 20°C. Now you change the fan speed with your fan item. As the temperature has to be sent together with the new fan speed the binding sends set_control_info?stemp=20&f_rate=4, thus overwriting the previously sent 21°C.

No, I believe this is what happens instead:

  • Setting temp to 21C -> get_control_info -> set_control_info?stemp=21 + other values from get_control_info
  • Setting fan speed to 4 -> get_control_info -> (the stemp should be 21 already here) change the f_rate=4 -> send it back via set_control_info - even though the settemp channel may still be 20 if a poll hasn't occurred yet.

For each action for the channel, it first gets the current info so nothing is "stale" / old.

And that's all already done prior to this PR.

@psmedley
Copy link
Contributor

My example is similar to @Jogobo

I have a rule setup so that if the set point or inside temp change, and the delta between them is >= 2C - then increase the fan speed.

I have other rules to set the setpoint to 16C overnight - but up to 23C during the day. When the setpoint goes from 16C to 23C is also triggers the fan speed rule - and on a decent number of occasions, the setpoint ends up reverting to the 'old' setpoint of 16C.

This was with the code in 4.2.0

@jimtng
Copy link
Contributor Author

jimtng commented Jul 27, 2024

I have a rule setup so that if the set point or inside temp change, and the delta between them is >= 2C - then increase the fan speed.

I have other rules to set the setpoint to 16C overnight - but up to 23C during the day. When the setpoint goes from 16C to 23C is also triggers the fan speed rule - and on a decent number of occasions, the setpoint ends up reverting to the 'old' setpoint of 16C.

This was with the code in 4.2.0

I can't immediately imagine how this could have happened. Could you make this problem appear consistently, perhaps by lowering the polling refresh rate to say 10 minutes and triggering the sequence of events manually?

@jimtng jimtng marked this pull request as ready for review July 29, 2024 06:58
@jimtng jimtng requested a review from caffineehacker as a code owner July 29, 2024 06:58
@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Aug 1, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just one comment.

Signed-off-by: Jimmy Tanagra <[email protected]>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks,LGTM.

Will merge when the potential issue @Jogobo and @psmedley mention is resolved.

@jimtng
Copy link
Contributor Author

jimtng commented Aug 28, 2024

ping @Jogobo @psmedley

@psmedley
Copy link
Contributor

Apologies, I've been busy. I just did some experimentation with trying to reproduce - I can't - even with refresh set to 600 seconds.

@Jogobo
Copy link

Jogobo commented Aug 28, 2024

Did not come across the problems @psmedley had. So from my side there is no reason to prevent the changes from being merged.

@lsiepel lsiepel merged commit 198b9b1 into openhab:main Aug 28, 2024
5 checks passed
@jimtng jimtng deleted the daikin-channel-update branch August 28, 2024 10:11
digitaldan pushed a commit to digitaldan/openhab-addons that referenced this pull request Aug 29, 2024
…penhab#17149)

* [daikin] Update channels immediately after a successful API command

Signed-off-by: Jimmy Tanagra <[email protected]>
@jimtng jimtng added this to the 4.3 milestone Sep 25, 2024
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
…penhab#17149)

* [daikin] Update channels immediately after a successful API command

Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Patrik Gfeller <[email protected]>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
…penhab#17149)

* [daikin] Update channels immediately after a successful API command

Signed-off-by: Jimmy Tanagra <[email protected]>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
…penhab#17149)

* [daikin] Update channels immediately after a successful API command

Signed-off-by: Jimmy Tanagra <[email protected]>
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
…penhab#17149)

* [daikin] Update channels immediately after a successful API command

Signed-off-by: Jimmy Tanagra <[email protected]>
Signed-off-by: Ciprian Pascu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants