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