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

[Brad Bishop]

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.

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)