The type of sensor value in redfish should be double or int64?

[James Feist]

On 10/15/19 12:21 PM, Brad Bishop wrote:
> at 1:07 PM, Ed Tanous <ed.tanous at intel.com> wrote:
> 
>> On 10/15/19 8:17 AM, Patrick Venture wrote:
>>> On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug at gmail.com> wrote:
>>>> Hi,
>>>>
>>>> I tried to override sensor values with redfish Patch, referring to 
>>>> the Voltages example of 
>>>> https://github.com/openbmc/bmcweb/search?q=413961de&type=Commits, 
>>>> but met an error "Invalid argument". Comparing the code 
>>>> https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/sensors.hpp#L2192 
>>>> and the doc 
>>>> https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/xyz/openbmc_project/Sensor/Value.interface.yaml#L21, 
>>>> the current code uses double type, but the doc says the value type 
>>>> should be int64. It seems a scale difference. Why the code uses 
>>>> double here?
>>
>> To my understanding, the phosphor-hwmon sensor system doesn't support
>> overriding sensor values at runtime, regardless of whether or not you
>> resolved the type issues with the arguments.
> 
> I’m just curious…what is the use-case for writing a voltage sensor…or 
> any sensor for that matter?  What do you do with the user-supplied value 
> when the sensor is a piece of hardware that does not comprehend setting 
> a value?  This is probably my lack of Redfish-fu showing.

We mostly use it for our test suite.. I'm not aware of many other usages 
at this time.

> 
> FWIW when I originally defined the Sensor.Value interface, it was meant 
> to be read only.  In fact the documentation says this, even if it is not 
> enforced anywhere:
> 
>      All Sensor.Value properties are read-only.
> 
> If a device provides some kind of control mechanism there are other 
> interfaces for expressing/modeling those, typically/ideally in the 
> control namespace.
> 
>> If your goal is just "get
>> it to work", I'd recommend switching to dbus-sensors (which I realize is
>> non-trivial for some systems).  With that said, it would be great to get
>> this int64 to double conversion going again.
>>
>>
>>> The short answer is, code generally authored by Intel uses doubles for
>>> the sensor values, whereas the original OpenBMC sensor models used
>>> int64.
>>>
>>> A while back there was a bit of a debate about this, and it looks like
>>> we never truly resolved it.  Brad, perhaps this is a good time to?
>>> The idea is, that with the int64 storage, we store the raw value and
>>> we also store the scaling factor, so that one can scale the number if
>>> they choose to.  Sort of keeping the values as they are -- instead of
>>> operating on them before publishing them to dbus.  We do notably
>>> however, modify values in phosphor-hwmon before publishing them as
>>> often there are scaling factors and offsets, beyond the conversion
>>> from mC to C.
>>>
>>> phosphor-host-ipmid's sensor YAML requires the type to be specified
>>> for the sensor value interface already, and does validly support both
>>> types: int64_t and double (I tested it when I briefly had a mixed
>>> environment).
>>> phosphor-hwmon reports values without applying the scaling factor, but
>>> it is aware of the scaling factor, so it could.
>>> phosphor-pid-control works with both types correctly.
>>
>> bmcweb also works with both types for everything but the PATCH interface.
>>
>>> I'm not sure about the phosphor-fan stuff, but I imagine we'll find
>>> some arithmetic we can drop where it ingests values.
>>>
>>> This change from int64 to double is big enough though that many
>>> configuration files would need to be adjusted, which is non-trivial.
>>> I argue however that having code that doesn't have a common interface
>>> or interfaces for sensors is less than ideal.  Having multiple options
>>> for talking to sensors is fine, but in my opinion this is only true if
>>> they implement different interfaces, or the same interface.  In this
>>> case, we have dbus-sensors and phosphor-hwmon which both implement the
>>> same interface, but differently: int64 vs double.
>>>
>>> I think using doubles makes sense at the dbus level because generally
>>> one wants that version of the value.  Therefore you end up with code
>>> in each daemon that reads the sensor value and the scale so that it
>>> can perform the same operation that another daemon is also going to
>>> perform, etc.  Instead of just doing it once.
>>>
>>> I'll climb off my debate box now and climb onto my voting box and say,
>>> I'd like to make phosphor-hwmon report the value as double and I'm
>>> willing to review incoming patches that address other aspects of the
>>> codebase to bring it all together -- since they'll need to be in a
>>> locked step-forward.
>>
>> Another vote for moving to double.  The systems I work on don't use
>> phosphor-hwmon, so there's not a lot of ways to test the other
>> components.  The pieces that I use (dbus-sensors, phosphor-pid-control,
>> bmcweb) all work correctly with double.
>>
>> The reviews to move to double are still open, unresolved, and James has
>> patches to several daemons that need to be converted.
>> https://gerrit.openbmc-project.xyz/q/topic:%22double-sensor%22+(status:open%20OR%20status:merged) 
>>
>