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

[Patrick Venture]

On Tue, Oct 15, 2019 at 12:04 PM Brad Bishop
<bradleyb at fuzziesquirrel.com> wrote:
>
> 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?

There may very well be a lesson here.  In this case, The push to
double was pushed back, and then we ended up with software that
followed one version of the interface, and other bits that followed
another version.  Most of the software is compatible with both types
here, but having two types does feel like it's defeating the
well-defined dbus interfaces.

>
> > 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?

I argue that maintaining the sensor value in its final form saves some
operations and I'd like to normalize to that.

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