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

Confusing battery_ok conundrum #2948

Closed
DigiH opened this issue May 30, 2024 · 11 comments
Closed

Confusing battery_ok conundrum #2948

DigiH opened this issue May 30, 2024 · 11 comments

Comments

@DigiH
Copy link
Contributor

DigiH commented May 30, 2024

This has been bugging me for ages, but with my ON/OFF consistency PR I though I finally might as well get it off my chest.

battery_ok, when being used as a binary 0/1 n decoders could be such a nice proper binary for all of them, also when creating HA discovery messages, for a very nice

Screenshot 2024-05-30 at 16 31 46

Unfortunately, instead it uses a very awkward transformation of

"battery_ok": {
        "device_type": "sensor",
        "object_suffix": "B",
        "config": {
            "device_class": "battery",
            "name": "Battery",
            "unit_of_measurement": "%",
            "value_template": "{{ float(value) * 99 + 1 }}",
            "state_class": "measurement",
            "entity_category": "diagnostic"
        }
    },

causing 0/off to be 1% and 1/on to be 100%.

This has already caused some YouTube reviewers to mention that there must be something wrong in the code, as their battery showed 1% , but the weather station was still running fine - that didn't make sense to them.

As there seem to be only a few decoders (TBH Archos etc.) which actually do produce a float for their battery level, my suggestion would be to rename their battery key to something like battery_level to then apply the above or any other required transformation, but to leave all battery_ok instances as they are, a nicely interpretable binary, which, when applied correctly in a discovery message, nicely produces the above for HA, and for all other controllers a binary 0/1 also makes a lot more sense than 1 and 100.

@zuckschwerdt
Copy link
Collaborator

We did document that battery_ok is a level between 0 and 1, with most devices not using any steps in between though.
https://triq.org/rtl_433/DATA_FORMAT.html#common-device-data

I'm not sure why the mapping for HA is 1% to 100%. It might be to indicate low-but-not-dead or some problem where 0 would not show?

It is a unified value (0–1) for all types of devices though. The extra bit is battery_mV, where supported.

@DigiH
Copy link
Contributor Author

DigiH commented May 30, 2024

I'm not sure why the mapping for HA is 1% to 100%. It might be to indicate low-but-not-dead or some problem where 0 would not show?

Because the above pasted discovery implementation for "battery_ok" only ever uses the
"value_template": "{{ float(value) * 99 + 1 }}",
for all of the sensors, creating a 1% to 100% only for all "battery_ok" discoveries., even if they are actually correct binary 0/1 values.

So I still think that two separate keys "battery_ok" and "battery_level" would be the clearest solution, or alternatively a check for float or integer would be required to actually correctly create discovery messages for the currently ambiguous battery_ok definition you linked to above - which would still cause ambiguity for 1/100%.

I have to admit though, that I'm coming from OpenMQTTGateway, where the above discovery routine has been taken over 1:1.

@zuckschwerdt
Copy link
Collaborator

I actually meant the "why" (the "how" is clear, I wrote that as a cheap example in #1016), I can't remember any particular reason.

If there is some foo to fix it in the script or the template that would be preferred. It took years to carefully get all decoders to the "battery_ok" standard without deprecating too much too soon along the way. I don't think we'll ever touch that "standard" again.

@DigiH
Copy link
Contributor Author

DigiH commented May 30, 2024

I actually meant the "why"

To have an actual sensible battery LOW/OK discovery for HA discoveries for correct binary battery_ok decoders, and proper 0% - 100% with all the possible steps in between for all float battery indicating decoders, instead of only 100% and 1% for all of them.

As actually described in
https://triq.org/rtl_433/DATA_FORMAT.html#common-device-data
but there it only describes on how it should be reported, but unfortunately doesn't allow to make any distinction between float or binary when being read for discovery purposes.

This would avoid user confusion for all general users as well as YouTube review users.

It took years to carefully get all decoders to the "battery_ok" standard without deprecating too much too soon along the way.

I just wished then that there would have been a distinction between proper float battery levels and LOW/OK binary ones from the beginning, not having them pooled together under one "battery_ok", so that the majority of binary LOW/OK statuses would actually be discovered correctly and only ever not as either 100% and 1% only.

@DigiH
Copy link
Contributor Author

DigiH commented May 30, 2024

As I personally do not use discovery at all, all my rtl_433 devices have been properly implemented to use correct binary LOW/OK statuses - as I do not have any which report a float.

So with any reluctance to touch this issue, I'm fine with closing this and a continued awkward mismatching discovery. Up to all the other users to wrangle with the current output ;)

It was still worth a shot … to try and make rtl_433 at least a bit more auto-discovery friendly …

@DigiH
Copy link
Contributor Author

DigiH commented May 30, 2024

Ok, I cumbersomely fixed this for OpenMQTTGateway, now correctly discovering binary battery_ok statuses as such, which is actually the majority of decoders, looking like
Screenshot 2024-05-31 at 00 35 28
and
Screenshot 2024-05-31 at 00 38 54

while still discovering, and therefore displaying, all float battery statuses as sensors with a rounded integer and the % unit, i. e. 1% - 100%
image

Closing this for rtl_433, as auto-discovery doesn't seem to be a high priority

@DigiH DigiH closed this as completed May 30, 2024
@zuckschwerdt
Copy link
Collaborator

That looks very nice.
You are right, there is no maintainer for the rtl_433_mqtt_hass.py script currently.

@DigiH
Copy link
Contributor Author

DigiH commented May 31, 2024

Unfortunately this is still very awkward to do, with different types populating the same property key in rtl_433.

That is likely also why the rtl_433_mqtt_hass.py script and the resulting HA rtl_433 MQTT Auto Discovery Add-on only handle the simple unified properties which are always floats for all decoders, like temperature_C, temperature_F, humidity, moisture etc. but completely ignoring all other existing ambiguous property keys.

Just some examples to show where my frustration is coming from 😜

UV and/or UVI keys for several weather stations - also broken in rtl_433 MQTT Auto Discovery
https://community.openmqttgateway.com/t/ha-sensor-missing-for-fineoffset-wh65b-weather-station/3223

Nexa Security, only discovered with the single universal RSSI key, but no other properties at all - also broken in rtl_433 MQTT Auto Discovery
https://community.openmqttgateway.com/t/missing-discovery-messages-for-nexa-433mhz-devices/3305

From your name and profile here I think you might also speak German, so at 6:20 of - actually this issue here - also broken in rtl_433 MQTT Auto Discovery
https://www.youtube.com/watch?v=KbRNd1DyRbg

And some issue discussions here, where missing keys are asked for to be included in the auto-discovery. This will only be possible though when selectively conditioning these keys with their applicable type decoders, as otherwise all remaining keys in other decoders with that particular name will be discovered, but being of different types they will always only ever show up as Unknown in the wrongly discovered UI - possibly even worse than not being discovered at all.

This is also what I had to do with the "state" key when implementing the Nexa Security mentioned above. Only allowing "state" to be discovered as a binary_sensor for the Nexa-Security, Brennenstuhl-RCS2044, Proove-Security and Waveman-Switch. With the last one actually not yet working, because of
#2946

So because of these different types being included in the same name keys it is currently virtually impossible, or only possible with very meticulous and long work and a pain to maintain, to have auto-discovery working nicely for all of rtl_433 ☹️

@gdt
Copy link
Collaborator

gdt commented May 31, 2024

To really fix this, I think we need to have a document with a normative data dictionary that says what keys mean and types, and then change the decoders that don't line up with that, separating per-decoder keys that just tell it like they got it (bit fidelity) and keys that are in a standard namespace. The elephant in the room: are we willing to break backwards compat to fix things, or aren't we? I think we should be willing.

@DigiH
Copy link
Contributor Author

DigiH commented May 31, 2024

The elephant in the room: are we willing to break backwards compat to fix things, or aren't we? I think we should be willing.

That is always the big worry ;) and I also commented this for the yet to be merged battery_ok fix for OpenMQTTGateway. But if a complete and well done auto-discovery for rtl_433 is desired at all, breaking backwards compatibility is unavoidable.

My vote would also be yes, as more and more HA users, and other controllers picking up the HA auto-discovery convention, rely on auto-discovery a lot.

@DigiH
Copy link
Contributor Author

DigiH commented May 31, 2024

Quite frankly, that is also why I started with logging this issue and the property casing consistency PR, as both are braking, but not majorly breaking, to see what the responses would be.

And property casing consistency is one vital base requirement for all possible other changes towards a better auto-discoverabilty for any binary string properties.

So both might be a starting test bed for any such changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants