<div dir="ltr"><div>Hi there! Question about updateValue(), in sensor.hpp file, in dbus-sensors package.</div><div><br></div><div>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?</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>Any thoughts on these points? Thanks for reading this.</div><div>Josh</div><div><br></div><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></div>