hardcoded median function in phosphor-virtual-sensor

[Vijay Khemka]

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 at 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 at linux.ibm.com
> >>> <mailto:mspinler at 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.
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20210105/00ed375d/attachment.htm>