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

[mqtt] Treat incoming empty string as NULL for most types #16307

Merged
merged 2 commits into from Jan 29, 2024

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Jan 19, 2024

Empty strings are often received when deleting retained topics when a device goes offline, or as the result of a transformation that is missing a value (such as a "scene" event from zwave-js-ui, which sends JSON with a timestamp and the scene value, then immediately sends a value to the topic with only a timestamp).

For string channels, add a configuration value to allow setting a specific string for treating as NULL, since empty string can make sense for that type.

Empty strings are often received when deleting retained topics when a device
goes offline, or as the result of a transformation that is missing
a value (such as a "scene" event from zwave-js-ui, which sends JSON with
a timestamp and the scene value, then immediately sends a value to the topic
with only a timestamp).

For string channels, add a configuration value to allow setting a specific
string for treating as UNDEF, since empty string can make sense for that
type.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks for this enhancement.
Just a question: Are you sure that UNDEF is the right state here and not maybe NULL?
UNDEF is meant for ambiguous situations, when there actually IS a real state out there, we just do not know how to represent it in the item. NULL means that there isn't any state at all.

@ccutrer
Copy link
Contributor Author

ccutrer commented Jan 27, 2024

No I'm not sure at all. I'm never quite clear on which of whose is which. The intended purpose here is "we know that the item does not currently have a state" not "we don't know what the state is." I thought UNDEF was what I wanted, since NULL is what items start out as when first created or on boot, before persistence has restored them (and thus we don't know what their state is, or if we even have a state).

@kaikreuzer
Copy link
Member

Well, the question is how we want treat the incoming value. If we say that the current state of the item is probably right and we simply want to ignore the incoming empty string, then UNDEF is probably indeed the right option. If the empty string means that a topic does not exist anymore and there is no representation "in the outside world" anymore, NULL would be better.
If the first case is what we deal with here, all is fine.

@ccutrer
Copy link
Contributor Author

ccutrer commented Jan 27, 2024

Okay, I think we want use NULL then. UNDEF is used for NaN, which does match your description, and this is for either "the device is gone, so no longer has a state" or "there is no state." Either of which is a definitive representation.

@apella12
Copy link
Contributor

apella12 commented Jan 28, 2024

Background: I have been running Zwave-JS-UI (zwave2mqtt) using generic MQTT things because of issues with the HA configs (i.e #15145). As noted above, HA scenes configs have momentary values, but revert to empty. I addressed (no WARN messages) with the empty Scene topic problem in the generic MQTT by using a string channel, only getting the "value" and using MAP (=OFF).

I recently set up a test environment mirroring one production instance using the HA configs. The HA Scene configs are pulled into OH as a number channel; mqtt:homeassistant_zwavejs2mqtt_5F0xf1c43721_5Fnode8:6035d9536c:zwavejs2mqtt_5F0xf1c43721_5Fnode8:zwavejs2mqtt_5F0xf1c43721_5F8_2D91_2D0_2Dscene_2D006#sensor (Number)
The channels are;
{"state_topic":"zwave1/nodeID_8/central_scene/endpoint_0/scene/007","value_template":"{{ value_json.value | default('') }}","state_class":"measurement","availability":[{"payload_available":"true","payload_not_available":"false","topic":"zwave1/nodeID_8/status","value_template":"{{'true' if value_json.value else 'false'}}"},{"topic":"zwave1/_CLIENTS/ZWAVE_GATEWAY-zwave-js-ui/status","value_template":"{{'online' if value_json.value else 'offline'}}"},{"payload_available":"true","payload_not_available":"false","topic":"zwave1/driver/status"}],"availability_mode":"all","json_attributes_topic":"zwave1/nodeID_8/central_scene/endpoint_0/scene/007","device":{"identifiers":["zwavejs2mqtt_0xf1c43721_node8"],"manufacturer":"Fibargroup","model":"Keyfob (FGKF601)","name":"nodeID_8","sw_version":"3.2"},"name":"nodeID_8_scene_state_scene_007","unique_id":"zwavejs2mqtt_0xf1c43721_8-91-0-scene-007"}

And the WARN sequence (just 1 example). I have 26 scene channels (not all used).

2024-01-28 13:48:31.350 [DEBUG] [.internal.JinjaTransformationService] - about to transform '{"time":1706458126275}' by the function '{{ value_json.value | default('') }}'
2024-01-28 13:48:31.351 [DEBUG] [.internal.JinjaTransformationService] - transformation resulted in ''
2024-01-28 13:48:31.352 [WARN ] [ab.binding.mqtt.generic.ChannelState] - Command '' from channel 'mqtt:homeassistant_zwavejs2mqtt_5F0xf1c43721_5Fnode59:6035d9536c:zwavejs2mqtt_5F0xf1c43721_5Fnode59:zwavejs2mqtt_5F0xf1c43721_5F59_2D91_2D0_2Dscene_2D001#sensor' not supported by type 'NumberValue': null

Anyway I was going to create an issue, but saw this PR and wanted to ask if this code was going to fix the Scene problem? If not, is there a way to change the HA Scene config from Number to String in OH? Despite all the complexity, all the Scenes contain "91" as that is the Zwave command class enum for central_scene.

@ccutrer
Copy link
Contributor Author

ccutrer commented Jan 29, 2024

@apella12 : yes, this will fix that error in the logs. And is one of several reasons I wrote this PR. I also use zwave-js-ui.

since these are for definitive "this channel no longer has a value",
not "this channel has a value we simply can't represent"

Signed-off-by: Cody Cutrer <cody@cutrer.us>
@ccutrer ccutrer changed the title [mqtt] Treat incoming empty string as UNDEF for most types [mqtt] Treat incoming empty string as NULL for most types Jan 29, 2024
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@kaikreuzer kaikreuzer merged commit 5da9dda into openhab:main Jan 29, 2024
3 checks passed
@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature for an existing add-on label Jan 29, 2024
@ccutrer ccutrer deleted the mqtt-empty-string-undef branch January 29, 2024 22:05
@kaikreuzer kaikreuzer added this to the 4.2 milestone Jan 29, 2024
ErikDB87 pushed a commit to ErikDB87/openhab-addons-openweathermap that referenced this pull request Jan 31, 2024
)

* [mqtt] Treat incoming empty string as NULL for most types

Empty strings are often received when deleting retained topics when a device
goes offline, or as the result of a transformation that is missing
a value (such as a "scene" event from zwave-js-ui, which sends JSON with
a timestamp and the scene value, then immediately sends a value to the topic
with only a timestamp).

For string channels, add a configuration value to allow setting a specific
string for treating as NULL, since empty string can make sense for that
type.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Erik De Boeck <deboeck.erik@gmail.com>
@stedon81
Copy link

stedon81 commented Feb 3, 2024

Hi ErikDB87,
thanks very much for this achievement! Exactly I was looking and hoping for!
However, actually your change in the mqtt binding violates the way how the Homie Convention says to interprete the values. (There they explicitly say that an empty value is forbidden: "An empty string ("") is not a valid payload").
Therefore I asked the community around the Homie Convention to come up with a specification for those cases (homieiot/convention#284). I would strongly hope that they realize this as simple as your idea is.

Cheers,
Stefan D.

@ccutrer
Copy link
Contributor Author

ccutrer commented Feb 3, 2024

Heh, yes, I was very aware the Homie convention forbids empty payloads (presumably because that's how you "delete" retained topics in MQTT). But given their language, one should never encounter an empty payload, and thus doesn't specify how one should handle it if it does happen - it's simply error handling. For example, it does not say "a client should retain the prior value when an empty string is received."

I was not aware of that discussion, or even the existence of an upcoming Homie 5, so thank you for that link. I stopped looking at the spec when it had not changed for years, except for some minor changes that they didn't actually change the version number for.

I do agree with you on the alerts topic though- the spec is so vague as to the contents of its payloads that it's essentially useless.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/mqtt-discussion/154139/13

austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
)

* [mqtt] Treat incoming empty string as NULL for most types

Empty strings are often received when deleting retained topics when a device
goes offline, or as the result of a transformation that is missing
a value (such as a "scene" event from zwave-js-ui, which sends JSON with
a timestamp and the scene value, then immediately sends a value to the topic
with only a timestamp).

For string channels, add a configuration value to allow setting a specific
string for treating as NULL, since empty string can make sense for that
type.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/ignoring-empty-mqtt-payload/154554/9

ccutrer added a commit to ccutrer/openhab-addons that referenced this pull request Apr 11, 2024
Since openhab#16307, the state could get set to NULL, not just UNDEF, and
once it got in that state PercentageValue would throw an error every
time it received any other message or command, effectively rendering
the channel broken until openHAB restarted.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
ccutrer added a commit to ccutrer/openhab-addons that referenced this pull request Apr 11, 2024
Since openhab#16307, the state could get set to NULL, not just UNDEF, and
once it got in that state PercentageValue would throw an error every
time it received any other message or command, effectively rendering
the channel broken until openHAB restarted.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
ccutrer added a commit to ccutrer/openhab-addons that referenced this pull request Apr 11, 2024
Since openhab#16307, everything except TextValue would treat an empty
string as NULL. TextValue was excluded because empty string
could be a valid value. Instead it had a config option added
to match for a NULL value if you wanted. BUT, if a TextValue
has specific valid states configured (such as from Homie or
Home Assistant bindings), then empty string is no longer a
valid value, and should receive the same treatment of setting
the state to NULL when encountered.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
ccutrer added a commit to ccutrer/openhab-addons that referenced this pull request Apr 11, 2024
Since openhab#16307, everything except TextValue would treat an empty
string as NULL. TextValue was excluded because empty string
could be a valid value. Instead it had a config option added
to match for a NULL value if you wanted. BUT, if a TextValue
has specific valid states configured (such as from Homie or
Home Assistant bindings), then empty string is no longer a
valid value, and should receive the same treatment of setting
the state to NULL when encountered.

Signed-off-by: Cody Cutrer <cody@cutrer.us>
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.

None yet

5 participants