Re: hardcoded median function in phosphor-virtual-sensor

[Patrick Williams <patrick@stwcx.xyz>]

[-- 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 --]