Re: hardcoded median function in phosphor-virtual-sensor

[Patrick Williams <patrick@stwcx.xyz>]

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