hardcoded median function in phosphor-virtual-sensor

hardcoded median function in phosphor-virtual-sensor

[Matt Spinler]

Hi,

Just putting on the list what was decided after some lengthy discussions 
on discord.

I need a median of some sensor values, where this median sensor has 
threshold interfaces
whose values must be defined in entity-manager.  Since exprtk 
expressions are not allowed in
entity-manager, I cannot just port the PVS's JSON config into an 
entity-manager config.

Instead, I will make a new entity-manager config that will have the 
component sensors
along  with the thresholds to use, with a subtype of median, vaguely 
something like:

{

Type: "VirtualSensor"

Name: "MySensorName"

Subtype: "Median"

Sensors: [ "Sensor1", "Sensor2", .... ]

ThresholdsWithHysteresis [ ]

minInput: 0

maxInput: 100

}


The minInput/maxInput are needed so we don't use garbage sensor readings 
in the median
algorithm.  PVS will look for this config to be provided on D-Bus by 
entity-manager, and if
it's there it will calculate the median (in C++, not exprtk) and use it 
as the virtual sensor value.

Thanks,
Matt

Re: hardcoded median function in phosphor-virtual-sensor

[Vijay Khemka]

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

On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com> wrote:

> Hi,
>
> Just putting on the list what was decided after some lengthy discussions
> on discord.
>
> I need a median of some sensor values, where this median sensor has
> threshold interfaces
> whose values must be defined in entity-manager.  Since exprtk
> expressions are not allowed in
> entity-manager, I cannot just port the PVS's JSON config into an
> entity-manager config.
>
> I missed this discussion but why can't we simply use virtual sensor as
expertk provides median function and we have threshold support for
each virtual sensor. Please help, if I am missing anything.


> Instead, I will make a new entity-manager config that will have the
> component sensors
> along  with the thresholds to use, with a subtype of median, vaguely
> something like:
>
> {
>
> Type: "VirtualSensor"
>
> Name: "MySensorName"
>
> Subtype: "Median"
>
> Sensors: [ "Sensor1", "Sensor2", .... ]
>
> ThresholdsWithHysteresis [ ]
>
> minInput: 0
>
> maxInput: 100
>
> }
>
>
> The minInput/maxInput are needed so we don't use garbage sensor readings
> in the median
> algorithm.  PVS will look for this config to be provided on D-Bus by
> entity-manager, and if
> it's there it will calculate the median (in C++, not exprtk) and use it
> as the virtual sensor value.
>
> Thanks,
> Matt
>
>

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

Re: hardcoded median function in phosphor-virtual-sensor

[Matt Spinler]

On 1/4/2021 2:54 PM, Vijay Khemka wrote:
> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler ...
> This Message Is From an External Sender
> This message came from outside your organization.
>
>
>
> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com 
> <mailto:mspinler@linux.ibm.com>> wrote:
>
>     Hi,
>
>     Just putting on the list what was decided after some lengthy
>     discussions
>     on discord.
>
>     I need a median of some sensor values, where this median sensor has
>     threshold interfaces
>     whose values must be defined in entity-manager.  Since exprtk
>     expressions are not allowed in
>     entity-manager, I cannot just port the PVS's JSON config into an
>     entity-manager config.
>
> I missed this discussion but why can't we simply use virtual sensor as
> expertk provides median function and we have threshold support for
> each virtual sensor. Please help, if I am missing anything.

If you're asking why can't we just have PVS get its exprtk expression to
use from entity-manager, and encode the median there, it's because Ed
doesn't want exprtk in entity-manager config files (I'll defer to him on 
the reasons).

If you're asking now that the median function is hardcoded, why write it in
C++ instead of exprtk, it's because the exprtk code would be so similar to
C++ code (skip the bad values, put the sensors in a vector,  call 
nth_element)
that writing it in exprtk doesn't really buy us anything, and using C++ lets
me skip making the symbol table.

>     Instead, I will make a new entity-manager config that will have the
>     component sensors
>     along  with the thresholds to use, with a subtype of median, vaguely
>     something like:
>
>     {
>
>     Type: "VirtualSensor"
>
>     Name: "MySensorName"
>
>     Subtype: "Median"
>
>     Sensors: [ "Sensor1", "Sensor2", .... ]
>
>     ThresholdsWithHysteresis [ ]
>
>     minInput: 0
>
>     maxInput: 100
>
>     }
>
>
>     The minInput/maxInput are needed so we don't use garbage sensor
>     readings
>     in the median
>     algorithm.  PVS will look for this config to be provided on D-Bus by
>     entity-manager, and if
>     it's there it will calculate the median (in C++, not exprtk) and
>     use it
>     as the virtual sensor value.
>
>     Thanks,
>     Matt
>

Re: hardcoded median function in phosphor-virtual-sensor

[Lei Yu]

On Tue, Jan 5, 2021 at 6:58 AM Matt Spinler <mspinler@linux.ibm.com> wrote:
> >     whose values must be defined in entity-manager.  Since exprtk
> >     expressions are not allowed in
> >     entity-manager, I cannot just port the PVS's JSON config into an
> >     entity-manager config.
> >
> > I missed this discussion but why can't we simply use virtual sensor as
> > expertk provides median function and we have threshold support for
> > each virtual sensor. Please help, if I am missing anything.

I did not notice the discussion as well.
From my understanding, the exprtk is defined in the json config
(/usr/share/phosphor-virtual-sensor/virtual_sensor_config.json), and
technically we could use any expression that exprtk supports.
e.g. we use max() in our system (see below example), which is not
upstreamed yet but it works well.
    "Expression": "max(T0, T1, T2)"

-- 
BRs,
Lei YU

Re: hardcoded median function in phosphor-virtual-sensor

[Matt Spinler]

On 1/4/2021 8:34 PM, Lei Yu wrote:
> On Tue, Jan 5, 2021 at 6:58 AM Matt Spinler <mspinler@linux.ibm.com> wrote:
>>>      whose values must be defined in entity-manager.  Since exprtk
>>>      expressions are not allowed in
>>>      entity-manager, I cannot just port the PVS's JSON config into an
>>>      entity-manager config.
>>>
>>> I missed this discussion but why can't we simply use virtual sensor as
>>> expertk provides median function and we have threshold support for
>>> each virtual sensor. Please help, if I am missing anything.
> I did not notice the discussion as well.
>  From my understanding, the exprtk is defined in the json config
> (/usr/share/phosphor-virtual-sensor/virtual_sensor_config.json), and
> technically we could use any expression that exprtk supports.
> e.g. we use max() in our system (see below example), which is not
> upstreamed yet but it works well.
>      "Expression": "max(T0, T1, T2)"

Yea, I agree, what you have here is exactly how PVS was designed to be used.
My  goal was to add support to have its config be able to be defined in
the entity manager JSON instead, which is what is driving the work of 
needing
selectable expressions based on the name.

Re: hardcoded median function in phosphor-virtual-sensor

[Patrick Williams]

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

On Mon, Jan 04, 2021 at 04:57:51PM -0600, Matt Spinler wrote:
> On 1/4/2021 2:54 PM, Vijay Khemka wrote:
> > On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com 
> > <mailto:mspinler@linux.ibm.com>> wrote:
> >
> >     I need a median of some sensor values, where this median sensor has
> >     threshold interfaces
> >     whose values must be defined in entity-manager.  Since exprtk
> >     expressions are not allowed in
> >     entity-manager, I cannot just port the PVS's JSON config into an
> >     entity-manager config.
> >
> > I missed this discussion but why can't we simply use virtual sensor as
> > expertk provides median function and we have threshold support for
> > each virtual sensor. Please help, if I am missing anything.
> 
> If you're asking why can't we just have PVS get its exprtk expression to
> use from entity-manager, and encode the median there, it's because Ed
> doesn't want exprtk in entity-manager config files (I'll defer to him on 
> the reasons).

As part of offline discussions on this I said that having a one-off EM
config for median to allow you to make progress is reasonable, but I
don't think having a bunch of one-offs is a viable long-term solution
for phosphor-virtual-sensors.  Basically we're kicking the can down the
road until we have a better understanding of what kinds of EM/PVS
configs we're going to need.

> If you're asking now that the median function is hardcoded, why write it in
> C++ instead of exprtk, it's because the exprtk code would be so similar to
> C++ code (skip the bad values, put the sensors in a vector,  call 
> nth_element)
> that writing it in exprtk doesn't really buy us anything, and using C++ lets
> me skip making the symbol table.

I would certainly prefer we use the exprtk code here.  You should be
able to generate (at runtime) a exprtk expression from the EM config you
specified below.

My rationale is:
    * Ed suggested that a long-term answer might be a few lookup tables
      / transformations from the EM configs to the PVS expressions.  I'm
      not fully bought into this yet but it seems like a reasonable
      direction to explore.

    * You wrote "because the exprtk code would be so similar to the C++
      code", which is why you *should* do it in exprtk.  The rest of the
      PVS code is already this way so why something different and
      require dual maintanence?  If the exprtk-based support code is
      missing some of these features, lets add them because others will
      likely need them as well.

> >     Instead, I will make a new entity-manager config that will have the
> >     component sensors
> >     along  with the thresholds to use, with a subtype of median, vaguely
> >     something like:
> >
> >     {
> >     Type: "VirtualSensor"
> >     Name: "MySensorName"
> >     Subtype: "Median"
> >     Sensors: [ "Sensor1", "Sensor2", .... ]
> >     ThresholdsWithHysteresis [ ]
> >     minInput: 0
> >     maxInput: 100
> >     }
> >

I would expect the 'exprtk' expression from your EM config to be
something like "median(Sensor1, Sensor2...)".  You should be able to
feed this into the existing virtual-sensor constructors and not have to
make the symbol table yourself.

Exprtk doesn't currently support a 'median' operator but it does support
'avg'.  We should contribute this upstream and add as a patch in the
meantime.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: hardcoded median function in phosphor-virtual-sensor

[Matt Spinler]

On 1/5/2021 8:27 AM, Patrick Williams wrote:
> On Mon, Jan 04, 2021 at 04:57:51PM -0600, Matt Spinler wrote:
>> On 1/4/2021 2:54 PM, Vijay Khemka wrote:
>>> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com
>>> <mailto:mspinler@linux.ibm.com>> wrote:
>>>
>>>      I need a median of some sensor values, where this median sensor has
>>>      threshold interfaces
>>>      whose values must be defined in entity-manager.  Since exprtk
>>>      expressions are not allowed in
>>>      entity-manager, I cannot just port the PVS's JSON config into an
>>>      entity-manager config.
>>>
>>> I missed this discussion but why can't we simply use virtual sensor as
>>> expertk provides median function and we have threshold support for
>>> each virtual sensor. Please help, if I am missing anything.
>> If you're asking why can't we just have PVS get its exprtk expression to
>> use from entity-manager, and encode the median there, it's because Ed
>> doesn't want exprtk in entity-manager config files (I'll defer to him on
>> the reasons).
> As part of offline discussions on this I said that having a one-off EM
> config for median to allow you to make progress is reasonable, but I
> don't think having a bunch of one-offs is a viable long-term solution
> for phosphor-virtual-sensors.  Basically we're kicking the can down the
> road until we have a better understanding of what kinds of EM/PVS
> configs we're going to need.
>
>> If you're asking now that the median function is hardcoded, why write it in
>> C++ instead of exprtk, it's because the exprtk code would be so similar to
>> C++ code (skip the bad values, put the sensors in a vector,  call
>> nth_element)
>> that writing it in exprtk doesn't really buy us anything, and using C++ lets
>> me skip making the symbol table.
> I would certainly prefer we use the exprtk code here.  You should be
> able to generate (at runtime) a exprtk expression from the EM config you
> specified below.
>
> My rationale is:
>      * Ed suggested that a long-term answer might be a few lookup tables
>        / transformations from the EM configs to the PVS expressions.  I'm
>        not fully bought into this yet but it seems like a reasonable
>        direction to explore.
>
>      * You wrote "because the exprtk code would be so similar to the C++
>        code", which is why you *should* do it in exprtk.  The rest of the
>        PVS code is already this way so why something different and
>        require dual maintanence?  If the exprtk-based support code is
>        missing some of these features, lets add them because others will
>        likely need them as well.

See below.

>>>      Instead, I will make a new entity-manager config that will have the
>>>      component sensors
>>>      along  with the thresholds to use, with a subtype of median, vaguely
>>>      something like:
>>>
>>>      {
>>>      Type: "VirtualSensor"
>>>      Name: "MySensorName"
>>>      Subtype: "Median"
>>>      Sensors: [ "Sensor1", "Sensor2", .... ]
>>>      ThresholdsWithHysteresis [ ]
>>>      minInput: 0
>>>      maxInput: 100
>>>      }
>>>
> I would expect the 'exprtk' expression from your EM config to be
> something like "median(Sensor1, Sensor2...)".  You should be able to
> feed this into the existing virtual-sensor constructors and not have to
> make the symbol table yourself.

Every variable used by exprtk needs to be added to the symbol table 
first by the C++.

Also, we need a slightly tweaked median of our 3 ambient temp sensors:
1) throw out values outside of minInput/maxInput
2) if there is an even number, because we threw out one, choose the 
higher value, and
     don't do the average of the 2  that I believe an actual median 
would use.
3) if we threw out all 3 (very unlikely), use NaN as the sensor value.

This is easy to do in C++ using std::nth_element, and basically looks 
the same in
exprtk which is why I suggested just using C++ though I don't really 
care that much either way,
but I don't see how we could upstream this as a true median().  In fact, 
since
the underlying code would just use  nth_element anyway, I'm not even 
sure it would
be accepted and is probably why there isn't already a median().

Since I guess it could be argued this isn't a true median, maybe we 
shouldn't call it
a median, which is fine, but we still need it.

> Exprtk doesn't currently support a 'median' operator but it does support
> 'avg'.  We should contribute this upstream and add as a patch in the
> meantime.
>

Re: hardcoded median function in phosphor-virtual-sensor

[Vijay Khemka]

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

I have a basic question, what are we trying to achieve here.
1. Are we avoiding of using PVS here completely or
2. we don't want to define separate json config for PVS

because in my understanding PVS will work straight forward
if we define expression in PVS config file.

Regards
-Vijay

On Tue, Jan 5, 2021 at 7:58 AM Matt Spinler <mspinler@linux.ibm.com> wrote:

>
>
> On 1/5/2021 8:27 AM, Patrick Williams wrote:
> > On Mon, Jan 04, 2021 at 04:57:51PM -0600, Matt Spinler wrote:
> >> On 1/4/2021 2:54 PM, Vijay Khemka wrote:
> >>> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com
> >>> <mailto:mspinler@linux.ibm.com>> wrote:
> >>>
> >>>      I need a median of some sensor values, where this median sensor
> has
> >>>      threshold interfaces
> >>>      whose values must be defined in entity-manager.  Since exprtk
> >>>      expressions are not allowed in
> >>>      entity-manager, I cannot just port the PVS's JSON config into an
> >>>      entity-manager config.
> >>>
> >>> I missed this discussion but why can't we simply use virtual sensor as
> >>> expertk provides median function and we have threshold support for
> >>> each virtual sensor. Please help, if I am missing anything.
> >> If you're asking why can't we just have PVS get its exprtk expression to
> >> use from entity-manager, and encode the median there, it's because Ed
> >> doesn't want exprtk in entity-manager config files (I'll defer to him on
> >> the reasons).
> > As part of offline discussions on this I said that having a one-off EM
> > config for median to allow you to make progress is reasonable, but I
> > don't think having a bunch of one-offs is a viable long-term solution
> > for phosphor-virtual-sensors.  Basically we're kicking the can down the
> > road until we have a better understanding of what kinds of EM/PVS
> > configs we're going to need.
> >
> >> If you're asking now that the median function is hardcoded, why write
> it in
> >> C++ instead of exprtk, it's because the exprtk code would be so similar
> to
> >> C++ code (skip the bad values, put the sensors in a vector,  call
> >> nth_element)
> >> that writing it in exprtk doesn't really buy us anything, and using C++
> lets
> >> me skip making the symbol table.
> > I would certainly prefer we use the exprtk code here.  You should be
> > able to generate (at runtime) a exprtk expression from the EM config you
> > specified below.
> >
> > My rationale is:
> >      * Ed suggested that a long-term answer might be a few lookup tables
> >        / transformations from the EM configs to the PVS expressions.  I'm
> >        not fully bought into this yet but it seems like a reasonable
> >        direction to explore.
> >
> >      * You wrote "because the exprtk code would be so similar to the C++
> >        code", which is why you *should* do it in exprtk.  The rest of the
> >        PVS code is already this way so why something different and
> >        require dual maintanence?  If the exprtk-based support code is
> >        missing some of these features, lets add them because others will
> >        likely need them as well.
>
> See below.
>
> >>>      Instead, I will make a new entity-manager config that will have
> the
> >>>      component sensors
> >>>      along  with the thresholds to use, with a subtype of median,
> vaguely
> >>>      something like:
> >>>
> >>>      {
> >>>      Type: "VirtualSensor"
> >>>      Name: "MySensorName"
> >>>      Subtype: "Median"
> >>>      Sensors: [ "Sensor1", "Sensor2", .... ]
> >>>      ThresholdsWithHysteresis [ ]
> >>>      minInput: 0
> >>>      maxInput: 100
> >>>      }
> >>>
> > I would expect the 'exprtk' expression from your EM config to be
> > something like "median(Sensor1, Sensor2...)".  You should be able to
> > feed this into the existing virtual-sensor constructors and not have to
> > make the symbol table yourself.
>
> Every variable used by exprtk needs to be added to the symbol table
> first by the C++.
>
> Also, we need a slightly tweaked median of our 3 ambient temp sensors:
> 1) throw out values outside of minInput/maxInput
> 2) if there is an even number, because we threw out one, choose the
> higher value, and
>      don't do the average of the 2  that I believe an actual median
> would use.
> 3) if we threw out all 3 (very unlikely), use NaN as the sensor value.
>
> This is easy to do in C++ using std::nth_element, and basically looks
> the same in
> exprtk which is why I suggested just using C++ though I don't really
> care that much either way,
> but I don't see how we could upstream this as a true median().  In fact,
> since
> the underlying code would just use  nth_element anyway, I'm not even
> sure it would
> be accepted and is probably why there isn't already a median().
>
> Since I guess it could be argued this isn't a true median, maybe we
> shouldn't call it
> a median, which is fine, but we still need it.
>
> > Exprtk doesn't currently support a 'median' operator but it does support
> > 'avg'.  We should contribute this upstream and add as a patch in the
> > meantime.
> >
>
>

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

Re: hardcoded median function in phosphor-virtual-sensor

[Matt Spinler]

On 1/5/2021 11:18 AM, Vijay Khemka wrote:
> I have a basic question, what are we trying to achieve here. 1....
> This Message Is From an External Sender
> This message came from outside your organization.
>
> I have a basic question, what are we trying to achieve here.
> 1. Are we avoiding of using PVS here completely or
> 2. we don't want to define separate json config for PVS
>
> because in my understanding PVS will work straight forward
> if we define expression in PVS config file.
>

My goal was to simply have PVS get its config from entity-manager D-Bus, 
so that when we add support for new systems we just put the config in an 
entity-manager JSON file we need anyway (and also because we support 
multiple systems in the same flash image which may have different sensors).

I actually had this coded and working, but having the expression come 
from entity manager was rejected (see 
https://gerrit.openbmc-project.xyz/c/openbmc/entity-manager/+/38707), 
and so this is the compromise.


> Regards
> -Vijay
>
> On Tue, Jan 5, 2021 at 7:58 AM Matt Spinler <mspinler@linux.ibm.com 
> <mailto:mspinler@linux.ibm.com>> wrote:
>
>
>
>     On 1/5/2021 8:27 AM, Patrick Williams wrote:
>     > On Mon, Jan 04, 2021 at 04:57:51PM -0600, Matt Spinler wrote:
>     >> On 1/4/2021 2:54 PM, Vijay Khemka wrote:
>     >>> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler
>     <mspinler@linux.ibm.com <mailto:mspinler@linux.ibm.com>
>     >>> <mailto:mspinler@linux.ibm.com
>     <mailto:mspinler@linux.ibm.com>>> wrote:
>     >>>
>     >>>      I need a median of some sensor values, where this median
>     sensor has
>     >>>      threshold interfaces
>     >>>      whose values must be defined in entity-manager.  Since exprtk
>     >>>      expressions are not allowed in
>     >>>      entity-manager, I cannot just port the PVS's JSON config
>     into an
>     >>>      entity-manager config.
>     >>>
>     >>> I missed this discussion but why can't we simply use virtual
>     sensor as
>     >>> expertk provides median function and we have threshold support for
>     >>> each virtual sensor. Please help, if I am missing anything.
>     >> If you're asking why can't we just have PVS get its exprtk
>     expression to
>     >> use from entity-manager, and encode the median there, it's
>     because Ed
>     >> doesn't want exprtk in entity-manager config files (I'll defer
>     to him on
>     >> the reasons).
>     > As part of offline discussions on this I said that having a
>     one-off EM
>     > config for median to allow you to make progress is reasonable, but I
>     > don't think having a bunch of one-offs is a viable long-term
>     solution
>     > for phosphor-virtual-sensors.  Basically we're kicking the can
>     down the
>     > road until we have a better understanding of what kinds of EM/PVS
>     > configs we're going to need.
>     >
>     >> If you're asking now that the median function is hardcoded, why
>     write it in
>     >> C++ instead of exprtk, it's because the exprtk code would be so
>     similar to
>     >> C++ code (skip the bad values, put the sensors in a vector,  call
>     >> nth_element)
>     >> that writing it in exprtk doesn't really buy us anything, and
>     using C++ lets
>     >> me skip making the symbol table.
>     > I would certainly prefer we use the exprtk code here. You should be
>     > able to generate (at runtime) a exprtk expression from the EM
>     config you
>     > specified below.
>     >
>     > My rationale is:
>     >      * Ed suggested that a long-term answer might be a few
>     lookup tables
>     >        / transformations from the EM configs to the PVS
>     expressions.  I'm
>     >        not fully bought into this yet but it seems like a reasonable
>     >        direction to explore.
>     >
>     >      * You wrote "because the exprtk code would be so similar to
>     the C++
>     >        code", which is why you *should* do it in exprtk. The
>     rest of the
>     >        PVS code is already this way so why something different and
>     >        require dual maintanence?  If the exprtk-based support
>     code is
>     >        missing some of these features, lets add them because
>     others will
>     >        likely need them as well.
>
>     See below.
>
>     >>>      Instead, I will make a new entity-manager config that
>     will have the
>     >>>      component sensors
>     >>>      along  with the thresholds to use, with a subtype of
>     median, vaguely
>     >>>      something like:
>     >>>
>     >>>      {
>     >>>      Type: "VirtualSensor"
>     >>>      Name: "MySensorName"
>     >>>      Subtype: "Median"
>     >>>      Sensors: [ "Sensor1", "Sensor2", .... ]
>     >>>      ThresholdsWithHysteresis [ ]
>     >>>      minInput: 0
>     >>>      maxInput: 100
>     >>>      }
>     >>>
>     > I would expect the 'exprtk' expression from your EM config to be
>     > something like "median(Sensor1, Sensor2...)".  You should be able to
>     > feed this into the existing virtual-sensor constructors and not
>     have to
>     > make the symbol table yourself.
>
>     Every variable used by exprtk needs to be added to the symbol table
>     first by the C++.
>
>     Also, we need a slightly tweaked median of our 3 ambient temp sensors:
>     1) throw out values outside of minInput/maxInput
>     2) if there is an even number, because we threw out one, choose the
>     higher value, and
>          don't do the average of the 2  that I believe an actual median
>     would use.
>     3) if we threw out all 3 (very unlikely), use NaN as the sensor value.
>
>     This is easy to do in C++ using std::nth_element, and basically looks
>     the same in
>     exprtk which is why I suggested just using C++ though I don't really
>     care that much either way,
>     but I don't see how we could upstream this as a true median().  In
>     fact,
>     since
>     the underlying code would just use  nth_element anyway, I'm not even
>     sure it would
>     be accepted and is probably why there isn't already a median().
>
>     Since I guess it could be argued this isn't a true median, maybe we
>     shouldn't call it
>     a median, which is fine, but we still need it.
>
>     > Exprtk doesn't currently support a 'median' operator but it does
>     support
>     > 'avg'.  We should contribute this upstream and add as a patch in the
>     > meantime.
>     >
>

Re: hardcoded median function in phosphor-virtual-sensor

[Ed Tanous]

On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com> wrote:
>
> Hi,
>
> Just putting on the list what was decided after some lengthy discussions
> on discord.
>
> I need a median of some sensor values, where this median sensor has
> threshold interfaces
> whose values must be defined in entity-manager.  Since exprtk
> expressions are not allowed in
> entity-manager, I cannot just port the PVS's JSON config into an
> entity-manager config.
>
> Instead, I will make a new entity-manager config that will have the
> component sensors
> along  with the thresholds to use, with a subtype of median, vaguely
> something like:
>
> {
>
> Type: "VirtualSensor"

I'm kinda thinking we just go with Type: "MedianSensor".  Technically
there are already other "virtual" sensors that have their own primary
type, so let's be consistent with them, and just declare this as a
primary type: "Median"

>
> Name: "MySensorName"
>
> Subtype: "Median"
>
> Sensors: [ "Sensor1", "Sensor2", .... ]

Just call this "Inputs" to be consistent in naming with the PID type
and stewise type.

>
> ThresholdsWithHysteresis [ ]

Should this just forward on thresholds from the sensors themselves to
reduce the amount of configuration, and to be consistent?  I'm
assuming this could be omitted, and we could just forward on the
median sensors threshold state if we wanted to.

If we really do need it, it should be called "Thresholds" to be
consistent with the other sensors.

>
> minInput: 0
>
> maxInput: 100
>
> }
>
>
> The minInput/maxInput are needed so we don't use garbage sensor readings
> in the median
> algorithm.

Doesn't the median algorithm already give you that behavior without
having to declare a new range?  If a sensor goes out of range, it's
very unlikely to be the median, so pre-gating the sensors just seems
like extra work.  Overall, this kind of config feels like something
each sensor itself should own, as they already have more information
than the median sensor.  If the sensor knows it has "garbage" values,
it should post some form of NAN that the virtual sensor can see and
act on.

What is the behavior if all sensors go out of this range or are invalid?
How will the MinOutput and MaxOutput for the sensor be calculated?  I
would assume based on the max/min of the inputs, but it's probably
worth calling out explicitly.
Do we need to add a PowerState parameter so the thresholds don't trip
on power cycles?


>  PVS will look for this config to be provided on D-Bus by
> entity-manager, and if
> it's there it will calculate the median (in C++, not exprtk) and use it
> as the virtual sensor value.
>
> Thanks,
> Matt
>

Re: hardcoded median function in phosphor-virtual-sensor

[Ed Tanous]

On Mon, Jan 4, 2021 at 12:55 PM Vijay Khemka <vijaykhemkalinux@gmail.com> wrote:
>
>
>
> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> Just putting on the list what was decided after some lengthy discussions
>> on discord.
>>
>> I need a median of some sensor values, where this median sensor has
>> threshold interfaces
>> whose values must be defined in entity-manager.  Since exprtk
>> expressions are not allowed in
>> entity-manager, I cannot just port the PVS's JSON config into an
>> entity-manager config.
>>
> I missed this discussion but why can't we simply use virtual sensor as
> expertk provides median function and we have threshold support for
> each virtual sensor. Please help, if I am missing anything.

We had a lengthy discussion on discord the other day (hooray for
having chat history), and previously on your design review for this
feature.  Those two places are probably the best way to get caught up.

>
>>
>> Instead, I will make a new entity-manager config that will have the
>> component sensors
>> along  with the thresholds to use, with a subtype of median, vaguely
>> something like:
>>
>> {
>>
>> Type: "VirtualSensor"
>>
>> Name: "MySensorName"
>>
>> Subtype: "Median"
>>
>> Sensors: [ "Sensor1", "Sensor2", .... ]
>>
>> ThresholdsWithHysteresis [ ]
>>
>> minInput: 0
>>
>> maxInput: 100
>>
>> }
>>
>>
>> The minInput/maxInput are needed so we don't use garbage sensor readings
>> in the median
>> algorithm.  PVS will look for this config to be provided on D-Bus by
>> entity-manager, and if
>> it's there it will calculate the median (in C++, not exprtk) and use it
>> as the virtual sensor value.
>>
>> Thanks,
>> Matt
>>

Re: hardcoded median function in phosphor-virtual-sensor

[Ed Tanous]

On Tue, Jan 5, 2021 at 7:59 AM Matt Spinler <mspinler@linux.ibm.com> wrote:
>
>
>
> On 1/5/2021 8:27 AM, Patrick Williams wrote:
> > On Mon, Jan 04, 2021 at 04:57:51PM -0600, Matt Spinler wrote:
> >> On 1/4/2021 2:54 PM, Vijay Khemka wrote:
> >>> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com
> >>> <mailto:mspinler@linux.ibm.com>> wrote:
> >>>
> >>>      I need a median of some sensor values, where this median sensor has
> >>>      threshold interfaces
> >>>      whose values must be defined in entity-manager.  Since exprtk
> >>>      expressions are not allowed in
> >>>      entity-manager, I cannot just port the PVS's JSON config into an
> >>>      entity-manager config.
> >>>
> >>> I missed this discussion but why can't we simply use virtual sensor as
> >>> expertk provides median function and we have threshold support for
> >>> each virtual sensor. Please help, if I am missing anything.
> >> If you're asking why can't we just have PVS get its exprtk expression to
> >> use from entity-manager, and encode the median there, it's because Ed
> >> doesn't want exprtk in entity-manager config files (I'll defer to him on
> >> the reasons).
> > As part of offline discussions on this I said that having a one-off EM
> > config for median to allow you to make progress is reasonable, but I
> > don't think having a bunch of one-offs is a viable long-term solution
> > for phosphor-virtual-sensors.  Basically we're kicking the can down the
> > road until we have a better understanding of what kinds of EM/PVS
> > configs we're going to need.
> >
> >> If you're asking now that the median function is hardcoded, why write it in
> >> C++ instead of exprtk, it's because the exprtk code would be so similar to
> >> C++ code (skip the bad values, put the sensors in a vector,  call
> >> nth_element)
> >> that writing it in exprtk doesn't really buy us anything, and using C++ lets
> >> me skip making the symbol table.
> > I would certainly prefer we use the exprtk code here.  You should be
> > able to generate (at runtime) a exprtk expression from the EM config you
> > specified below.
> >
> > My rationale is:
> >      * Ed suggested that a long-term answer might be a few lookup tables
> >        / transformations from the EM configs to the PVS expressions.  I'm
> >        not fully bought into this yet but it seems like a reasonable
> >        direction to explore.
> >
> >      * You wrote "because the exprtk code would be so similar to the C++
> >        code", which is why you *should* do it in exprtk.  The rest of the
> >        PVS code is already this way so why something different and
> >        require dual maintanence?  If the exprtk-based support code is
> >        missing some of these features, lets add them because others will
> >        likely need them as well.
>
> See below.
>
> >>>      Instead, I will make a new entity-manager config that will have the
> >>>      component sensors
> >>>      along  with the thresholds to use, with a subtype of median, vaguely
> >>>      something like:
> >>>
> >>>      {
> >>>      Type: "VirtualSensor"
> >>>      Name: "MySensorName"
> >>>      Subtype: "Median"
> >>>      Sensors: [ "Sensor1", "Sensor2", .... ]
> >>>      ThresholdsWithHysteresis [ ]
> >>>      minInput: 0
> >>>      maxInput: 100
> >>>      }
> >>>
> > I would expect the 'exprtk' expression from your EM config to be
> > something like "median(Sensor1, Sensor2...)".  You should be able to
> > feed this into the existing virtual-sensor constructors and not have to
> > make the symbol table yourself.
>
> Every variable used by exprtk needs to be added to the symbol table
> first by the C++.
>
> Also, we need a slightly tweaked median of our 3 ambient temp sensors:
> 1) throw out values outside of minInput/maxInput
> 2) if there is an even number, because we threw out one, choose the
> higher value, and
>      don't do the average of the 2  that I believe an actual median
> would use.
> 3) if we threw out all 3 (very unlikely), use NaN as the sensor value.
>

These kinds of corner cases are exactly why IMO C++ is easier in the
long run.  Those 3 conditions are trivial to add to a C++ based
daemon, but would require a lot of complex expertk code to define if
the corner cases were found later.  In C++ they just end up as an
extra branch.

> This is easy to do in C++ using std::nth_element, and basically looks
> the same in
> exprtk which is why I suggested just using C++ though I don't really
> care that much either way,
> but I don't see how we could upstream this as a true median().  In fact,
> since
> the underlying code would just use  nth_element anyway, I'm not even
> sure it would
> be accepted and is probably why there isn't already a median().
>
> Since I guess it could be argued this isn't a true median, maybe we
> shouldn't call it
> a median, which is fine, but we still need it.

Maybe we could call it "AirIntakeTempAggregator" or something of that sort?

>
> > Exprtk doesn't currently support a 'median' operator but it does support
> > 'avg'.  We should contribute this upstream and add as a patch in the
> > meantime.
> >
>

Re: hardcoded median function in phosphor-virtual-sensor

[Patrick Williams]

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

On Tue, Jan 05, 2021 at 09:56:34AM -0600, Matt Spinler wrote:
> 
> 
> On 1/5/2021 8:27 AM, Patrick Williams wrote:
> > On Mon, Jan 04, 2021 at 04:57:51PM -0600, Matt Spinler wrote:
> >> On 1/4/2021 2:54 PM, Vijay Khemka wrote:
> >>> On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com

> > I would expect the 'exprtk' expression from your EM config to be
> > something like "median(Sensor1, Sensor2...)".  You should be able to
> > feed this into the existing virtual-sensor constructors and not have to
> > make the symbol table yourself.
> 
> Every variable used by exprtk needs to be added to the symbol table 
> first by the C++.

Isn't that what this code does already?

https://github.com/openbmc/phosphor-virtual-sensor/blob/26edaad467a44ff9b69968ac0912aa3e9e3d0a62/virtualSensor.cpp#L132

Maybe I was imprecise when I said "exprtk expression".  I really meant
"transform from EM config to the JSON config format PVS already uses and
reuse the existing code".

> Also, we need a slightly tweaked median of our 3 ambient temp sensors:
> 1) throw out values outside of minInput/maxInput
> 2) if there is an even number, because we threw out one, choose the 
> higher value, and
>      don't do the average of the 2  that I believe an actual median 
> would use.
> 3) if we threw out all 3 (very unlikely), use NaN as the sensor value.
> 
> This is easy to do in C++ using std::nth_element, and basically looks 
> the same in
> exprtk which is why I suggested just using C++ though I don't really 
> care that much either way,
> but I don't see how we could upstream this as a true median().  In fact, 
> since
> the underlying code would just use  nth_element anyway, I'm not even 
> sure it would
> be accepted and is probably why there isn't already a median().

I think median would have been accepted but, yes, this isn't a median.

Why do we ever have sensors values outside acceptable real world values?
Can you use the 'clamp' methods to keep your values inside an acceptable
real world value?

> Since I guess it could be argued this isn't a true median, maybe we 
> shouldn't call it
> a median, which is fine, but we still need it.

Yes, we need to stop trying to reuse well-understood names for something
that is not the well-understood meaning.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: hardcoded median function in phosphor-virtual-sensor

[Patrick Williams]

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

On Tue, Jan 05, 2021 at 09:38:12AM -0800, Ed Tanous wrote:
> On Tue, Jan 5, 2021 at 7:59 AM Matt Spinler <mspinler@linux.ibm.com> wrote:
> >
> > Also, we need a slightly tweaked median of our 3 ambient temp sensors:
> > 1) throw out values outside of minInput/maxInput
> > 2) if there is an even number, because we threw out one, choose the
> > higher value, and
> >      don't do the average of the 2  that I believe an actual median
> > would use.
> > 3) if we threw out all 3 (very unlikely), use NaN as the sensor value.
> >
> 
> These kinds of corner cases are exactly why IMO C++ is easier in the
> long run.  Those 3 conditions are trivial to add to a C++ based
> daemon, but would require a lot of complex expertk code to define if
> the corner cases were found later.  In C++ they just end up as an
> extra branch.

I would agree.  If these are the kinds of problems that we end up
solving with PVS, exprtk is not appropriate.  The initial problem
attempting to be solved was "I have a small set of sensors that I need
to do some relatively simple math on".  The moment your "virtual sensor"
has an if-condition, exprtk is probably not the right screwdriver to
hammer with.

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]