* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 16:24 ` Ed Tanous
@ 2020-09-16 17:08 ` Patrick Williams
2020-09-16 17:19 ` Ed Tanous
2020-09-16 17:49 ` Andrei Kartashev
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Patrick Williams @ 2020-09-16 17:08 UTC (permalink / raw)
To: Ed Tanous; +Cc: James Feist, openbmc, Andrei Kartashev
[-- Attachment #1: Type: text/plain, Size: 3549 bytes --]
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.
>
> >
> > > 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.
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.
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.
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 17:08 ` Patrick Williams
@ 2020-09-16 17:19 ` Ed Tanous
2020-09-16 17:29 ` Patrick Williams
0 siblings, 1 reply; 12+ messages in thread
From: Ed Tanous @ 2020-09-16 17:19 UTC (permalink / raw)
To: Patrick Williams; +Cc: James Feist, openbmc, Andrei Kartashev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 17:19 ` Ed Tanous
@ 2020-09-16 17:29 ` Patrick Williams
2020-09-17 4:13 ` Ed Tanous
0 siblings, 1 reply; 12+ messages in thread
From: Patrick Williams @ 2020-09-16 17:29 UTC (permalink / raw)
To: Ed Tanous; +Cc: James Feist, openbmc, Andrei Kartashev
[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]
On Wed, Sep 16, 2020 at 10:19:56AM -0700, Ed Tanous wrote:
> On Wed, Sep 16, 2020 at 10:08 AM Patrick Williams <patrick@stwcx.xyz> wrote:
> > 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?
Yes. Originally the IPMI hanlders did and they seem to still.
https://github.com/openbmc/phosphor-host-ipmid/blob/master/sensorhandler.cpp#L652
I don't know what happens to this code when a dbus-sensors sensor is
read.
> > 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.
Agreed. But, a C-enum to C-enum map is easier to maintain than a path
segment extraction and string to C-enum map.
> > 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".
We could also remove the path segment requirement. I'm not sure the
value of it in one way or another. It is only redundant to a human,
isn't it?
--
Patrick Williams
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 17:29 ` Patrick Williams
@ 2020-09-17 4:13 ` Ed Tanous
0 siblings, 0 replies; 12+ messages in thread
From: Ed Tanous @ 2020-09-17 4:13 UTC (permalink / raw)
To: Patrick Williams; +Cc: James Feist, openbmc, Andrei Kartashev
On Wed, Sep 16, 2020 at 10:29 AM Patrick Williams <patrick@stwcx.xyz> wrote:
>
> On Wed, Sep 16, 2020 at 10:19:56AM -0700, Ed Tanous wrote:
> > On Wed, Sep 16, 2020 at 10:08 AM Patrick Williams <patrick@stwcx.xyz> wrote:
> > > 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?
>
> Yes. Originally the IPMI hanlders did and they seem to still.
>
> https://github.com/openbmc/phosphor-host-ipmid/blob/master/sensorhandler.cpp#L652
>
> I don't know what happens to this code when a dbus-sensors sensor is
> read.
>
> > > 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.
>
> Agreed. But, a C-enum to C-enum map is easier to maintain than a path
> segment extraction and string to C-enum map.
Fair point, although the segment extraction is usually 2 lines of
code, so it's in the noise a bit.
>
> > > 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".
>
> We could also remove the path segment requirement. I'm not sure the
> value of it in one way or another. It is only redundant to a human,
> isn't it?
I'd be in support of that too, but that's probably more work. I have
no problem adding Unit to the sensors, just curious if it was needed.
>
> --
> Patrick Williams
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 16:24 ` Ed Tanous
2020-09-16 17:08 ` Patrick Williams
@ 2020-09-16 17:49 ` Andrei Kartashev
2020-09-16 18:23 ` i.kononenko
2020-09-16 18:36 ` Andrei Kartashev
3 siblings, 0 replies; 12+ messages in thread
From: Andrei Kartashev @ 2020-09-16 17:49 UTC (permalink / raw)
To: Ed Tanous, James Feist; +Cc: openbmc
On Wed, 2020-09-16 at 09:24 -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.
>
> > > 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?
I really don't like this approach to determine Unit based on Path. It
is really not logical, I was surprised to find that "Objects
implementing Sensor.Value must be instantiated in the correct hierarchy
within the sensors namespace." Isn't interfaces are supposed to break
out hierarchy dependency?
More of then, now we have same algorithm if determining units in
different places - this is potential source of inconsistency.
I vote for implementing "Unit" property and removing calculation units
based on path everywhere.
--
Best regards,
Andrei Kartashev
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 16:24 ` Ed Tanous
2020-09-16 17:08 ` Patrick Williams
2020-09-16 17:49 ` Andrei Kartashev
@ 2020-09-16 18:23 ` i.kononenko
2020-09-17 4:24 ` Ed Tanous
2020-09-16 18:36 ` Andrei Kartashev
3 siblings, 1 reply; 12+ messages in thread
From: i.kononenko @ 2020-09-16 18:23 UTC (permalink / raw)
To: openbmc; +Cc: Andrei Kartashev
On 16.09.2020 19:24, 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.
I have a number of patches which adds the "Unit" dbus-property to each of
dbus-sensors (adcsensor, pwmsensor, etc.),
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?
>
> 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?
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.
>>
>>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 18:23 ` i.kononenko
@ 2020-09-17 4:24 ` Ed Tanous
2020-09-17 21:42 ` i.kononenko
0 siblings, 1 reply; 12+ messages in thread
From: Ed Tanous @ 2020-09-17 4:24 UTC (permalink / raw)
To: Ed Tanous, James Feist; +Cc: openbmc, Andrei Kartashev
On Wed, Sep 16, 2020 at 6:31 PM i.kononenko <i.kononenko@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@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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-17 4:24 ` Ed Tanous
@ 2020-09-17 21:42 ` i.kononenko
0 siblings, 0 replies; 12+ messages in thread
From: i.kononenko @ 2020-09-17 21:42 UTC (permalink / raw)
To: openbmc; +Cc: Andrei Kartashev
On 17.09.2020 07:24, Ed Tanous wrote:
> On Wed, Sep 16, 2020 at 6:31 PM i.kononenko <i.kononenko@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@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.
I'd prepare a gerrit-changes [1] which adds this feature.
However, for note, the change affect the topic of dependency between 'dbus-sensors' and 'phosphor-dbus-interfaces'.
I'v pretty sure we must do add this dependency, because sdbusplus enum's
have a special cast to stringview which every dbus-consumers will expected.
I want kendly ask, actually, have we way to implement that stuff without add
depend on 'phosphor-dbus-interfaces'?
[1] https://gerrit.openbmc-project.xyz/q/topic:sensors_unit
>
>> 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
--
best,
Igor Kononenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: dbus-sensors: Unit property for xyz.openbmc_project.Sensor.Value interface
2020-09-16 16:24 ` Ed Tanous
` (2 preceding siblings ...)
2020-09-16 18:23 ` i.kononenko
@ 2020-09-16 18:36 ` Andrei Kartashev
3 siblings, 0 replies; 12+ messages in thread
From: Andrei Kartashev @ 2020-09-16 18:36 UTC (permalink / raw)
To: openbmc
On Wed, 2020-09-16 at 09:24 -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.
>
> > > 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?
I really don't like this approach to determine Units based on Path. It
is really not logical, I was surprised to find that "Objects
implementing Sensor.Value must be instantiated in the correct hierarchy
within the sensors namespace." Isn't interfaces are supposed to break
out hierarchy dependency?
Semantically consumers can only guess units, but only dbus-sensors
being data source really know units, so it would be logical that dbus-
sensors explicitly exports units.
More of then, now we have same algorithm of determining units in
different places - this is potential source of inconsistency.
I vote for implementing "Unit" property and removing calculation units
based on path everywhere.
PS: sorry if this message would duplicate, but I didn't receive back
the message I have sent hour ago.
--
Best regards,
Andrei Kartashev
^ permalink raw reply [flat|nested] 12+ messages in thread