Notion of Flakiness in Sensor Readings

Notion of Flakiness in Sensor Readings

[Patrick Venture]

Currently, there are a few approaches in phosphor-hwmon on how to
handle sensors being present sometimes, or flaky on reads.  We've
talked in the past about how to handle a read failure that should be
ignored.  A real failure that is flaky or temporary.

I was thinking of just adding a property to the Sensor.Value interface
to report information.  Basically, someone reading the value needs to
know if it's valid.  The idea of a range check for validity doesn't
work as cleanly for this specific purpose, in my opinion.  The min/max
can be used cleanly to determine the linearization though!

I was thinking perhaps a boolean, that you can check to see if the
value should be used or trusted.  Then, I thought, perhaps, a enum
with a series of states, starting with the states of "Valid',
"Invalid."  Not being able to think of a third state, I fell back onto
a boolean.

Thoughts?

Patrick

Re: Notion of Flakiness in Sensor Readings

[James Feist]

On 10/12/2018 01:53 PM, Patrick Venture wrote:
> Currently, there are a few approaches in phosphor-hwmon on how to
> handle sensors being present sometimes, or flaky on reads.  We've
> talked in the past about how to handle a read failure that should be
> ignored.  A real failure that is flaky or temporary.
> 
> I was thinking of just adding a property to the Sensor.Value interface
> to report information.  Basically, someone reading the value needs to
> know if it's valid.  The idea of a range check for validity doesn't
> work as cleanly for this specific purpose, in my opinion.  The min/max
> can be used cleanly to determine the linearization though!
> 
> I was thinking perhaps a boolean, that you can check to see if the
> value should be used or trusted.  Then, I thought, perhaps, a enum
> with a series of states, starting with the states of "Valid',
> "Invalid."  Not being able to think of a third state, I fell back onto
> a boolean.

For Valid / Invalid we've produced nan on D-Bus to know that the sensor 
is in an invalid state (like power off for tach values). Although I'm 
not sure how this would be produced using the int64 interface, for the 
double std::nan will go over d-bus correctly.

> 
> Thoughts?
> 
> Patrick
> 

Re: Notion of Flakiness in Sensor Readings

[Patrick Venture]

On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
>
> On 10/12/2018 01:53 PM, Patrick Venture wrote:
> > Currently, there are a few approaches in phosphor-hwmon on how to
> > handle sensors being present sometimes, or flaky on reads.  We've
> > talked in the past about how to handle a read failure that should be
> > ignored.  A real failure that is flaky or temporary.
> >
> > I was thinking of just adding a property to the Sensor.Value interface
> > to report information.  Basically, someone reading the value needs to
> > know if it's valid.  The idea of a range check for validity doesn't
> > work as cleanly for this specific purpose, in my opinion.  The min/max
> > can be used cleanly to determine the linearization though!
> >
> > I was thinking perhaps a boolean, that you can check to see if the
> > value should be used or trusted.  Then, I thought, perhaps, a enum
> > with a series of states, starting with the states of "Valid',
> > "Invalid."  Not being able to think of a third state, I fell back onto
> > a boolean.
>
> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
> is in an invalid state (like power off for tach values). Although I'm
> not sure how this would be produced using the int64 interface, for the
> double std::nan will go over d-bus correctly.

So if i just wait until we're using double, the issue basically
resolves itself? :D :D

>
> >
> > Thoughts?
> >
> > Patrick
> >

Re: Notion of Flakiness in Sensor Readings

[Matt Spinler]

On 10/12/2018 4:23 PM, Patrick Venture wrote:
> On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
>> On 10/12/2018 01:53 PM, Patrick Venture wrote:
>>> Currently, there are a few approaches in phosphor-hwmon on how to
>>> handle sensors being present sometimes, or flaky on reads.  We've
>>> talked in the past about how to handle a read failure that should be
>>> ignored.  A real failure that is flaky or temporary.
>>>
>>> I was thinking of just adding a property to the Sensor.Value interface
>>> to report information.  Basically, someone reading the value needs to
>>> know if it's valid.  The idea of a range check for validity doesn't
>>> work as cleanly for this specific purpose, in my opinion.  The min/max
>>> can be used cleanly to determine the linearization though!
>>>
>>> I was thinking perhaps a boolean, that you can check to see if the
>>> value should be used or trusted.  Then, I thought, perhaps, a enum
>>> with a series of states, starting with the states of "Valid',
>>> "Invalid."  Not being able to think of a third state, I fell back onto
>>> a boolean.

If the hwmon driver for your device provides a _fault attribute, and 
that gets set, the code is already
set up to create an OperationalStatus interface on that sensor instance 
and set the Functional
property to false.

>> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
>> is in an invalid state (like power off for tach values). Although I'm
>> not sure how this would be produced using the int64 interface, for the
>> double std::nan will go over d-bus correctly.
> So if i just wait until we're using double, the issue basically
> resolves itself? :D :D
>
>>> Thoughts?
>>>
>>> Patrick
>>>

Re: Notion of Flakiness in Sensor Readings

[Patrick Venture]

On Fri, Oct 12, 2018 at 2:28 PM Matt Spinler
<mspinler@linux.vnet.ibm.com> wrote:
>
>
>
> On 10/12/2018 4:23 PM, Patrick Venture wrote:
> > On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
> >> On 10/12/2018 01:53 PM, Patrick Venture wrote:
> >>> Currently, there are a few approaches in phosphor-hwmon on how to
> >>> handle sensors being present sometimes, or flaky on reads.  We've
> >>> talked in the past about how to handle a read failure that should be
> >>> ignored.  A real failure that is flaky or temporary.
> >>>
> >>> I was thinking of just adding a property to the Sensor.Value interface
> >>> to report information.  Basically, someone reading the value needs to
> >>> know if it's valid.  The idea of a range check for validity doesn't
> >>> work as cleanly for this specific purpose, in my opinion.  The min/max
> >>> can be used cleanly to determine the linearization though!
> >>>
> >>> I was thinking perhaps a boolean, that you can check to see if the
> >>> value should be used or trusted.  Then, I thought, perhaps, a enum
> >>> with a series of states, starting with the states of "Valid',
> >>> "Invalid."  Not being able to think of a third state, I fell back onto
> >>> a boolean.
>
> If the hwmon driver for your device provides a _fault attribute, and
> that gets set, the code is already
> set up to create an OperationalStatus interface on that sensor instance
> and set the Functional
> property to false.

I'd like to drop this such that the failure behavior can be returning
a NaN double.
https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127

For the drivers that don't have the fault attribute, and in the case
where phosphor-host-ipmid doesn't check for that kind of failure.

>
> >> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
> >> is in an invalid state (like power off for tach values). Although I'm
> >> not sure how this would be produced using the int64 interface, for the
> >> double std::nan will go over d-bus correctly.
> > So if i just wait until we're using double, the issue basically
> > resolves itself? :D :D
> >
> >>> Thoughts?
> >>>
> >>> Patrick
> >>>
>

Re: Notion of Flakiness in Sensor Readings

[Matthew Barth]

On 10/12/2018 04:32 PM, Patrick Venture wrote:
> On Fri, Oct 12, 2018 at 2:28 PM Matt Spinler
> <mspinler@linux.vnet.ibm.com> wrote:
>>
>>
>>
>> On 10/12/2018 4:23 PM, Patrick Venture wrote:
>>> On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
>>>> On 10/12/2018 01:53 PM, Patrick Venture wrote:
>>>>> Currently, there are a few approaches in phosphor-hwmon on how to
>>>>> handle sensors being present sometimes, or flaky on reads.  We've
>>>>> talked in the past about how to handle a read failure that should be
>>>>> ignored.  A real failure that is flaky or temporary.
>>>>>
>>>>> I was thinking of just adding a property to the Sensor.Value interface
>>>>> to report information.  Basically, someone reading the value needs to
>>>>> know if it's valid.  The idea of a range check for validity doesn't
>>>>> work as cleanly for this specific purpose, in my opinion.  The min/max
>>>>> can be used cleanly to determine the linearization though!
>>>>>
>>>>> I was thinking perhaps a boolean, that you can check to see if the
>>>>> value should be used or trusted.  Then, I thought, perhaps, a enum
>>>>> with a series of states, starting with the states of "Valid',
>>>>> "Invalid."  Not being able to think of a third state, I fell back onto
>>>>> a boolean.
>>
>> If the hwmon driver for your device provides a _fault attribute, and
>> that gets set, the code is already
>> set up to create an OperationalStatus interface on that sensor instance
>> and set the Functional
>> property to false.
> 
> I'd like to drop this such that the failure behavior can be returning
> a NaN double.
> https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127
> 
> For the drivers that don't have the fault attribute, and in the case
> where phosphor-host-ipmid doesn't check for that kind of failure.
> 
IMHO, we should handle this by removing the sensor from dbus instead of 
using a special value type if we're unable to use the OperationalStatus. 
In either case, the code utilizing this sensor would need to 
specifically handle this NaN value or handle having the sensor removed 
from dbus. It seems more generically, to me, to have hwmon remove the 
sensor from dbus instead of using a NaN double so this can be used 
across other sensor drivers that may not return a double type.
>>
>>>> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
>>>> is in an invalid state (like power off for tach values). Although I'm
>>>> not sure how this would be produced using the int64 interface, for the
>>>> double std::nan will go over d-bus correctly.
>>> So if i just wait until we're using double, the issue basically
>>> resolves itself? :D :D
>>>
>>>>> Thoughts?
>>>>>
>>>>> Patrick
>>>>>
>>
> 

Re: Notion of Flakiness in Sensor Readings

[Patrick Venture]

On Mon, Oct 15, 2018 at 6:40 AM Matthew Barth <msbarth@linux.ibm.com> wrote:
>
>
>
> On 10/12/2018 04:32 PM, Patrick Venture wrote:
> > On Fri, Oct 12, 2018 at 2:28 PM Matt Spinler
> > <mspinler@linux.vnet.ibm.com> wrote:
> >>
> >>
> >>
> >> On 10/12/2018 4:23 PM, Patrick Venture wrote:
> >>> On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
> >>>> On 10/12/2018 01:53 PM, Patrick Venture wrote:
> >>>>> Currently, there are a few approaches in phosphor-hwmon on how to
> >>>>> handle sensors being present sometimes, or flaky on reads.  We've
> >>>>> talked in the past about how to handle a read failure that should be
> >>>>> ignored.  A real failure that is flaky or temporary.
> >>>>>
> >>>>> I was thinking of just adding a property to the Sensor.Value interface
> >>>>> to report information.  Basically, someone reading the value needs to
> >>>>> know if it's valid.  The idea of a range check for validity doesn't
> >>>>> work as cleanly for this specific purpose, in my opinion.  The min/max
> >>>>> can be used cleanly to determine the linearization though!
> >>>>>
> >>>>> I was thinking perhaps a boolean, that you can check to see if the
> >>>>> value should be used or trusted.  Then, I thought, perhaps, a enum
> >>>>> with a series of states, starting with the states of "Valid',
> >>>>> "Invalid."  Not being able to think of a third state, I fell back onto
> >>>>> a boolean.
> >>
> >> If the hwmon driver for your device provides a _fault attribute, and
> >> that gets set, the code is already
> >> set up to create an OperationalStatus interface on that sensor instance
> >> and set the Functional
> >> property to false.
> >
> > I'd like to drop this such that the failure behavior can be returning
> > a NaN double.
> > https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127
> >
> > For the drivers that don't have the fault attribute, and in the case
> > where phosphor-host-ipmid doesn't check for that kind of failure.
> >
> IMHO, we should handle this by removing the sensor from dbus instead of
> using a special value type if we're unable to use the OperationalStatus.
> In either case, the code utilizing this sensor would need to
> specifically handle this NaN value or handle having the sensor removed
> from dbus. It seems more generically, to me, to have hwmon remove the
> sensor from dbus instead of using a NaN double so this can be used
> across other sensor drivers that may not return a double type.

In our infrastructure if we removed the sensor from dbus we'd have to
report an invalid reading but not a sensor missing reading.  So, to
us, if it's NaN or not on dbus we have to treat it the same way
basically, so fewer code changes to support it is more or less the
goal.  Although right now phosphor-host-ipmid isn't set up nicely to
deal with NaN(). I have a downstream change that I needed to introduce
so that it excepts on that specific type of failure and then returns
invalid reading (or something akin to that).

> >>
> >>>> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
> >>>> is in an invalid state (like power off for tach values). Although I'm
> >>>> not sure how this would be produced using the int64 interface, for the
> >>>> double std::nan will go over d-bus correctly.
> >>> So if i just wait until we're using double, the issue basically
> >>> resolves itself? :D :D
> >>>
> >>>>> Thoughts?
> >>>>>
> >>>>> Patrick
> >>>>>
> >>
> >
>

Re: Notion of Flakiness in Sensor Readings

[Brad Bishop]

> On Oct 12, 2018, at 5:32 PM, Patrick Venture <venture@google.com> wrote:
> 
> On Fri, Oct 12, 2018 at 2:28 PM Matt Spinler
> <mspinler@linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>> On 10/12/2018 4:23 PM, Patrick Venture wrote:
>>> On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
>>>> On 10/12/2018 01:53 PM, Patrick Venture wrote:
>>>>> Currently, there are a few approaches in phosphor-hwmon on how to
>>>>> handle sensors being present sometimes, or flaky on reads.  We've
>>>>> talked in the past about how to handle a read failure that should be
>>>>> ignored.  A real failure that is flaky or temporary.
>>>>> 
>>>>> I was thinking of just adding a property to the Sensor.Value interface
>>>>> to report information.  Basically, someone reading the value needs to
>>>>> know if it's valid.  The idea of a range check for validity doesn't
>>>>> work as cleanly for this specific purpose, in my opinion.  The min/max
>>>>> can be used cleanly to determine the linearization though!
>>>>> 
>>>>> I was thinking perhaps a boolean, that you can check to see if the
>>>>> value should be used or trusted.  Then, I thought, perhaps, a enum
>>>>> with a series of states, starting with the states of "Valid',
>>>>> "Invalid."  Not being able to think of a third state, I fell back onto
>>>>> a boolean.
>> 
>> If the hwmon driver for your device provides a _fault attribute, and
>> that gets set, the code is already
>> set up to create an OperationalStatus interface on that sensor instance
>> and set the Functional
>> property to false.
> 
> I'd like to drop this such that the failure behavior can be returning
> a NaN double.

One benefit I see here is that everything is in the same DBus interface
(the same property even), but with OperationalStatus you have two and
thus more DBus calls, if you are polling?  Is that it or are there others?

> https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127
> 
> For the drivers that don't have the fault attribute,

It seems like OperationalStatus could be extended to emulate easily here.

> and in the case where phosphor-host-ipmid doesn't check for that kind of
> failure.

So is there no value in consistency of interface?  A lot of things
can be ‘not working’ on a BMC besides sensors - do you want to model that
in the DBus API differently for each domain (which seems like the direction
you are headed)?

Further this model falls apart if you ever want something besides _working_
and _not_working_ for possible states.  I realize YAGNI, but 1 - I have seen that
in the past, 2 - it seems trivial to implement, and 3 - the down-side of
OperationalStatus is not apparent to me.

> 
>> 
>>>> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
>>>> is in an invalid state (like power off for tach values). Although I'm
>>>> not sure how this would be produced using the int64 interface, for the
>>>> double std::nan will go over d-bus correctly.
>>> So if i just wait until we're using double, the issue basically
>>> resolves itself? :D :D
>>> 
>>>>> Thoughts?
>>>>> 
>>>>> Patrick

Re: Notion of Flakiness in Sensor Readings

[Patrick Venture]

On Mon, Oct 15, 2018 at 6:14 PM Brad Bishop <bradleyb@fuzziesquirrel.com> wrote:
>
>
>
> > On Oct 12, 2018, at 5:32 PM, Patrick Venture <venture@google.com> wrote:
> >
> > On Fri, Oct 12, 2018 at 2:28 PM Matt Spinler
> > <mspinler@linux.vnet.ibm.com> wrote:
> >>
> >>
> >>
> >> On 10/12/2018 4:23 PM, Patrick Venture wrote:
> >>> On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
> >>>> On 10/12/2018 01:53 PM, Patrick Venture wrote:
> >>>>> Currently, there are a few approaches in phosphor-hwmon on how to
> >>>>> handle sensors being present sometimes, or flaky on reads.  We've
> >>>>> talked in the past about how to handle a read failure that should be
> >>>>> ignored.  A real failure that is flaky or temporary.
> >>>>>
> >>>>> I was thinking of just adding a property to the Sensor.Value interface
> >>>>> to report information.  Basically, someone reading the value needs to
> >>>>> know if it's valid.  The idea of a range check for validity doesn't
> >>>>> work as cleanly for this specific purpose, in my opinion.  The min/max
> >>>>> can be used cleanly to determine the linearization though!
> >>>>>
> >>>>> I was thinking perhaps a boolean, that you can check to see if the
> >>>>> value should be used or trusted.  Then, I thought, perhaps, a enum
> >>>>> with a series of states, starting with the states of "Valid',
> >>>>> "Invalid."  Not being able to think of a third state, I fell back onto
> >>>>> a boolean.
> >>
> >> If the hwmon driver for your device provides a _fault attribute, and
> >> that gets set, the code is already
> >> set up to create an OperationalStatus interface on that sensor instance
> >> and set the Functional
> >> property to false.
> >
> > I'd like to drop this such that the failure behavior can be returning
> > a NaN double.
>
> One benefit I see here is that everything is in the same DBus interface
> (the same property even), but with OperationalStatus you have two and
> thus more DBus calls, if you are polling?  Is that it or are there others?
>
> > https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127
> >
> > For the drivers that don't have the fault attribute,
>
> It seems like OperationalStatus could be extended to emulate easily here.
>
> > and in the case where phosphor-host-ipmid doesn't check for that kind of
> > failure.
>
> So is there no value in consistency of interface?  A lot of things
> can be ‘not working’ on a BMC besides sensors - do you want to model that
> in the DBus API differently for each domain (which seems like the direction
> you are headed)?
>
> Further this model falls apart if you ever want something besides _working_
> and _not_working_ for possible states.  I realize YAGNI, but 1 - I have seen that
> in the past, 2 - it seems trivial to implement, and 3 - the down-side of
> OperationalStatus is not apparent to me.

I have no issue with OperationalStatus for this -- I believe Matt's
comment was that it's available gratis if the driver supports it.
Since this driver may not (doesn't, but i haven't checked recently), I
haven't dug back into that use case.

The idea of a sensor falling off and being added to the dbus every
other time the sensor needs to be read falls over for at least one key
reason: 1) You can't read the sensor on demand anymore.

I brought all this up because I want to get away from -errno patches,
and I want the error path to follow the natural state of things.
Reading a dbus double and seeing NaN() may be enough of an error, and
you can check OperationalStatus or other interfaces for more
information if desired.

>
> >
> >>
> >>>> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
> >>>> is in an invalid state (like power off for tach values). Although I'm
> >>>> not sure how this would be produced using the int64 interface, for the
> >>>> double std::nan will go over d-bus correctly.
> >>> So if i just wait until we're using double, the issue basically
> >>> resolves itself? :D :D
> >>>
> >>>>> Thoughts?
> >>>>>
> >>>>> Patrick

Re: Notion of Flakiness in Sensor Readings

[Brad Bishop]

> On Oct 15, 2018, at 10:15 PM, Patrick Venture <venture@google.com> wrote:
> 
> On Mon, Oct 15, 2018 at 6:14 PM Brad Bishop <bradleyb@fuzziesquirrel.com> wrote:
>> 
>> 
>> 
>>> On Oct 12, 2018, at 5:32 PM, Patrick Venture <venture@google.com> wrote:
>>> 
>>> On Fri, Oct 12, 2018 at 2:28 PM Matt Spinler
>>> <mspinler@linux.vnet.ibm.com> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 10/12/2018 4:23 PM, Patrick Venture wrote:
>>>>> On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
>>>>>> On 10/12/2018 01:53 PM, Patrick Venture wrote:
>>>>>>> Currently, there are a few approaches in phosphor-hwmon on how to
>>>>>>> handle sensors being present sometimes, or flaky on reads.  We've
>>>>>>> talked in the past about how to handle a read failure that should be
>>>>>>> ignored.  A real failure that is flaky or temporary.
>>>>>>> 
>>>>>>> I was thinking of just adding a property to the Sensor.Value interface
>>>>>>> to report information.  Basically, someone reading the value needs to
>>>>>>> know if it's valid.  The idea of a range check for validity doesn't
>>>>>>> work as cleanly for this specific purpose, in my opinion.  The min/max
>>>>>>> can be used cleanly to determine the linearization though!
>>>>>>> 
>>>>>>> I was thinking perhaps a boolean, that you can check to see if the
>>>>>>> value should be used or trusted.  Then, I thought, perhaps, a enum
>>>>>>> with a series of states, starting with the states of "Valid',
>>>>>>> "Invalid."  Not being able to think of a third state, I fell back onto
>>>>>>> a boolean.
>>>> 
>>>> If the hwmon driver for your device provides a _fault attribute, and
>>>> that gets set, the code is already
>>>> set up to create an OperationalStatus interface on that sensor instance
>>>> and set the Functional
>>>> property to false.
>>> 
>>> I'd like to drop this such that the failure behavior can be returning
>>> a NaN double.
>> 
>> One benefit I see here is that everything is in the same DBus interface
>> (the same property even), but with OperationalStatus you have two and
>> thus more DBus calls, if you are polling?  Is that it or are there others?
>> 
>>> https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127
>>> 
>>> For the drivers that don't have the fault attribute,
>> 
>> It seems like OperationalStatus could be extended to emulate easily here.
>> 
>>> and in the case where phosphor-host-ipmid doesn't check for that kind of
>>> failure.
>> 
>> So is there no value in consistency of interface?  A lot of things
>> can be ‘not working’ on a BMC besides sensors - do you want to model that
>> in the DBus API differently for each domain (which seems like the direction
>> you are headed)?
>> 
>> Further this model falls apart if you ever want something besides _working_
>> and _not_working_ for possible states.  I realize YAGNI, but 1 - I have seen that
>> in the past, 2 - it seems trivial to implement, and 3 - the down-side of
>> OperationalStatus is not apparent to me.
> 
> I have no issue with OperationalStatus for this -- I believe Matt's
> comment was that it's available gratis if the driver supports it.

Correct.  But I think it could be extended to emulate it for drivers that
don’t - similar to the thresholds.

> Since this driver may not (doesn't, but i haven't checked recently), I
> haven't dug back into that use case.
> 
> The idea of a sensor falling off and being added to the dbus every
> other time the sensor needs to be read falls over for at least one key
> reason: 1) You can't read the sensor on demand anymore.

I agree - I don’t think we want to do this anymore, when we have
options like OperationalStatus or NaN.

> 
> I brought all this up because I want to get away from -errno patches,
> and I want the error path to follow the natural state of things.
> Reading a dbus double and seeing NaN() may be enough of an error, and
> you can check OperationalStatus or other interfaces for more
> information if desired.

That SGTM.  I thought you wanted to drop OperationalStatus completely.

> 
>> 
>>> 
>>>> 
>>>>>> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
>>>>>> is in an invalid state (like power off for tach values). Although I'm
>>>>>> not sure how this would be produced using the int64 interface, for the
>>>>>> double std::nan will go over d-bus correctly.
>>>>> So if i just wait until we're using double, the issue basically
>>>>> resolves itself? :D :D
>>>>> 
>>>>>>> Thoughts?
>>>>>>> 
>>>>>>> Patrick

Re: Notion of Flakiness in Sensor Readings

[Patrick Venture]

On Mon, Oct 15, 2018 at 7:30 PM Brad Bishop <bradleyb@fuzziesquirrel.com> wrote:
>
>
>
> > On Oct 15, 2018, at 10:15 PM, Patrick Venture <venture@google.com> wrote:
> >
> > On Mon, Oct 15, 2018 at 6:14 PM Brad Bishop <bradleyb@fuzziesquirrel.com> wrote:
> >>
> >>
> >>
> >>> On Oct 12, 2018, at 5:32 PM, Patrick Venture <venture@google.com> wrote:
> >>>
> >>> On Fri, Oct 12, 2018 at 2:28 PM Matt Spinler
> >>> <mspinler@linux.vnet.ibm.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 10/12/2018 4:23 PM, Patrick Venture wrote:
> >>>>> On Fri, Oct 12, 2018 at 2:09 PM James Feist <james.feist@linux.intel.com> wrote:
> >>>>>> On 10/12/2018 01:53 PM, Patrick Venture wrote:
> >>>>>>> Currently, there are a few approaches in phosphor-hwmon on how to
> >>>>>>> handle sensors being present sometimes, or flaky on reads.  We've
> >>>>>>> talked in the past about how to handle a read failure that should be
> >>>>>>> ignored.  A real failure that is flaky or temporary.
> >>>>>>>
> >>>>>>> I was thinking of just adding a property to the Sensor.Value interface
> >>>>>>> to report information.  Basically, someone reading the value needs to
> >>>>>>> know if it's valid.  The idea of a range check for validity doesn't
> >>>>>>> work as cleanly for this specific purpose, in my opinion.  The min/max
> >>>>>>> can be used cleanly to determine the linearization though!
> >>>>>>>
> >>>>>>> I was thinking perhaps a boolean, that you can check to see if the
> >>>>>>> value should be used or trusted.  Then, I thought, perhaps, a enum
> >>>>>>> with a series of states, starting with the states of "Valid',
> >>>>>>> "Invalid."  Not being able to think of a third state, I fell back onto
> >>>>>>> a boolean.
> >>>>
> >>>> If the hwmon driver for your device provides a _fault attribute, and
> >>>> that gets set, the code is already
> >>>> set up to create an OperationalStatus interface on that sensor instance
> >>>> and set the Functional
> >>>> property to false.
> >>>
> >>> I'd like to drop this such that the failure behavior can be returning
> >>> a NaN double.
> >>
> >> One benefit I see here is that everything is in the same DBus interface
> >> (the same property even), but with OperationalStatus you have two and
> >> thus more DBus calls, if you are polling?  Is that it or are there others?
> >>
> >>> https://github.com/openbmc/phosphor-hwmon/blob/master/hwmonio.cpp#L127
> >>>
> >>> For the drivers that don't have the fault attribute,
> >>
> >> It seems like OperationalStatus could be extended to emulate easily here.
> >>
> >>> and in the case where phosphor-host-ipmid doesn't check for that kind of
> >>> failure.
> >>
> >> So is there no value in consistency of interface?  A lot of things
> >> can be ‘not working’ on a BMC besides sensors - do you want to model that
> >> in the DBus API differently for each domain (which seems like the direction
> >> you are headed)?
> >>
> >> Further this model falls apart if you ever want something besides _working_
> >> and _not_working_ for possible states.  I realize YAGNI, but 1 - I have seen that
> >> in the past, 2 - it seems trivial to implement, and 3 - the down-side of
> >> OperationalStatus is not apparent to me.
> >
> > I have no issue with OperationalStatus for this -- I believe Matt's
> > comment was that it's available gratis if the driver supports it.
>
> Correct.  But I think it could be extended to emulate it for drivers that
> don’t - similar to the thresholds.
>
> > Since this driver may not (doesn't, but i haven't checked recently), I
> > haven't dug back into that use case.
> >
> > The idea of a sensor falling off and being added to the dbus every
> > other time the sensor needs to be read falls over for at least one key
> > reason: 1) You can't read the sensor on demand anymore.
>
> I agree - I don’t think we want to do this anymore, when we have
> options like OperationalStatus or NaN.
>
> >
> > I brought all this up because I want to get away from -errno patches,
> > and I want the error path to follow the natural state of things.
> > Reading a dbus double and seeing NaN() may be enough of an error, and
> > you can check OperationalStatus or other interfaces for more
> > information if desired.
>
> That SGTM.  I thought you wanted to drop OperationalStatus completely.

No, I definitely want to keep it.  And the idea of emulating it was
sort of the idea going in -- although I hadn't thought of it when I
started this chain - - the but general idea that I can use something
like NaN and get the results for the flakiness situation with few
changes in phosphor-host-ipmid, I'm pretty happy with it.

There may be other types of errors that warrant using OperationStatus,
but that is a second check, so that's something that could be avoided
for some errors.

I think we're all on the same page.

>
> >
> >>
> >>>
> >>>>
> >>>>>> For Valid / Invalid we've produced nan on D-Bus to know that the sensor
> >>>>>> is in an invalid state (like power off for tach values). Although I'm
> >>>>>> not sure how this would be produced using the int64 interface, for the
> >>>>>> double std::nan will go over d-bus correctly.
> >>>>> So if i just wait until we're using double, the issue basically
> >>>>> resolves itself? :D :D
> >>>>>
> >>>>>>> Thoughts?
> >>>>>>>
> >>>>>>> Patrick