dbus-sensors updateValue update hysteresis logic question

[Andrew Macrae]

>
> 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 this is just for threshold comparison, would it make more sense to use a
more typical method of applying hysteresis?
To simplify the code could we implement something with no hidden state, and
only one bit of state like a schmitt trigger?

For clarification, could we say the threshold is violated when the value
rises above it, but the violation is only
resolved/cleared/no-longer-violated when the readings have fallen by an
additional hysteresis value below the threshold?

static bool overThreshold;
overThreshold = (reading>__threshold-overThreshold*__thresholdHysteresis)

by making the state observable we make it much easier to understand, test
and cover all the behavior.

On Wed, Feb 26, 2020 at 9:10 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/20200226/fc99dd4a/attachment.htm>