Re: hardcoded median function in phosphor-virtual-sensor

[Matt Spinler <mspinler@linux.ibm.com>]

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