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

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

[Carol Wang]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

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?

Thanks!

[-- Attachment #2: Type: text/html, Size: 988 bytes --]

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

[Patrick Venture]

On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug@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?
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 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 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!

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

[Ed Tanous]

On 10/15/19 8:17 AM, Patrick Venture wrote:
> On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug@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?
> 

To my understanding, the phosphor-hwmon sensor system doesn't support
overriding sensor values at runtime, regardless of whether or not you
resolved the type issues with the arguments.  If your goal is just "get
it to work", I'd recommend switching to dbus-sensors (which I realize is
non-trivial for some systems).  With that said, it would be great to get
this int64 to double conversion going again.


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

bmcweb also works with both types for everything but the PATCH interface.

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

Another vote for moving to double.  The systems I work on don't use
phosphor-hwmon, so there's not a lot of ways to test the other
components.  The pieces that I use (dbus-sensors, phosphor-pid-control,
bmcweb) all work correctly with double.

The reviews to move to double are still open, unresolved, and James has
patches to several daemons that need to be converted.
https://gerrit.openbmc-project.xyz/q/topic:%22double-sensor%22+(status:open%20OR%20status:merged)

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

[Brad Bishop]

at 11:17 AM, Patrick Venture <venture@google.com> wrote:

> On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug@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?

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

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

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

[Brad Bishop]

at 1:07 PM, Ed Tanous <ed.tanous@intel.com> wrote:

> On 10/15/19 8:17 AM, Patrick Venture wrote:
>> On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug@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?
>
> To my understanding, the phosphor-hwmon sensor system doesn't support
> overriding sensor values at runtime, regardless of whether or not you
> resolved the type issues with the arguments.

I’m just curious…what is the use-case for writing a voltage sensor…or any  
sensor for that matter?  What do you do with the user-supplied value when  
the sensor is a piece of hardware that does not comprehend setting a  
value?  This is probably my lack of Redfish-fu showing.

FWIW when I originally defined the Sensor.Value interface, it was meant to  
be read only.  In fact the documentation says this, even if it is not  
enforced anywhere:

     All Sensor.Value properties are read-only.

If a device provides some kind of control mechanism there are other  
interfaces for expressing/modeling those, typically/ideally in the control  
namespace.

> If your goal is just "get
> it to work", I'd recommend switching to dbus-sensors (which I realize is
> non-trivial for some systems).  With that said, it would be great to get
> this int64 to double conversion going again.
>
>
>> 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?
>> 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.
>
> bmcweb also works with both types for everything but the PATCH interface.
>
>> 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 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 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.
>
> Another vote for moving to double.  The systems I work on don't use
> phosphor-hwmon, so there's not a lot of ways to test the other
> components.  The pieces that I use (dbus-sensors, phosphor-pid-control,
> bmcweb) all work correctly with double.
>
> The reviews to move to double are still open, unresolved, and James has
> patches to several daemons that need to be converted.
> https://gerrit.openbmc-project.xyz/q/topic:%22double-sensor%22+(status:open%20OR%20status:merged)

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

[James Feist]

On 10/15/19 12:21 PM, Brad Bishop wrote:
> at 1:07 PM, Ed Tanous <ed.tanous@intel.com> wrote:
> 
>> On 10/15/19 8:17 AM, Patrick Venture wrote:
>>> On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug@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?
>>
>> To my understanding, the phosphor-hwmon sensor system doesn't support
>> overriding sensor values at runtime, regardless of whether or not you
>> resolved the type issues with the arguments.
> 
> I’m just curious…what is the use-case for writing a voltage sensor…or 
> any sensor for that matter?  What do you do with the user-supplied value 
> when the sensor is a piece of hardware that does not comprehend setting 
> a value?  This is probably my lack of Redfish-fu showing.

We mostly use it for our test suite.. I'm not aware of many other usages 
at this time.

> 
> FWIW when I originally defined the Sensor.Value interface, it was meant 
> to be read only.  In fact the documentation says this, even if it is not 
> enforced anywhere:
> 
>      All Sensor.Value properties are read-only.
> 
> If a device provides some kind of control mechanism there are other 
> interfaces for expressing/modeling those, typically/ideally in the 
> control namespace.
> 
>> If your goal is just "get
>> it to work", I'd recommend switching to dbus-sensors (which I realize is
>> non-trivial for some systems).  With that said, it would be great to get
>> this int64 to double conversion going again.
>>
>>
>>> 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?
>>> 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.
>>
>> bmcweb also works with both types for everything but the PATCH interface.
>>
>>> 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 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 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.
>>
>> Another vote for moving to double.  The systems I work on don't use
>> phosphor-hwmon, so there's not a lot of ways to test the other
>> components.  The pieces that I use (dbus-sensors, phosphor-pid-control,
>> bmcweb) all work correctly with double.
>>
>> The reviews to move to double are still open, unresolved, and James has
>> patches to several daemons that need to be converted.
>> https://gerrit.openbmc-project.xyz/q/topic:%22double-sensor%22+(status:open%20OR%20status:merged) 
>>
> 

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

[Kun Yi]

[-- Attachment #1: Type: text/plain, Size: 5737 bytes --]

On Tue, Oct 15, 2019 at 1:32 PM James Feist <james.feist@linux.intel.com>
wrote:

> On 10/15/19 12:21 PM, Brad Bishop wrote:
> > at 1:07 PM, Ed Tanous <ed.tanous@intel.com> wrote:
> >
> >> On 10/15/19 8:17 AM, Patrick Venture wrote:
> >>> On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug@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?
> >>
> >> To my understanding, the phosphor-hwmon sensor system doesn't support
> >> overriding sensor values at runtime, regardless of whether or not you
> >> resolved the type issues with the arguments.
> >
> > I’m just curious…what is the use-case for writing a voltage sensor…or
> > any sensor for that matter?  What do you do with the user-supplied value
> > when the sensor is a piece of hardware that does not comprehend setting
> > a value?  This is probably my lack of Redfish-fu showing.
>
> We mostly use it for our test suite.. I'm not aware of many other usages
> at this time.
>

We write to margin sensors to control the PID fan loop. Under Redfish they
could be different typed resources.

>
> >
> > FWIW when I originally defined the Sensor.Value interface, it was meant
> > to be read only.  In fact the documentation says this, even if it is not
> > enforced anywhere:
> >
> >      All Sensor.Value properties are read-only.
> >
> > If a device provides some kind of control mechanism there are other
> > interfaces for expressing/modeling those, typically/ideally in the
> > control namespace.
> >
> >> If your goal is just "get
> >> it to work", I'd recommend switching to dbus-sensors (which I realize is
> >> non-trivial for some systems).  With that said, it would be great to get
> >> this int64 to double conversion going again.
> >>
> >>
> >>> 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?
> >>> 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.
> >>
> >> bmcweb also works with both types for everything but the PATCH
> interface.
> >>
> >>> 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 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 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.
> >>
> >> Another vote for moving to double.  The systems I work on don't use
> >> phosphor-hwmon, so there's not a lot of ways to test the other
> >> components.  The pieces that I use (dbus-sensors, phosphor-pid-control,
> >> bmcweb) all work correctly with double.
> >>
> >> The reviews to move to double are still open, unresolved, and James has
> >> patches to several daemons that need to be converted.
> >>
> https://gerrit.openbmc-project.xyz/q/topic:%22double-sensor%22+(status:open%20OR%20status:merged)
> >>
> >
>


-- 
Regards,
Kun

[-- Attachment #2: Type: text/html, Size: 8037 bytes --]

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

[Ed Tanous]

On 10/15/19 12:04 PM, Brad Bishop wrote:
> Is it possible to get bmcweb supporting patch with double and int?  On
> the surface this seems like the shortest path to enabling Carol.

Unless phosphor-hwmon is patched to support settable sensors, I don't
think there's a good reason to add support to bmcweb.  With that said,
it's just software, it could certainly be done.  Someone would need to
sort out how to manage the scale factor, as a Redfish PATCH request
doesn't have a concept of scale, and operates on a float, so you'd have
to do some software handholding to convert the value to something
appropriate before it was scaled to the int64.

With that said, all of this seems like a lot of work.


Just to clarify in case anyone isn't clear, the /xyz/* and /org/* type
REST handlers do support SetProperty for int and double, and introspects
the daemon to determine which one should be used.  So far as I'm aware,
the only thing non-functional is a Redfish patch of a sensor, as it's a
bit more complex to do properly.

-Ed

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

[Brad Bishop]

at 6:19 PM, Ed Tanous <ed.tanous@intel.com> wrote:

> On 10/15/19 12:04 PM, Brad Bishop wrote:
>> Is it possible to get bmcweb supporting patch with double and int?  On
>> the surface this seems like the shortest path to enabling Carol.
>
> Unless phosphor-hwmon is patched to support settable sensors, I don't
> think there's a good reason to add support to bmcweb.

Ok.  A couple different questions then.

What does it mean to be settable?  phosphor-hwmon implements Sensor.Value  
and, (in violation of the interface documentation), doesn’t prevent an  
external application from writing to the properties in it.

I assumed the assertion that phosphor-hwmon doesn’t provide settable  
sensors is because the value set by an external application doesn’t stick?   
The phosphor-hwmon application logic will write over it the next time it  
polls the hwmon device.

So what does dbus-sensors do for hardware sensors?  Does the application  
ignore the hardware state after being patched via Redfish?

> With that said,
> it's just software, it could certainly be done.  Someone would need to
> sort out how to manage the scale factor, as a Redfish PATCH request
> doesn't have a concept of scale, and operates on a float, so you'd have
> to do some software handholding to convert the value to something
> appropriate before it was scaled to the int64.
>
> With that said, all of this seems like a lot of work.

I don’t know of any alternatives that are less work.  If there are some,  
I’d love to hear about them.  There were several years of OpenBMC  
development against the integer interface signature before double became a  
problem - as Patrick mentioned - undoing all that will not be trivial.

>
> Just to clarify in case anyone isn't clear, the /xyz/* and /org/* type
> REST handlers do support SetProperty for int and double, and introspects
> the daemon to determine which one should be used.  So far as I'm aware,
> the only thing non-functional is a Redfish patch of a sensor, as it's a
> bit more complex to do properly.

I had a look at the Sensor schema in the latest Resource and Schema guide  
and the sensor reading property is read only.  Why do we even allow it to  
be patched?

>
> -Ed

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

[James Feist]

On 10/15/19 4:15 PM, Brad Bishop wrote:
> at 6:19 PM, Ed Tanous <ed.tanous@intel.com> wrote:
> 
>> On 10/15/19 12:04 PM, Brad Bishop wrote:
>>> Is it possible to get bmcweb supporting patch with double and int?  On
>>> the surface this seems like the shortest path to enabling Carol.
>>
>> Unless phosphor-hwmon is patched to support settable sensors, I don't
>> think there's a good reason to add support to bmcweb.
> 
> Ok.  A couple different questions then.
> 
> What does it mean to be settable?  phosphor-hwmon implements 
> Sensor.Value and, (in violation of the interface documentation), doesn’t 
> prevent an external application from writing to the properties in it.
> 
> I assumed the assertion that phosphor-hwmon doesn’t provide settable 
> sensors is because the value set by an external application doesn’t 
> stick?  The phosphor-hwmon application logic will write over it the next 
> time it polls the hwmon device.
> 
> So what does dbus-sensors do for hardware sensors?  Does the application 
> ignore the hardware state after being patched via Redfish?

Yes until the sensor is reset.


> 
>> With that said,
>> it's just software, it could certainly be done.  Someone would need to
>> sort out how to manage the scale factor, as a Redfish PATCH request
>> doesn't have a concept of scale, and operates on a float, so you'd have
>> to do some software handholding to convert the value to something
>> appropriate before it was scaled to the int64.
>>
>> With that said, all of this seems like a lot of work.
> 
> I don’t know of any alternatives that are less work.  If there are some, 
> I’d love to hear about them.  There were several years of OpenBMC 
> development against the integer interface signature before double became 
> a problem - as Patrick mentioned - undoing all that will not be trivial.
> 
>>
>> Just to clarify in case anyone isn't clear, the /xyz/* and /org/* type
>> REST handlers do support SetProperty for int and double, and introspects
>> the daemon to determine which one should be used.  So far as I'm aware,
>> the only thing non-functional is a Redfish patch of a sensor, as it's a
>> bit more complex to do properly.
> 
> I had a look at the Sensor schema in the latest Resource and Schema 
> guide and the sensor reading property is read only.  Why do we even 
> allow it to be patched?
> 

The intention was for this to only be in manufacturing test mode (not 
sure if that is true today or not). So under a normal user, it would be 
read only.

>>
>> -Ed
> 

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

[Brad Bishop]

at 7:19 PM, James Feist <james.feist@linux.intel.com> wrote:

> On 10/15/19 4:15 PM, Brad Bishop wrote:
>> at 6:19 PM, Ed Tanous <ed.tanous@intel.com> wrote:
>>> On 10/15/19 12:04 PM, Brad Bishop wrote:
>>>> Is it possible to get bmcweb supporting patch with double and int?  On
>>>> the surface this seems like the shortest path to enabling Carol.
>>>
>>> Unless phosphor-hwmon is patched to support settable sensors, I don't
>>> think there's a good reason to add support to bmcweb.
>> Ok.  A couple different questions then.
>> What does it mean to be settable?  phosphor-hwmon implements  
>> Sensor.Value and, (in violation of the interface documentation), doesn’t  
>> prevent an external application from writing to the properties in it.
>> I assumed the assertion that phosphor-hwmon doesn’t provide settable  
>> sensors is because the value set by an external application doesn’t  
>> stick?  The phosphor-hwmon application logic will write over it the next  
>> time it polls the hwmon device.
>> So what does dbus-sensors do for hardware sensors?  Does the application  
>> ignore the hardware state after being patched via Redfish?
>
> Yes until the sensor is reset.

Ok.  Well this behavior could be trivially added to phosphor-hwmon too if  
that improves the case for adding patch support for the integer interface.

>
>
>>> With that said,
>>> it's just software, it could certainly be done.  Someone would need to
>>> sort out how to manage the scale factor, as a Redfish PATCH request
>>> doesn't have a concept of scale, and operates on a float, so you'd have
>>> to do some software handholding to convert the value to something
>>> appropriate before it was scaled to the int64.
>>>
>>> With that said, all of this seems like a lot of work.
>> I don’t know of any alternatives that are less work.  If there are some,  
>> I’d love to hear about them.  There were several years of OpenBMC  
>> development against the integer interface signature before double became  
>> a problem - as Patrick mentioned - undoing all that will not be trivial.
>>> Just to clarify in case anyone isn't clear, the /xyz/* and /org/* type
>>> REST handlers do support SetProperty for int and double, and introspects
>>> the daemon to determine which one should be used.  So far as I'm aware,
>>> the only thing non-functional is a Redfish patch of a sensor, as it's a
>>> bit more complex to do properly.
>> I had a look at the Sensor schema in the latest Resource and Schema  
>> guide and the sensor reading property is read only.  Why do we even  
>> allow it to be patched?
>
> The intention was for this to only be in manufacturing test mode (not  
> sure if that is true today or not). So under a normal user, it would be  
> read only.

Ok, makes sense.  thx for the info James.

>
>>> -Ed

Re: 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@fuzziesquirrel.com> wrote:
>
> at 11:17 AM, Patrick Venture <venture@google.com> wrote:
>
> > On Mon, Oct 14, 2019 at 7:57 PM Carol Wang <karo33bug@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!
>

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

[Lei YU]

On Wed, Oct 16, 2019 at 8:45 AM Patrick Venture <venture@google.com> wrote:
> > >
> > > 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.
>

The phosphor-dbus-interface xyz.openbmc_project.Sensor.Value interface
specifically defines the `Value` property as int64, so it looks like
dbus-sensors is not following this interface.

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

The interface defines the Unit and Scale as well, so the application
code could provide a calculated value and unit to end-user, which is
easier to understand.

Going back to Carol's question, what she really needs to do is to
PATCH PowerCap, instead of PATCH a sensor value.
So we are OK that PATCH sensor does not work for phosphor-hwmon sensors.
Carol should be able to write some specific code in bmcweb to
introspect the PowerCap's DBus object  and handle both double and
int_64 case.