Proposal: Flag diagnostic properties with the proper category

[jcsanyi]

Home Assistant entity_category supports marking an entity as diagnostic, which separates secondary readings from a device's main sensors in the UI. Theengs doesn't set this anywhere today, so every decoded property currently becomes a regular sensor in HA. It's worth noting that other MQTT gateways like Zigbee2MQTT already do support this.

To make this work, we would need to add ent_cat: diagnostic to the discovery message for properties it applies to.

Carrying this discussion over from theengs/gateway#322, where it came up alongside the RSSI proposal.

Scope

After taking an initial look through the 122 device files, the battery properties are the most obvious diagnostic ones to be flagged:

property	devices
batt	61	battery percentage
volt	23	battery voltage

These are almost all diagnostic, but we do need to be careful about sensors whose primary job is monitoring batteries - for those, they should not be flagged as diagnostic.

In addition, there's a small handful of additional sensors that may make sense to also mark as being diagnostic:

property	devices
packet / packet_1 / packet_2	9	packet counter for some sensors
txpower	4	tx reference power to help calculate distance for beacons

Options for flagging

There's two primary options for marking which properties are diagnostic:

1. Auto-detect based on the property name (batt, volt, packet, txpower) - and then allow exceptions to be flagged in the decoder for individual properties. That would minimize the changes needed in the decoder, at the expense of having the logic live in two separate places. If the auto-detect itself lives in the gateways, that is especially bad as it's split across repos and needs to be known/duplicated for the decoder to even make sense.

2. Mark each property directly in the decoder - no automatic inference. This means touching more decoder files, but has the advantage of keeping the logic more visible and centralized.

In either case, we'd add a new optional field in the decoder output - something like category: diagnostic, sitting alongside unit and name. The gateways should already ignore these extra property keys until they need to use them - so it should be safe to add this category value for either all diagnostic properties or for the exceptions. One thing to make sure we check though is overflows on string buffer length in OMG or other embedded gateways - extending the properties across the decoder library has the potential to lead to overflows if not done carefully.

Options for rolling out

Adding this change would result in changes for all users that are currently using these devices, and there's been some pushback on that before.

Having an easy way to override the entity category in HA would help minimize the risk, and I've proposed that in home-assistant/architecture#1442 - but the initial reaction isn't great and I'm not sure if it'll be approved.

@DigiH proposed a boolean config option to disable the diagnostic category entirely, allowing affected users to stick with the current behaviour. Defaulting that config to off would be the least impactful, but also goes against the spirit of @1technophile's earlier comment about not adding configs for things that are part of the standard. That was referring to RSSI at the time and we ended up leaning towards no config at all for that - but maybe a config that defaults to ON is a better fit here?

Other notes

This would be implemented after the RSSI entity is added in theengs/gateway#322, and RSSI should also be marked as diagnostic. If there's a config, it should affect the RSSI entity as well even though it's not device-specific.

I'm limiting this proposal to the Theengs Gateway as a first step, although we'd have to touch the decoder libs, so we need to make sure the changes are transparent to other projects that use them. I'd plan to port this to OMG later if it proves useful here. Note that there's some custom processing for BM2/BM6 battery sensors in OMG that need to be considered if/when we get there.

Open Questions

• do we flag packet and txpower as well, or just batt and volt?
• how do we encode this in the decoder libs?
  • just by exception, or for all diagnostic properties?
• do we add a config to disable this?
  • if so, what's the default?

(edit: minor update to the counts and added packet_1, packet_2 - I missed a handful of sensors in the original count)

[1technophile]

Here are my thoughts on this:

• Depending on the device some data may be considered as functional or diagnostic, functional -> device purpose (measure temperature), diagnostic -> device health (internal battery level)
• The decoder should be able to identify the device data, functional / diagnostic so that the data consumers know which level of priority should be given in their UI
• We should be mindfull of the decoder size as it is used on uC with limited capacities
  A rapid estimate if we consider adding "cat":"diag" to the battery properties outside of battery monitoring device gives 1-2Kb of Progmem which is acceptable.

On your questions:

• Yes we should flag packets and txpower
• All diagnostic properties (estimate gives limited impact)
• I would not consider a config at the decoder level, at gateway level it can be discussed separately

[DigiH]

I just realised that the whole Tag Property table is all messed up with the changed new doc system. Let get this fixed first so that it becomes clearer with the remaining free tag bits.

https://decoder.theengs.io/participate/adding-decoders.html#tag-property

[DigiH]

Ok, I fixed the table, and you can now see that for Byte[1] the Bits6-7] are still available to be assigned, so Bit[6] can become "bvpr" ("batteryvoltageprimary") for all the decoders where battery and/or voltage are primary properties.

You can turn on Advertising and Advanced Data in OMG and Theengs Gateway if you want to see the extra tag defined data in the published JSON.

[1technophile]

Yes, I'm ok with using a bit on the tag property

[DigiH]

As we already use a bit there to tag only the tracker devices which then selectively get a device_tracker discovery through that tag bit, sing something very similar for the Config discoveries seems just logical to me.

[DigiH]

I have implemented and merged the "bvpp" (Battery and/or voltage are primary properties) tag, so it can now be used for differentiation during discovery creation.

3da3902

[jcsanyi]

Thanks for getting that in so quickly!

One copy/paste typo I noticed - the docs table labels the new bit as Bit[5] rather than Bit[6].

Unrelated, but while you're in that table: the type list has ENRG twice, at 12 and 20, and 20-253 - Reserved for future use overlaps 20 - ENRG on the line above

[DigiH]

Thanks for spotting the errors 👍

[jcsanyi]

I've created theengs/gateway#325 for the gateway change to consume the new bvpp flag and publish ent_cat as part of the discovery payloads.