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

[i.kononenko]

On 17.09.2020 07:24, Ed Tanous wrote:
> 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.

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