<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 15, 2019 at 1:32 PM 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 10/15/19 12:21 PM, Brad Bishop wrote:<br>
> at 1:07 PM, Ed Tanous <<a href="mailto:ed.tanous@intel.com" target="_blank">ed.tanous@intel.com</a>> wrote:<br>
> <br>
>> On 10/15/19 8:17 AM, Patrick Venture wrote:<br>
>>> On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <<a href="mailto:karo33bug@gmail.com" target="_blank">karo33bug@gmail.com</a>> wrote:<br>
>>>> Hi,<br>
>>>><br>
>>>> I tried to override sensor values with redfish Patch, referring to <br>
>>>> the Voltages example of <br>
>>>> <a href="https://github.com/openbmc/bmcweb/search?q=413961de&type=Commits" rel="noreferrer" target="_blank">https://github.com/openbmc/bmcweb/search?q=413961de&type=Commits</a>, <br>
>>>> but met an error "Invalid argument". Comparing the code <br>
>>>> <a href="https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/sensors.hpp#L2192" rel="noreferrer" target="_blank">https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/sensors.hpp#L2192</a> <br>
>>>> and the doc <br>
>>>> <a href="https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/xyz/openbmc_project/Sensor/Value.interface.yaml#L21" rel="noreferrer" target="_blank">https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/xyz/openbmc_project/Sensor/Value.interface.yaml#L21</a>, <br>
>>>> the current code uses double type, but the doc says the value type <br>
>>>> should be int64. It seems a scale difference. Why the code uses <br>
>>>> double here?<br>
>><br>
>> To my understanding, the phosphor-hwmon sensor system doesn't support<br>
>> overriding sensor values at runtime, regardless of whether or not you<br>
>> resolved the type issues with the arguments.<br>
> <br>
> I’m just curious…what is the use-case for writing a voltage sensor…or <br>
> any sensor for that matter?  What do you do with the user-supplied value <br>
> when the sensor is a piece of hardware that does not comprehend setting <br>
> a value?  This is probably my lack of Redfish-fu showing.<br>
<br>
We mostly use it for our test suite.. I'm not aware of many other usages <br>
at this time.<br></blockquote><div><br></div><div>We write to margin sensors to control the PID fan loop. Under Redfish they could be different typed resources.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> FWIW when I originally defined the Sensor.Value interface, it was meant <br>
> to be read only.  In fact the documentation says this, even if it is not <br>
> enforced anywhere:<br>
> <br>
>      All Sensor.Value properties are read-only.<br>
> <br>
> If a device provides some kind of control mechanism there are other <br>
> interfaces for expressing/modeling those, typically/ideally in the <br>
> control namespace.<br>
> <br>
>> If your goal is just "get<br>
>> it to work", I'd recommend switching to dbus-sensors (which I realize is<br>
>> non-trivial for some systems).  With that said, it would be great to get<br>
>> this int64 to double conversion going again.<br>
>><br>
>><br>
>>> The short answer is, code generally authored by Intel uses doubles for<br>
>>> the sensor values, whereas the original OpenBMC sensor models used<br>
>>> int64.<br>
>>><br>
>>> A while back there was a bit of a debate about this, and it looks like<br>
>>> we never truly resolved it.  Brad, perhaps this is a good time to?<br>
>>> The idea is, that with the int64 storage, we store the raw value and<br>
>>> we also store the scaling factor, so that one can scale the number if<br>
>>> they choose to.  Sort of keeping the values as they are -- instead of<br>
>>> operating on them before publishing them to dbus.  We do notably<br>
>>> however, modify values in phosphor-hwmon before publishing them as<br>
>>> often there are scaling factors and offsets, beyond the conversion<br>
>>> from mC to C.<br>
>>><br>
>>> phosphor-host-ipmid's sensor YAML requires the type to be specified<br>
>>> for the sensor value interface already, and does validly support both<br>
>>> types: int64_t and double (I tested it when I briefly had a mixed<br>
>>> environment).<br>
>>> phosphor-hwmon reports values without applying the scaling factor, but<br>
>>> it is aware of the scaling factor, so it could.<br>
>>> phosphor-pid-control works with both types correctly.<br>
>><br>
>> bmcweb also works with both types for everything but the PATCH interface.<br>
>><br>
>>> I'm not sure about the phosphor-fan stuff, but I imagine we'll find<br>
>>> some arithmetic we can drop where it ingests values.<br>
>>><br>
>>> This change from int64 to double is big enough though that many<br>
>>> configuration files would need to be adjusted, which is non-trivial.<br>
>>> I argue however that having code that doesn't have a common interface<br>
>>> or interfaces for sensors is less than ideal.  Having multiple options<br>
>>> for talking to sensors is fine, but in my opinion this is only true if<br>
>>> they implement different interfaces, or the same interface.  In this<br>
>>> case, we have dbus-sensors and phosphor-hwmon which both implement the<br>
>>> same interface, but differently: int64 vs double.<br>
>>><br>
>>> I think using doubles makes sense at the dbus level because generally<br>
>>> one wants that version of the value.  Therefore you end up with code<br>
>>> in each daemon that reads the sensor value and the scale so that it<br>
>>> can perform the same operation that another daemon is also going to<br>
>>> perform, etc.  Instead of just doing it once.<br>
>>><br>
>>> I'll climb off my debate box now and climb onto my voting box and say,<br>
>>> I'd like to make phosphor-hwmon report the value as double and I'm<br>
>>> willing to review incoming patches that address other aspects of the<br>
>>> codebase to bring it all together -- since they'll need to be in a<br>
>>> locked step-forward.<br>
>><br>
>> Another vote for moving to double.  The systems I work on don't use<br>
>> phosphor-hwmon, so there's not a lot of ways to test the other<br>
>> components.  The pieces that I use (dbus-sensors, phosphor-pid-control,<br>
>> bmcweb) all work correctly with double.<br>
>><br>
>> The reviews to move to double are still open, unresolved, and James has<br>
>> patches to several daemons that need to be converted.<br>
>> <a href="https://gerrit.openbmc-project.xyz/q/topic:%22double-sensor%22+(status:open%20OR%20status:merged)" rel="noreferrer" target="_blank">https://gerrit.openbmc-project.xyz/q/topic:%22double-sensor%22+(status:open%20OR%20status:merged)</a> <br>
>><br>
> <br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr">Regards,<div>Kun</div></div></div></div>