search

openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.9k stars 3.6k forks source link

[mqtt] REFRESH command doesn't restore item to current value in MQTT #12150

Closed ccutrer closed 1 year ago

ccutrer commented 2 years ago

I have an MQTT channel with separate command and state topics. The Item is set to autoupdate=true (the default). The device at the other end may take a long time updating the state topic with the new value after receiving a command (or not update at all, if it's rejected!)

Expected Behavior

When the device rejects the command, I should be able to send a REFRESH to the item, and it will restore it to the current state in MQTT. But it does not.

Current Behavior

It just stays forever, making OpenHAB out-of-sync with the device, with no way to fix it besides restarting the MQTT binding (or maybe just the thing?)

Possible Solution

Make https://github.com/openhab/openhab-addons/blob/78e214651fa5ea715a65d8464df1108f1632290c/bundles/org.openhab.binding.mqtt.generic/src/main/java/org/openhab/binding/mqtt/generic/ChannelState.java#L334 only execute if you have a state topic and a command topic that aren't the same. If they're the same, that line makes sense to immediately update the cache, because the command you sent is the new state of the item in MQTT. But if a device has separate topics, it might ignore the command, and the state should be able to be restored.

Steps to Reproduce (for Bugs)

  1. Set up an MQTT channel with differing (completely unrelated!) command and state topics. Have the state topic start with a retained value of say "77"
  2. Link an item to the channel
  3. Send a command, such as "78" to the channel
  4. Note that in MQTT, the state topic still has a value of "77"
  5. Send a REFRESH to the item.
  6. It should now restore to 77, but it doesn't change.

Context

I have a device that's very slow to acknowledge commands, and occasionally drops them, but I still want to use autoupdate to make my UI easy to interact with. I have written a rule so that when a command is sent to the item, after 30s it will send a REFRESH so that OpenHAB will show the actual state of the world. I would expect it to work, but it's not.

Your Environment

ccutrer commented 2 years ago

Oh, one other piece of information. If I use the console and openhab:update the item to another value, then I send REFRESH, it does work to revert to the current state as what's in MQTT.

Rossko57 commented 2 years ago

It shouldn't work, really. REFRESH is not "restore last" REFRESH should cause the binding to re-fetch the up to date data from remote device or service, when possible. There's no standard way to do that in MQTT.

It's arguable that if the remote device has configured a retained message on this topic, the binding could fetch that from the broker. I don't know if that is possible without going through unsubscribe/resubscribe ?

If I use the console and openhab:update the item to another value, then I send REFRESH, it does work to revert to the current state as what's in MQTT.

I'd call that a bug, unless it is actually doing the 'retain' business.

ccutrer commented 2 years ago

I understand the intended meaning of REFRESH. And it seems the MQTT binding is saying "because I don't want to unsubscribe and resubscribe, I'm going to keep a local cache up to date of what's in MQTT, regardless of any linked items". and then it's providing that cache to the item when the item asks for a refresh. But the referenced line is making that cache out of date, and I would consider it a bug, since that cache is intended to represent the current state of MQTT if you were to do an unsubscribe and resubscribe.

I could see an argument that the proper way to fix it is to have REFRESH actually do an unsubscribe/resubscribe. 🤷 that wouldn't bother me, since the end user result would be the same. Either way, the bug is that OpenHAB is out of sync with the device, and a REFRESH should re-sync it, but it's not.

Rossko57 commented 2 years ago

Either way, the bug is that OpenHAB is out of sync with the device, and a REFRESH should re-sync it, but it's not.

But in the general case, that simply cannot be done in MQTT.

In the special case where the remote MQTT client has provided a retained message, it can be argued that is what REFRESH should provide (and a binding cache would be a good way to do that!) but a retained message is not necessarily anything to do with current remote state. MQTT allows you to set say a 'welcome' message as retained, and then publish current data non-retained. The broker will feed the 'welcome' to new subscribers.

If you want the binding to always provide the last valid message (which might be different from any retained message) then that's a feature you might reasonably ask for, as the best guess.

Can you give a use case, why would you want to trample on your data and then expect the binding to provide a roll-back?

I mean, I see your very specific reluctant device example, where you want autoupdate's cake and eat it. But I don't see the binding as your solution. I'd write a rule triggered on command to start a timer. If MQTT state update comes, cancel timer. Otherwise when times out, set UNDEF or whatever (there is no real current state as you know its all gone wrong somewhere)

ccutrer commented 2 years ago

Publishing a non-retained message over a retained message, and then a new subscription getting the original retained message is a very good point.

Let's take a step back then. Take that MQTT (in general) can't provide the current state of something (even though in practice almost every device will use retained for all messages, or not retained for all messages)... why does the MQTT binding even cache the value, or support the REFRESH command at all? If it's just "so that the thing can provide the value immediately when an item is linked to it", then why is sending a command to MQTT updating the cache at all? If the command and state topics are the same, OH will immediately receive back the value it just published, so it can update its cache. If they're not the same, the cache is suddenly being trampled on by a write to one topic when it should be representing the state of another topic that may or may not have received any messages. That sounds buggy to me, regardless of if messages are sent retained or not. For my use case, it's just a happy coincidence that REFRESH is being implemented, and that's what's revealing this bug in the cache.

Can you give a use case, why would you want to trample on your data and then expect the binding to provide a roll-back?

My use case is already in the ticket, but I'd be happy to elaborate more. I have a device (a furnace/thermostat) that uses the Homie convention. You publish commands to a "/set" topic. The device may or may not respect that command, or simply take a long time to apply (it's a very old industrial device with flakey communication). If/when it does process the command, it will send an update on the main state topic. From OpenHAB, originally I set autoupdate="false" on the Item, which gives the behavior that if the command doesn't ever get processed, the item won't update to the new value. But this is less than ideal from a UX perspective - if I want to change the temperature 2 degrees, I have to press UP once (in BasicUI), wait 5-20s until the update processes, then press UP again. By setting autoupdate="true" (which is generally recommended anyway), I can quickly press UP twice, without waiting for the first message to be "acked". For the most part commands are processed, it just takes a while to reflect the state. But occasionally they're not. So I added a rule that 30s after a (non-REFRESH) command is sent to the item, I send a REFRESH, hoping to get it to reflect what MQTT shows.

Overall I'm just wondering if you're open to me sending a PR to remove that update-cache-on-command line (either unconditionally, or only in certain conditions - like if the message received was retained, and state and command topics differ, etc.).

Rossko57 commented 2 years ago

why does the MQTT binding even cache the value

No idea. Maybe caching the whole message is part of sorting out duplicate messages that can be expected in MQTT.

support the REFRESH command at all?

I didn't think it did in the general case, and don't believe it should (or rather, accept it but do nothing - like many bindings). On the other hand, there are system start-up cases - channels effectively REFRESH as links are called into being, I guess. We may have already got a message from the broker before the Item is available to update.

It did prove a nuisance in earlier versions by trampling over Items restored by persistence at system startup with the "empty" cache. Background https://github.com/openhab/openhab-addons/issues/5879

wondering if you're open to me sending a PR

By all means - you don't need my approval and I don't get a vote :) Take care not to re-invent previous "startup" type issues.

ccutrer commented 2 years ago

why does the MQTT binding even cache the value

after looking through the code more, it's using the cachedValue.update(command) as its way to process incoming commands of several Command types into the proper type for that sort of Value. storing the result of that in the cached value is just kind of a side effect. the code also already has a kludgy work-around to avoid this unwanted side effect in some cases (i.e. a STOP command for a rollershutter has no idea what to set the cached state to).

Rossko57 commented 2 years ago

Side comment on expected REFRESH action. MQTT channels may be configured to process messages received on stateTopic into OH Item state updates (the usual case), or exceptionally into OH Item commands, or into system bus trigger events (unrelated to any Item).

I would say REFRESH should only re-cycle the state update action. (If anything at all - as already said I personally am not convinced it should (a) cache and re-enact transient messages or (b) re-fetch retained messages that do not reflect most recent message anyway)