<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-im" style="color:rgb(80,0,80)">On 2/25/20 5:26 PM, Josh Lehan wrote:<br>> Hi there! Question about updateValue(), in sensor.hpp file, in<br>> dbus-sensors package.<br>><br>> In updateValue(), it always calls set_property() to update the stored<br>> value to the new passed-in value, sending the new value on D-Bus.<br>> However, it then tests the hysteresis variable, to avoid a very small<br>> change. It only updates the stored value if the change is significant<br>> enough. This logic in updateValue() seems strange to me. Wouldn't you<br>> want to either do both of these operations at once, or none of them at all?<br><br></span>This is to not trip thresholds if the change is small, so you don't jump<br>back and forth over thresholds. We still update the value on d-bus for<br>anyone reading the value to have the most recent value.<br></blockquote><div><br></div><div>If this is just for threshold comparison, would it make more sense to use a more typical method of applying hysteresis?</div><div>To simplify the code could we implement something with no hidden state, and only one bit of state like a schmitt trigger?</div><div><br></div><div>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?</div><div><br></div><div>static bool overThreshold;</div><div>overThreshold = (reading>__threshold-overThreshold*__thresholdHysteresis)</div><div><br></div><div>by making the state observable we make it much easier to understand, test and cover all the behavior.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 26, 2020 at 9:10 AM James Feist <<a href="mailto:james.feist@linux.intel.com">james.feist@linux.intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2/25/20 5:26 PM, Josh Lehan wrote:<br>
> Hi there! Question about updateValue(), in sensor.hpp file, in <br>
> dbus-sensors package.<br>
> <br>
> In updateValue(), it always calls set_property() to update the stored <br>
> value to the new passed-in value, sending the new value on D-Bus. <br>
> However, it then tests the hysteresis variable, to avoid a very small <br>
> change. It only updates the stored value if the change is significant <br>
> enough. This logic in updateValue() seems strange to me. Wouldn't you <br>
> want to either do both of these operations at once, or none of them at all?<br>
<br>
This is to not trip thresholds if the change is small, so you don't jump <br>
back and forth over thresholds. We still update the value on d-bus for <br>
anyone reading the value to have the most recent value.<br>
<br>
> <br>
> If D-Bus is updated but the stored value is not also updated, it would <br>
> cause them to get out of sync, over time. Think of a slowly changing <br>
> value, just under the hysteresis threshold, but with each new reading, <br>
> over time, it adds up to a substantial change. This would cause the <br>
> D-Bus value to be updated but the stored value to not be updated. Then, <br>
> if the sensor were to return to the value of the stored value, it would <br>
> falsely be seen as not an update.<br>
<br>
I believe it always compares to the stored value, so this wouldn't be <br>
possible, as eventually the reading will drift far enough away from the <br>
stored value that isn't being updated. If it isn't, it should be.<br>
<br>
<br>
> <br>
> Also, in the PSUSensor::handleResponse() function, it does a test for <br>
> equality before calling updateValue(). Shouldn't this test be done by <br>
> updateValue() itself? Interestingly, PSUSensor doesn't check for <br>
> hysteresis at all, so these are two different tests. I'm considering <br>
> refactoring it, to have updateValue() make the decision, so the decision <br>
> is all made in one place.<br>
<br>
Yes, this seems like a discrepancy. Not sure why PSU sensor didn't <br>
follow the existing logic.<br>
<br>
<br>
> <br>
> What's more, PSUSensor uses floating-point equality when testing for <br>
> equality. This is considered harmful, and in practice means that most <br>
> readings aren't considered equal, even when they effectively are, so <br>
> there's a lot of excess update traffic on the D-Bus, defeating most of <br>
> the hysteresis logic above.<br>
> <br>
> And, finally, the hysteresis variable is defined as ((max-min)*0.01) <br>
> which means that a value must be changed by 1/100 of full scale, in <br>
> order to be considered a change. This seems to be throwing away some <br>
> useful sensor resolution here. IPMI is "accurate" down to 1/255 of full <br>
> scale, so it would make sense to at least be as precise as that allows. <br>
> It might be more useful to lower the hysteresis threshold to 1/1000 of <br>
> full scale, which would give plenty of headroom.<br>
<br>
The value on d-bus is still updating at least for the sensors I'm aware <br>
of. This should only be used for the threshold comparison.<br>
<br>
> <br>
> Any thoughts on these points? Thanks for reading this.<br>
> Josh<br>
> <br>
> Josh Lehan | Software Engineer |<a href="mailto:krellan@google.com" target="_blank">krellan@google.com</a> <br>
> <mailto:<a href="mailto:krellan@google.com" target="_blank">krellan@google.com</a>> | <a href="tel:(650)%20733-8941" value="+16507338941" target="_blank">+1 650-733-8941</a><br>
> <br>
</blockquote></div>