dbus-sensors updateValue update hysteresis logic question

[Josh Lehan]

Thanks for your feedback. Here's the change I wrote. I see you responded
there already, thanks again, I'll still paste the URL for future reference:

https://gerrit.openbmc-project.xyz/c/openbmc/dbus-sensors/+/29890

Josh

Josh Lehan | Software Engineer | krellan at google.com | +1 650-733-8941



On Wed, Feb 26, 2020 at 9:09 AM James Feist <james.feist at linux.intel.com>
wrote:

> 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 <(650)%20733-8941>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20200302/7ba25c70/attachment.htm>