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

[Ed Tanous]

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.  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)