Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface

[Ed Tanous <ed@tanous.net>]

On Wed, Sep 16, 2020 at 10:08 AM Patrick Williams <patrick@stwcx.xyz> wrote:
>
> I would prefer we implement Units in dbus-sensors.  Reasons below.
>
> On Wed, Sep 16, 2020 at 09:24:19AM -0700, Ed Tanous wrote:
> > On Wed, Sep 16, 2020 at 8:23 AM James Feist <james.feist@linux.intel.com> wrote:
> > >
> > > On 9/16/2020 6:28 AM, Andrei Kartashev wrote:
> > > > Hi,
> > > >
> > > > We noticed that dbus-sensors doesn't fully implement
> > > > xyz.openbmc_project.Sensor.Value interface: there is no Unit property
> > > > for all the sensors, defined by dbus-sensors.
> > > > I believe it was intentionally, but I still wondering what was the
> > > > reason?
> > >
> > > It was originally as the information seemed redundant. If the
> > > information is needed I'm fine with someone adding it, it just hasn't
> > > seemed to be a high priority.
> >
> > Considering we've gone this long with no impact (considering the path
> > can be used to lookup the unit) I wonder if we should consider
> > removing unit from the sensor Value API?  It doesn't seem used.
>
> The redundancy comes about from this list: [1].  I don't really recall
> why we went with that approach where sensors were required to be in a
> particular place in the object hierarchy.
>
> I think we only "got away" with "no impact" from the lack of
> implementation because the particular implementations that work well
> with dbus-sensors also didn't implement it because dbus-sensors didn't
> provide it.  That's kind of circular logic as a reason to eliminate it.

Are there any implementations that people use that rely on Units?

>
> >
> > >
> > > > I noticed that in intel-ipmi-oem units are determined based on object
> > > > paths, but that looks ugly since there is well-defined natural
> > > > interface for units in dbus.
> > > > Lack of the "Unit" property in the interface breaks some existing
> > > > logic.
> > > >
> >
> > Technically the way the interfaces define it, both are valid to use to
> > determine the Units, and both would need to be lookup tables.  Is
> > using the path any more ugly than using the property?
>
> There isn't really a programatic way to define and enforce object paths
> presently.  There is a programatic way to define and enforce
> enum values.  The enums don't actually need a lookup table, if you're
> using the sdbusplus generated headers and bindings.

The lookup table I was referring to was enum-value -> IPMI value.  I
don't think sdbusplus has the ability to generate that for you.

>
> If I were to guess which of the requirements would be more likely to be
> changed it would be the object path.  There are very few other places
> where we have such strict requirements on object paths (though we do
> have places where the object path has meaning).  The current definition
> is a bit ambiguous by what is meant by "the correct hierarchy within the
> sensors namespace[2]", but the current implementations seem to take this
> to mean `/xyz/openbmc_project/sensors/...`[3].  I don't know that this is
> particular convenient for a multi-host system or any case where a BMC
> is aggregating sensors from other BMCs.

I'd be fine with this solution too.  The question I should've asked is
"is there a way to get rid of the redundant information".

>
> Unless we get some stronger definition on what is meant by [2], the only
> completely correct code is to look for the type at all the various
> segments of the object path (ex. "fan_tach" could be found at
> /xyz/openbmc_project/sensors/fan_tach/node2/fan0 or
> /node2/xyz/openbmc_project/sensors/fan_tach/fan0).  Using the enum value
> is far easier in this case.

Fair point.  To be clear, I'm fine with adding Units it to
dbus-sensors;  I was just looking if this was an opportunity to
simplify.  It sounds like the answer is no, and there are important
uses for this.  If we did anything we would probably remove the type
in the path, and that seems far more intrusive.

>
> 1. https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/xyz/openbmc_project/Sensor/Value.interface.yaml#L37
> 2. https://github.com/openbmc/phosphor-dbus-interfaces/blob/master/xyz/openbmc_project/Sensor/Value.interface.yaml#L3
> 3. https://github.com/openbmc/bmcweb/blob/cb13a39253848ece442971301ade9c09d98bf08e/redfish-core/lib/sensors.hpp#L56
>
> --
> Patrick Williams