Ping Integration - diagnostic entities unit of measurement issue

[alexruffell]

The problem

I created a Ping sensor, using the Ping Integration, for a remote IP (1.1.1.1 in this example). I then enabled the 3 diagnostic "round trip time" entities without changing any defaults.

The default display precision for Average is: Default (11.476) with a Unit of Measurement of ms.

However, in the Diagnostic card it shows as 0.011 without a Unit of Measure. It is as if the Unit of Measure were s and not ms. Unless you change the default precision, this applies to all 3 entities. The 3rd one in the snapshot below has already been fixed.

To fix the display of the 3rd entity, you just need to select the same Display Precision as the default or another of your choosing.

Once you do that, the ping time is displayed in the desired format and the Unit of Measure is added.

The expected behavior is for the Diagnostic card to show the ping times using the defaulted precision and unit of measure without having to re-select the precision.

Below is what it should look like immediately after enabling the entities if the user were to leave everything at default setting:

On another note, it is my opinion that decimals for a Ping time expressed in ms are most often irrelevant which makes me wonder why the default wouldn't be set to no decimals (like Windows ping) or just one decimal (like *nux - at least the distribution I checked).

This looks more appropriate for majority of use cases:

What version of Home Assistant Core has the issue?

core-2024.7.2

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Ping (ICMP)

Link to integration documentation on our website

https://www.home-assistant.io/integrations/ping

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

[home-assistant[bot]]

Hey there @jpbede, mind taking a look at this issue as it has been labeled with an integration (ping) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of ping can trigger bot actions by commenting:

• @home-assistant close Closes the issue.
• @home-assistant rename Awesome new title Renames the issue.
• @home-assistant reopen Reopen the issue.
• @home-assistant unassign ping Removes the current integration label and assignees on the issue, add the integration domain after the command.
• @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue.
• @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

---

ping documentation ping source _{^{(message by IssueLinks)}}

[zSprawl]

I'm running 2024.9.3 and I'm seeing the same issue, except I can't get it to stop displaying seconds despite "hard setting it" to ms and with any size display precision.

As you can see, the graphed value (statistic) is correctly converted to ms, but the displayed value is not.

I was enabling these sensors to prepare for 2024.10.1. Does anyone know if this is still an issue in the latest?

[jpbede]

Thanks for bringing this up 👍

This is a unfortunately frontend issue and there is already a issue for this https://github.com/home-assistant/frontend/issues/21984. This is how the frontend currently handles duration sensors. We already set the right unit of measurement in the code:

https://github.com/home-assistant/core/blob/131fdf6898197dce96807427c34c1bfd609f3f82/homeassistant/components/ping/sensor.py#L42-L51

On another note, it is my opinion that decimals for a Ping time expressed in ms are most often irrelevant which makes me wonder why the default wouldn't be set to no decimals (like Windows ping) or just one decimal (like *nux - at least the distribution I checked).

There was no particular reason for this. I can change that

[parautenbach]

On another note, it is my opinion that decimals for a Ping time expressed in ms are most often irrelevant which makes me wonder why the default wouldn't be set to no decimals (like Windows ping) or just one decimal (like *nux - at least the distribution I checked).

For internet pings, perhaps, but on a local network your ping times can definitely below 1 ms.

[parautenbach]

device_class=SensorDeviceClass.DURATION, 

@jpbede may I ask why the old attributes were made diagnostic sensors? To me, diagnostic sensors are to get more info on the device, typically for debugging. The mdev, min, max, etc. to me are just all part of the basic functionality of a ping. It caught me out trying to find my old attributes, as I didn't expect it to be under diagnostics. I thought it would've been additional sensors one could enable under the configuration (like how some other integrations with config flow does it when it provides additional sensors).

[jpbede]

may I ask why the old attributes were made diagnostic sensors?

Every update (that's every ping cycle) has caused a state update on the state machine and not every body uses those attributes. Additionally this has cause a lot of entries in the state history.

To me, diagnostic sensors are to get more info on the device, typically for debugging. The mdev, min, max, etc. to me are just all part of the basic functionality of a ping.

This is right, but "diagnostics" (and "configuration) is just the category they appear in the UI. It does not have any other effects on the sensors. Those are sensors just like any other sensor entity.

I thought it would've been additional sensors one could enable under the configuration (like how some other integrations with config flow does it when it provides additional sensors).

You can enable then as any other disabled sensors. They are disabled by default as a lot of users do not need them, they just use the binary sensor or the device tracker.

Changing them to appear under "Sensors" is straightforward.

[alexruffell]

For internet pings, perhaps, but on a local network your ping times can definitely below 1 ms.

True, pinging my router returns values between 0.111 and 0.210ms but that degree of precision seems pointless to me in most non professional use cases. Anyhow that is easy for each user to pick what they prefer so it is a non issue.

However, to your point, when I create a new ping sensor, the unit of measure defaults to second instead of millisecond so it looks like this:

Not only is it confusing because the s UOM is not shown, it also loses the <1ms precision you want.

[parautenbach]

may I ask why the old attributes were made diagnostic sensors?

Every update (that's every ping cycle) has caused a state update on the state machine and not every body uses those attributes. Additionally this has cause a lot of entries in the state history.

Sorry, I was unclear. The emphasis was on diagnostic. I'm on board with the long-running architectural changes to get rid of (dynamic) attributes.

This is right, but "diagnostics" (and "configuration) is just the category they appear in the UI. It does not have any other effects on the sensors. Those are sensors just like any other sensor entity.

I think that the semantics of this do matter. To me it wasn't intuitive at all to look there for these sensors. I understand that it doesn't affect the behaviour of these sensors. Granted, building it into the config flow would've been more effort.

You can enable then as any other disabled sensors. They are disabled by default as a lot of users do not need them, they just use the binary sensor or the device tracker.

Correct. Not disputing whether they should've been enabled by default. I think it's fine to have them disabled by default. It was about my point above.

Anyway, I've taken this discussion a bit off course.

Fixing/solving the duration challenges is more important at this point.

[ausfas]

So I guess this will be fixed in future release?

[jpbede]

So I guess this will be fixed in future release?

When someone proposes a fix for it, yes. Again this is a issue within the frontend (home-assistant/frontend#21984), which not only applies to the Ping integration. On integration side we already do what we can do, we set the right unit of measurement and the right state class.

[gabry45]

The Frontend issue had been closed and I see this working correctly now. I think this issue can be closed now :)