dbus-sensors updateValue update hysteresis logic question

[Josh Lehan]

Hi there! Question about updateValue(), in sensor.hpp file, in dbus-sensors
package.

In updateValue(), it always calls set_property() to update the stored value
to the new passed-in value, sending the new value on D-Bus. However, it
then tests the hysteresis variable, to avoid a very small change. It only
updates the stored value if the change is significant enough. This logic in
updateValue() seems strange to me. Wouldn't you want to either do both of
these operations at once, or none of them at all?

If D-Bus is updated but the stored value is not also updated, it would
cause them to get out of sync, over time. Think of a slowly changing value,
just under the hysteresis threshold, but with each new reading, over time,
it adds up to a substantial change. This would cause the D-Bus value to be
updated but the stored value to not be updated. Then, if the sensor were to
return to the value of the stored value, it would falsely be seen as not an
update.

Also, in the PSUSensor::handleResponse() function, it does a test for
equality before calling updateValue(). Shouldn't this test be done by
updateValue() itself? Interestingly, PSUSensor doesn't check for hysteresis
at all, so these are two different tests. I'm considering refactoring it,
to have updateValue() make the decision, so the decision is all made in one
place.

What's more, PSUSensor uses floating-point equality when testing for
equality. This is considered harmful, and in practice means that most
readings aren't considered equal, even when they effectively are, so
there's a lot of excess update traffic on the D-Bus, defeating most of the
hysteresis logic above.

And, finally, the hysteresis variable is defined as ((max-min)*0.01) which
means that a value must be changed by 1/100 of full scale, in order to be
considered a change. This seems to be throwing away some useful sensor
resolution here. IPMI is "accurate" down to 1/255 of full scale, so it
would make sense to at least be as precise as that allows. It might be more
useful to lower the hysteresis threshold to 1/1000 of full scale, which
would give plenty of headroom.

Any thoughts on these points? Thanks for reading this.
Josh

Josh Lehan | Software Engineer | krellan at google.com | +1 650-733-8941
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20200225/11672e3a/attachment.htm>