Add battery_charging & last_seen attributes to GPSLogger #108118
Conversation
Skip update if last_seen went backwards.
| ATTR_ACTIVITY: attr.get(ATTR_ACTIVITY), | ||
| ATTR_ALTITUDE: attr.get(ATTR_ALTITUDE), | ||
| ATTR_BATTERY_CHARGING: attr.get(ATTR_BATTERY_CHARGING), |
There was a problem hiding this comment.
Battery charging should be a binary sensor entity.
There was a problem hiding this comment.
I don't care so much about this piece of data, but why make this special when all other miscellaneous data are attributes?
There was a problem hiding this comment.
It's our current standard to put all measurements that are relevant on their own as separate sensor entities.
There was a problem hiding this comment.
So, does that mean I need to pull out all the others, too, and make them separate sensor entities? At a minimum, shouldn't that also apply to activity, altitude, battery_level, direction & speed? And can I assume, since you did not reply to my comments about last_seen, it's ok to leave it as an attribute on the device_tracker entity?
There was a problem hiding this comment.
Existing attributes don't need to be moved at this time, since that would be a breaking change.
We don't allow new additions of attributes that should be sensors.
Last seen is not allowed as an attribute.
There was a problem hiding this comment.
Then there's no point.
| ATTR_DIRECTION: attr.get(ATTR_DIRECTION), | ||
| ATTR_LAST_SEEN: last_seen, |
There was a problem hiding this comment.
Last seen should be a timestamp sensor entity.
There was a problem hiding this comment.
It is much easier to compare multiple device_tracker entities for a single physical device (e.g., when using multiple presence detection integrations) when last_seen is an attribute, not a separate entity. And there is already precendence for a timestamp like this to be an attribute. E.g., last_seen in (the now defunct) Life360, Google Maps' last_seen, Tile's last_timestamp, and possibly even asuswrt's last_time_reachable. It is crucial for my custom composite integration that many, many people use.
There was a problem hiding this comment.
FWIW, I understand splitting out miscellaneous data into separate sensors as opposed to the original tendency to keep adding attributes to an existing entity. I have no problem with that in general.
However, in this case, I would argue that this is a perfect example of what an attribute should be used for. It is adding information about the main state (latitude, longitude & accuracy) that makes using that data more accurate. I.e., it describes when the data actually changed (as opposed to the last_updated field, that just indicates when the data was received by HA, which can be, and often is, very different.)
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
MartinHjelmare
changed the title
Accuracy should be 0 instead of None. Handle case when restored without last_seen attribute.
Proposed change
The GPSLogger app can provide battery charging status and an update timestamp. Allow configuring these in the Android app and, if present in the received webhook packet, add them as attributes to the GPSLogger tracker entities.
Note that it is possible for packets from the Android app to be received out of order due to network delays, etc. If this happens, the
device_trackerentity will move "backwards" (because it will be updated with old GPS data.) This can be prevented by utilizing thelast_seentimestamp. If a packet is received with alast_seenvalue that is before the previously received packet'slast_seenvalue, skip the newly received packet (and log a debug message to document this has happened.) This change implements that safeguard (which the Google Maps integration also implements, and the Life360 integration did before it was removed.)Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: