openhab-addons
openhab-addons copied to clipboard
openhab
[mqtt.generic] separate command parsing from cached value updating
fixes #12150
Previously, Value.update would parse the command, and update the cached value with that command. Which means that when sending a command towards MQTT (instead of processing an update from MQTT), the cached value was unintentionally updated. This prevented the REFRESH command from returning the most recent value received from the device.
Separating the two concerns also makes the test more obvious what they are testing, and vastly simplified a kludgy workaround that RollershutterValue was using to be able to process Commands that aren't States.
I've tested this in my local openhab installation to confirm that an MQTT channel with separate state and command topics, where the state topic does not change after sending a command, the item is set to autoupdate, I can now use REFRESH against the item and the prior state is restored.
Hi, I really like where this is going :-)
I just have one suggestion: Currently the same parseCommand is used for messages received from MQTT and for values to be published to MQTT. Can we separate them? Could we have 2 different parse functions, where one calls the other by default?
I remember, that we do have some problems with the percentage value. During command parsing it is not clear, if the devices or OHs value range should be used. If we would have separate functions it would be clear.
I totally agree on that point - I was super surprised when I realized incoming values from MQTT went through the same parsing function. Sure, I can add a separate name and default to current implementation relatively easily. I think any change of implementation for particular data types probably belong in their own PR.
@jochen314 : I've updated the PR with your suggestion. do you have any other feedback?
@lolodomo : can you look at this as well?
@ccutrer : any feedback on my comments ?
Note that there are now conflicts on few files.
@ccutrer : any feedback on my comments ?
Note that there are now conflicts on few files.
I think there's a good chance your comments need addressed, but I've been too busy to look at them. I'm hoping to address them in the next couple days. And I'll also address any merge conflicts.
@lolodomo: I finally got around to this. and no, there is no chance that type in those locations could be UNDEF (or NULL, or null). I've updated the prototypes to be Command instead of Type to ensure this as well. Those methods are called either from ChannelState.publishValue, in which case it's already a Command, or ChannelState.processMessage, which used TypeParser.parseCommand first, taking the declared types each Value allows, and returning if it's not one of those types.
@lolodomo : I'd really love to get this in. I'm doing a bunch of work on the mqtt.homeassistant binding that, while it doesn't strictly depend on this PR, will likely need to be updated after this is in.
I will prefer a second review by @openhab/add-ons-maintainers before merging this PR, I have some difficulties to evaluate the risks caused by these changes on all MQTT bindings in openHAB.
+1 additional eyes. It is a significant change. But I have been running it at my home for several months, and I use all of the MQTT sub-bindings (manually configured things/channels; homie; home assistant; ESP Milight Hub). I'm very confident that the only end-user visible change is that if you send a command to an MQTT channel, and the device does not update a corresponding state topic, and then you send REFRESH to the channel, the cached value will not have changed from the command. If you never send REFRESH to the channel, you'll never notice that the cached value wasn't changed. If you're concerned about this, I could see having an additional configuration option to say "assume the published values are accepted, and update internal cache". However, this seems contradictory to me with the auto update framework and state predictions. Maybe a better approach would be a way to say "for this MQTT device, make the AutoUpdatePolicy RECOMMEND rather than DEFAULT." If either of those get recommended, I'd rather implement them as a separate PR though - this one is already large enough.
rebased PR to resolve two minor merge conflicts in tests.
@antroids : any chance you'd be willing to look at this?
Would be nice if this can be merged for the next milestone so we can get some testing milage. I did some basic tests (not using all mqtt sub bindings though) and did not have any issue to report.
@antroids : any chance you'd be willing to look at this?
LGTM since it was tested and didn't broke something.
As for me, I would like to move State out of Value, rename Values to ValueHandlers or MqttTypes and make them immutable. These parse methods could be something like reducers: Command reduceIncoming(Outgoing)MqttMessage(State previousState, Command message). This could be another refactoring PR, for sure.
Are you are several to be confident with these changes, I will merge them. I have to take a look to additional changes applied after my last review.
I've tested this in my local openhab installation to confirm that an MQTT channel with separate state and command topics, where the state topic does not change after sending a command, the item is set to autoupdate, I can now use REFRESH against the item and the prior state is restored.
Trying to figure out all possible flows. What if we have Percentage (or any depending on the internal state) channel with the only command topic? I don't think that it will work properly after the change. Maybe we should consider command only channels as optimistic and update the value for them.
What if we have Percentage (or any depending on the internal state) channel with the only command topic? I don't think that it will work properly after the change. Maybe we should consider command only channels as optimistic and update the value for them.
Just before I push the merge button, I read that. Does it mean that I should not merge ?
He's referring to IncreaseDecreaseType essentially not working for a channel without a state topic, since we no longer keep track of the last command sent to it. I would argue that it never should have worked. What does it mean to ask a channel to increase if it doesn't keep track of its value? Alternately, we should enable (optionally) passing such commands directly through to MQTT, which is one of the follow up PRs I've been waiting for this one to be merged before working on.
What if we have Percentage (or any depending on the internal state) channel with the only command topic? I don't think that it will work properly after the change. Maybe we should consider command only channels as optimistic and update the value for them.
Just before I push the merge button, I read that. Does it mean that I should not merge ?
I think It can be merged. It rather addon design issue than error in this particular PR.