<div dir="ltr">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:<div><div><br></div><div><a href="https://gerrit.openbmc-project.xyz/c/openbmc/dbus-sensors/+/29890">https://gerrit.openbmc-project.xyz/c/openbmc/dbus-sensors/+/29890</a></div><div><br></div><div>Josh<br></div><div><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div style="line-height:1.5em;padding-top:10px;margin-top:10px;color:rgb(85,85,85);font-family:sans-serif"><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Josh Lehan |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(0,153,57);padding-top:2px;margin-top:2px"> <a href="mailto:krellan@google.com" target="_blank">krellan@google.com</a> |</span><span style="border-width:2px 0px 0px;border-style:solid;border-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> +1 650-733-8941</span></div><br></div></div></div></div></div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 26, 2020 at 9:09 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>