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

[Brad Bishop]

at 11:17 AM, Patrick Venture <venture at google.com> 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?
>
> 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?

Is it possible to get bmcweb supporting patch with double and int?  On the  
surface this seems like the shortest path to enabling Carol.

> 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.
>
> 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 agree.  This whole situation is…unfortunate.  I wonder if there is a  
lesson to be learned here?

> 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

Why?  Do you have a desire for this specifically or is it just to solve  
Carol’s issue?

> 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.
>
>> Thanks!