-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introduces standardized key for govee binary moisture sensors #2625
Conversation
I like the logic. The key name of "wet" seems somewhat short (almost like an acronym). Is there any grouping prefix part we could add to generalize this to other similar sensors? Something like state, detect, trigger, report, signal, … As in |
a9bc818
to
403abe9
Compare
Sure! Scanning through the existing device mappings in From
The Home Assistant UI conveys the state of binary moisture sensors as "wet" or "dry" so I do think "wet" should be part of the key. Of the prefixes you suggested, I like |
For the sake of thoroughness, here is some sample output from MQTT Explorer after the latest changes: Button Press (
|
I think Wet is the human-facing description and the name of the sensor is "moisture" with values ON/OFF or whatever the standard rtl_433 yes/no values are. We need to be careful about defining semantics for moisture sensors in general vs what I see as a slightly odd case. I think you are doing that. However, rtl_433 tends to be "just convert the on-air bits to json", and this is heading down the path of representing semantics. Obviously we need -- someplace, really not clear where -- some kind of stateful processing. This is clearly before. But, as a way to detangle the confused representation into "this report says wet" and "this report says not wet", this seems like progress. We also need to ask: does every message contain a wet/not-wet message, or are they only sometimes present? If sometimes, is it the case that sometimes this field will be present, and sometimes not? I think it's important not to report moisure:false when the over-the-air bits did not claim that. |
Good point about moisture being something with units, but I see that as being in a |
I don't have that much experience with Home Assistant or this codebase so I'm happy to defer to whatever key name / prefix is preferred by the maintainers. As I understand it, the goal is for the new key to be general enough that the decoders for any well-behaved binary moisture sensors can directly convert their raw wet/dry signals to it. I originally chose "wet" because it seemed like the most concise way to describe the state of an arbitrary binary moisture sensor (a sensor that detects the presence of moisture is either "wet" or "dry"). However, this naming convention doesn't apply cleanly to other binary sensors (e.g. there is no obvious adjective to describe when a sound sensor has heard a sound or a vibration sensor has felt a vibration etc) so I see the benefit of introducing a standard prefix like "detect" or "report" for all binary sensors so that they can all have a common naming scheme (i.e. "detect_<noun the binary sensor detects>").
I definitely understand the desire to keep higher-level semantics out of the rtl_433 codebase. However, I think the fact that the other PR transformed into a support ticket is evidence that the current decoder implementation makes these sensors difficult enough to integrate with HA today that they cause frustration and waste folks' time so some change is warranted to make them work more seamlessly out-of-the-box. As discussed in the other PR, these sensors are quirky but in the end they are best represented as binary moisture sensors in HA. Given that automatic device discovery in
In addition to wet/not-wet, this sensor sends occasional battery health messages. These messages are converted into json without the new
|
I'm good with this addition, also as template for the general case. It's a sensor to detect conditions. It |
I like I don't like |
My sentiment exactly, we can only decide this once and then users will depend on it staying that way. Thanks for the feedback on |
One question regarding Would it make sense to implement a timer? I.e. if condition is no longer reported "wet" for NN seconds - we assume it to be "dry". |
I completely understand waiting a few days for feedback. Since the json is exposed via MQTT it's essentially a read-only API. Once committed, other software will begin to depend on it at which point it will be impossible to change without breaking things for people downstream. |
This is contrary to doctrine. See #2635 |
Given my (admittedly limited) understanding, I would be hesitant to implement a timer within the decoder for the following reasons:
Overall, even if implementing a timer at this layer were allowed, it's not clear that the solution would be correct for all users. As such I think it's something that should be implemented in HA on a per-user basis, if desired. |
I bought a bunch of Govee water sernsors a while back because they are cheap and a number of bloggers seemed to be using them without going deep enough into the details. I'm not doing much with them other than using them as an audible warning. Correct me if I'm wrong, but no amount of coding/heuristics is going to fix the problem that the only thing you actually know is that you might eventually get some sort of event message from the Govee sensors. Then again you may never receive another message. It does not send states such as dry, or even reliable heartbeat/battery message that lets you know it is still there and still able to be received. Trying to synthesize device state from these optimistic event messages is likely to be wrong in some case and give a false sense of security. If you don't get any more messages from the sensor -- you don't know that it is actually dry or actually still working and capable of being received. So, don't obscure that fact to the user, it could lead to real damage.
Since the device itself can't be trusted, the requirement that the device be visited seems about the only reasonable thing to do. Though after pushing the button, do you actually know that the device is in the dry state and will re-trigger when it gets wet again? |
If you wanted to follow the Zigbee model, something closer to Zigbee2MQTT -- use something like the mqtt relay script to process rtl_433 output and then send it out over MQTT for Home Assistant to consume.
|
I just performed a few manual tests on my device and I think the sensor operates more like a simple switch. On the model I have, when the metal contacts are shorted by liquid a circuit is closed, the alarm triggers, and a "wet" message is transmitted. As long as the circuit remains closed the "wet" message is continuously transmitted each time the audible alarm fires (one message every 2-3 seconds). When the liquid is wiped off the alarm stops and no more messages are transmitted. If the contacts are shorted again a new "wet" message is sent (regardless of whether the button is pressed beforehand or not) and, on newer models, the "leak number" in the message is incremented by one. I don't believe anything needs to be done to "re-arm" the sensor since it operates by blindly transmitting wet messages when the circuit is closed. Given this behavior, implementing a timer in a relay script might work for the common case but would likely report incorrectly in some scenarios (e.g. if the battery failed while wet or the internal transmitter or local receiver failed). Using the button press signal to represent "dry" in Home Assistant seems much safer since it implies a human investigated and pushed the button and aligns with what the HA community has converged on.
As described above, I would say these sensors reliably transmit messages to represent the "wet state" when shorted but do not send anything when they are dried off. The "event" json key in the decoder is overloaded and arguably misused, likely because the original govee decoder covers two different govee sensors (leak and door) but maps data to a single json key. The device is best represented as a binary moisture sensor in HA but lacks any message which indicates that they are in the dry state (from their perspective, they are dry if they are not transmitting "wet") meaning the HA entity would never transition back to dry once wet. Since adding any timing / state would almost certainly do more harm than good, the "button pressed" message is used to transition the binary moisture sensor entity in Home Assistant from wet to dry (it has no state-changing effect on the device itself). Additionally, based on the code, the sensor also occasionally sends battery health messages. I can confirm that they do send a battery message when the battery is first inserted, beyond that I'm not sure. I hope that when the battery runs low at minimum they start to beep (like a smoke detector) but I've not owned mine long enough to observe that behavior. |
Speaking of the batteries - it's worth mentioning the following topic (which, I believe, was named slightly differently in the past) and, specifically, comment 81:
Looks like it sends a battery state only when powered on or when the level drops by 20-25% |
My comment above raised another question in my head - is there a way to retain the last known battery level somewhere in MQTT or HA, so it survives through the HA core reboots and version upgrades? |
@kcpants - Thank you the very thorough analysis and all of you work on this. Regarding frequency of battery updates, in case it helps give you some insights, here is every message I captured from my 5 Govee HS054 sensors from roughly Jan 2022 to Sept. 2023. These were purchased in 2021. (the water leak reports were from cleaning people accidentally setting them off) There are months between battery reports. It is possible there were missed messages. The gap from 2022-12 to 2023-08 for 6845 seems like at least some battery updates were missed.
|
@Vlad-Star wrote:
The MQTT retain flag implementation in rtl_433 is currently very course grained, so it is essentially on or off. Given the infrequency of Govee updates, you'd like the retain flag to be on for all Govee related messages. But if you turn retain on, any messages that are from false positives will stay around in your MQTT broker until you delete them. This might be a case where using something like mqtt_relay with customization would be more useful rather than forwarding everything directly from rtl_433 to MQTT. I commented in another thread about how I'd like meta data about devices to be retained and would be a good use case for a companion app. The quickest, probably easiest solution would be to use a trigger in Home Assistant, since that data is retained across Hass restarts. |
@rct Thanks for the battery message data! All this discussion is great but I feel like it may be growing beyond the scope of the contents of this pull request so, in the interest of getting this merged, I'd like to try to refocus things. Although this change is part of a larger effort to make the Govee leak sensors more user-friendly, the important aspects of this particular fix seem to be the naming convention for json keys corresponding to binary sensors (which up to now have not been defined in To summarize, this change implicitly proposes a general naming convention of It seems like folks are happy with I propose that if another few days goes by without significant concerns regarding the main points in this fix it can be integrated and I will move on to the change in |
mqtt: Big picture, we need to separate "send json decode to receiver" from "bridge to some other system with defined semantics". The in-rtl_433 mqtt is only suitable for the first. As for button pressing creating detect_wet:0, is this documented by Govee? Is the button available for other use as a separate channel? If the sensor is still wet, will it send the button-press code, and then send another wet code in a few seconds? It really seems like button press should be mapped to a button-press event and the next-level software should sort out semantics. But if the convention is that the button should be pressed by a human when they decide it is no longer wet, I can see it. Overall I think this device is badly designed and I don't mind adapting to make it useful, but there should be comments that this is irregular to work around a bad design. |
This is what the H5054 manual says:
|
I performed a few more manual tests to see how the button behaves in conjunction with the alarm. When the sensor is wet (contacts shorted) the button does silence the alarm for ~5 seconds as described in the manual. When the button is used for this function no "Button Press" message is sent. Additionally, while the sensor was wet I was unable to change the volume by pressing the button twice. In other words, based on my testing, the "Button Press" message is only sent when the sensor is dry (contacts open).
Few thoughts on this: First, note that this change does not remove the original Second, the fact that the button press message is only transmitted when the device is dry makes me think it's even more appropriate to include Third, keep in mind that there is always some "semantic" mapping when converting any device's raw signal into json that will then be sent to MQTT. Correct me if I'm wrong but it looks like most (if not all) decoders are implemented by reverse engineering captured signals sent by devices, not from a manufacturer spec sheet. As such, the json keys used by decoders are not prescribed by the device manufacturer, they are chosen by the person who implements the decoder for the device. In this case, I think the original author of the decoder made an unfortunate choice of mapping everything to Lines 375 to 384 in 07368e7
As a related example drawn from the code snippet above, the govee decoder author decided to include the battery_ok key in the json output whenever any message containing battery data is received (battery >= 0 ) regardless of what the battery level actually is and further they chose to send the battery level as a double (presumably representing the percent charged) as the value associated with that key. This goes against how the battery_ok key is used by other decoder authors who generally transmit either zero or one depending on whether the battery is low or not (as reported by the device). You can find many examples of this usage by searching for battery_ok in the repo; I've linked to one below for convenience. Because govee doesn't seem to send its own binary battery_ok signal the decoder author synthesized their own battery_ok key-value pair from the data that govee does send (this might also be worth changing to align with other devices but I'd like to defer that to a future discussion / pull request): Line 172 in 07368e7
As always, I'm happy to add comments if requested and although I do agree that the behavior of this device is unusual and suboptimal I don't believe including detect_wet: 0 alongside event: "Button Press" in the json output diverges too much from the intended purpose of the decoders.
|
I'm not seeing detect_wet == 0, because it is only implemented for the older decoder. I have sensors purchased in 2021, but maybe these are the older version?
I haven't looked at this too closely, is there any reason the key standardization and detect_wet logic should apply to both decoders? |
There is no reason not to add First, I don't own any of the older devices so it would be impossible for me to thoroughly test a fix in the older decoder. Having worked as a software developer for several years, even if the change to the old decoder were straightforward, I would be very reluctant to merge any untested code. Second, the older decoder actually handles two govee devices so it's not quite as simple as copy-pasting the code that's currently in the PR up to the other decoder. In particular, it's important that the |
Given that the button doesn't send a button event when it's still wet, I'm ok with button getting mapped to detect_wet:0 as a binary sensor report, and a button press event (although if we're assigning semantics, I'm a little iffy on the button press event). As for battery_ok, I am confused this minute about what the rules are, between rtl_433 and home assistant. The concepts are binary sensor, and 0-100%. I'd have to dig in and figure out the rules. I think it's ok to fix the other issues and let that be and address battery separately. |
I apparently have older hardware and can test. If breaking changes are going to be introduced in the name of standardizing and fixing things, it seems like it would be good to make the older and newer govee leak detectors behave the same. Edit: - I don't have the govee door sensor, but if there aren't samples in rtl_433_tests (or a PR) maybe we can get someone who has one to test, but clearly the two could have different "data makes" based on the event type. |
From a coding perspective my main concern when I looked at the older decoder was the Lines 241 to 261 in 07368e7
I was unsure if adding detect_wet to the shared output_fields array would result in the new field incorrectly appearing for the door sensor. On the surface, it looks to be safe assuming that the DATA_COND for detect_wet is only true for the leak sensors and that a key is omitted when its DATA_COND is false but didn't feel comfortable making the change without being able to test it.
I agree it would be ideal to add the new key to the old decoder. I'd like to emphasize though that no breaking changes are being proposed. The change adds a new field to the json output but no existing fields are being removed or modified so any setups that rely on the As a volunteer developer, I'd be happy to help with the modifications for the old decoder but would prefer someone who actually owns the device to create and submit a separate pull request. I'm certain the original authors of the decoder code possess the physical devices they are writing code for so this seems like a reasonable restriction. |
I believe the output fields are only used for figuring out what columns should go into CSV output. So yes adding any changes to that list potentially breaks someone's CSV usage if they don't pay attention to the column headings. CSV output is pretty fragile, it is only practical in limited cases. Anyone feeding data to a system like Home Assistant is likely not using CSV. With regards to being a volunteer, we're all volunteers here. I don't want to stand in the way of this being merged -- that's someone else's decision. But I do think given your new decoder still identifies as Model == "Govee Water" that it will be confusing to have two devices with the same identifier and different semantics. One bit of feedback that I believe is still open is that @gdt requested that you document the detect_wet state that has been introduced because without reading the comments in this PR that logic is going to be fairly opaque. |
@kcpants - I don't think I can push an additional commit to your branch/PR. Here's the same mod for the old decoder: rct@eddb4f3 Tested with my older H5054 water detector. As far as intentional changes to the Govee contact sensor, if I'm understanding things correctly
So I think this is acceptable. However, I'm tagging @skilau - the author of the PR with the contact sensor changes for feedback. |
@rct Nice work, you beat me to the fix! I also just finished prepping a diff with similar changes for you to apply and test. After looking through the code again my other big concern was also whether the door sensor has a button and sends its own "Button Press" messages. As you observed, the code snippet below seems to imply that the only message sent by the door contact sensor is "Open" ( Lines 202 to 234 in 07368e7
With respect to my comment regarding volunteering, I didn't mean to imply that others are not also volunteering (very sorry if it was interpreted that way!). Just that as volunteers we all have a right to decide how much responsibility we are willing to assume. By submitting this PR I am accountable for the code in it and, if it caused any problems down the line, I feel it would be my duty to fix them if at all possible. I'm comfortable taking this on for code I can fully test but not for code that I would be dependent on others (who I may not be able to coordinate with in the future) to validate. |
@kcpants - You patch looks nearly identical to my commit, other than:
As @gdt requested, you should have a similar comment somewhere in the code to explain what's going on because it can't be inferred from the code. With regards to not wanting to make changes to code for devices you can't test, I understand that and agree with the sentiment That's what the tests repo was far before it fell out of use. But I and others have had to make changes after doing some due diligence in order to make progress. For example see src/devices/acurite.c While your concern for the Govee contact sensor users is admirable, I see it as somewhat likely that there will be future issues opened against rtl_433 because there being two different behaviors for devices that both identify themselves as "Govee Water". |
I see these devices as problematic and lean to rip off the bandaid and fix, not worrying about compat so much. I agree that inconsistency is worse than risk of untested trouble. |
Hi all! About the door/contact sensors, I do still have them, but I do not use them... |
403abe9
to
c70d79f
Compare
This is expected behaviour. The battery_ok value is always a floating point value (0-100%) but as seen in most decoders it's truncated to a simple 0/1. https://triq.org/rtl_433/DATA_FORMAT.html#common-device-data |
The |
c70d79f
to
4685f50
Compare
Added the fix for the old decoder as well as a slightly more detailed version of the suggested comment to both decoders. I also removed the As I've said, I'd prefer if the changes for the two decoders were merged in two separate pull requests but I'm fine with piggy-backing them if others are taking responsibility for the older sensors (ultimately, I'm sure these changes will save many hours of people's lives and am happy to see both get fixed). |
Tests complete. Latest It also might be worthwhile for someone who owns the older revision of the sensors to verify that the "button press" behaves the same way as it does on the newer sensor (i.e. short the sensor with water, press the button while the alarm is sounding and verify that no "button press" message is received). Manually Shorted Sensor Contacts (
|
Sorry I should have mentioned sooner that I confirmed that earlier before I made my code change. Anyway here's the output with the head of the current PR:
Looks good to me. Thanks for all your work on this! LGTM |
Sorry, late entry for the naming scheme: Another contender would be |
I see detect and sense as almost synonyms, but sense is more about how and detect is more the result. |
I don’t have a strong feeling about either detect or sense, but I think detect is seems to match the language used in Home Assistant. So I’d stick with that. |
I think we're ready to merge and @rct said so as well. So @zuckschwerdt go ahead if you concur. |
Big thanks to all of you for taking the time for this discussion and make this such a polished feature. |
This pull request aims to introduce a new standard json key,
wet
, to represent whether a binary moisture sensor detects water or not and applies the new key to the H5054 Govee Water Leak Detector device. This key will be added to the mappings dictionary inrtl_433_mqtt_hass.py
in a separate pull request so that devices which publish MQTT messages with this key will be automatically discovered as binary moisture sensor devices in HA.Notes
wet
ormoist
seemed like the most appropriate keys to represent binary moisture sensors. I chosewet
but would be happy to change the key name if requested.event
key in this decoder. However, existing deployments almost certainly rely on it so it seems safest to leave it in place so as not to disrupt anyone's home ecosystem.