dbus-sensors updateValue update hysteresis logic question

[James Feist]

On 2/25/20 5:26 PM, Josh Lehan wrote:
> 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?

This is to not trip thresholds if the change is small, so you don't jump 
back and forth over thresholds. We still update the value on d-bus for 
anyone reading the value to have the most recent value.

> 
> 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.

I believe it always compares to the stored value, so this wouldn't be 
possible, as eventually the reading will drift far enough away from the 
stored value that isn't being updated. If it isn't, it should be.


> 
> 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.

Yes, this seems like a discrepancy. Not sure why PSU sensor didn't 
follow the existing logic.


> 
> 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.

The value on d-bus is still updating at least for the sensors I'm aware 
of. This should only be used for the threshold comparison.

> 
> Any thoughts on these points? Thanks for reading this.
> Josh
> 
> Josh Lehan | Software Engineer |krellan at google.com 
> <mailto:krellan at google.com> | +1 650-733-8941
>