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

[Ed Tanous]

On Wed, Sep 16, 2020 at 6:31 PM i.kononenko <i.kononenko at yadro.com> wrote:
>
>
>
> On 16.09.2020 19:24, Ed Tanous wrote:
> > On Wed, Sep 16, 2020 at 8:23 AM James Feist <james.feist at 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.
>
> I have a number of patches which adds the "Unit" dbus-property to each of
> dbus-sensors (adcsensor, pwmsensor, etc.),

Do you have a link?  I don't see them on gerrit.

> Also this changes append dependency from phosphor-dbus-interfaces to dbus-sensors recipe.
> Does the dbus-sensors package intentionally lack the phosphor-dbus-interfaces?
> Or can I safely add this dependency?

I'd rather not not, seeing as how we've made it this far without the
dependency, and we're not consistently using it, it seems odd to add
it just to support the Units property.  If asio were adjusted to use
it consistently, and we ported all the code over, and it made it
simpler, I'd be in more support.

>
> >
> > 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.
> >
>
> Ed, I'm sure that each sensor's unit may be obtained by analyzing a dbus-path in each service,
> but is it really any better than using well-conforming dbus-interface's?

In the sense that most of the dependent daemons support using path for
Units, yes, it would be easier to change.  In terms of assuming we're
going to push patches to all the dependent daemons, Units would
probably be better than using path.

> Setting the Unit once at the single centralized place (I'd suggest it should be dbus-sensors,
> which defenetly knows better about units) we'll prevent any further code duplication and
> re-implementing of this feature in the future.

Yep, I think we agree.  If you put up patchsets to do this, I'd be in
support.  Are you planning on making this change?

>
> >>
> >>> 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?
> >
>
> --
> best,
>
> Igor Kononenko