* hardcoded median function in phosphor-virtual-sensor @ 2021-01-04 17:48 Matt Spinler 2021-01-04 20:54 ` Vijay Khemka 2021-01-05 17:30 ` Ed Tanous 0 siblings, 2 replies; 14+ messages in thread From: Matt Spinler @ 2021-01-04 17:48 UTC (permalink / raw) To: OpenBMC Maillist Hi, Just putting on the list what was decided after some lengthy discussions on discord. 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. 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 } The minInput/maxInput are needed so we don't use garbage sensor readings in the median algorithm. PVS will look for this config to be provided on D-Bus by entity-manager, and if it's there it will calculate the median (in C++, not exprtk) and use it as the virtual sensor value. Thanks, Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-04 17:48 hardcoded median function in phosphor-virtual-sensor Matt Spinler @ 2021-01-04 20:54 ` Vijay Khemka 2021-01-04 22:57 ` Matt Spinler 2021-01-05 17:31 ` Ed Tanous 2021-01-05 17:30 ` Ed Tanous 1 sibling, 2 replies; 14+ messages in thread From: Vijay Khemka @ 2021-01-04 20:54 UTC (permalink / raw) To: Matt Spinler; +Cc: OpenBMC Maillist [-- Attachment #1: Type: text/plain, Size: 1356 bytes --] On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com> wrote: > Hi, > > Just putting on the list what was decided after some lengthy discussions > on discord. > > 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. > 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 > > } > > > The minInput/maxInput are needed so we don't use garbage sensor readings > in the median > algorithm. PVS will look for this config to be provided on D-Bus by > entity-manager, and if > it's there it will calculate the median (in C++, not exprtk) and use it > as the virtual sensor value. > > Thanks, > Matt > > [-- Attachment #2: Type: text/html, Size: 2072 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-04 20:54 ` Vijay Khemka @ 2021-01-04 22:57 ` Matt Spinler 2021-01-05 2:34 ` Lei Yu 2021-01-05 14:27 ` Patrick Williams 2021-01-05 17:31 ` Ed Tanous 1 sibling, 2 replies; 14+ messages in thread From: Matt Spinler @ 2021-01-04 22:57 UTC (permalink / raw) To: Vijay Khemka; +Cc: OpenBMC Maillist On 1/4/2021 2:54 PM, Vijay Khemka wrote: > On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler ... > This Message Is From an External Sender > This message came from outside your organization. > > > > On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com > <mailto:mspinler@linux.ibm.com>> wrote: > > Hi, > > Just putting on the list what was decided after some lengthy > discussions > on discord. > > 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). 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. > 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 > > } > > > The minInput/maxInput are needed so we don't use garbage sensor > readings > in the median > algorithm. PVS will look for this config to be provided on D-Bus by > entity-manager, and if > it's there it will calculate the median (in C++, not exprtk) and > use it > as the virtual sensor value. > > Thanks, > Matt > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-04 22:57 ` Matt Spinler @ 2021-01-05 2:34 ` Lei Yu 2021-01-05 14:18 ` Matt Spinler 2021-01-05 14:27 ` Patrick Williams 1 sibling, 1 reply; 14+ messages in thread From: Lei Yu @ 2021-01-05 2:34 UTC (permalink / raw) To: Matt Spinler; +Cc: OpenBMC Maillist, Vijay Khemka On Tue, Jan 5, 2021 at 6:58 AM Matt Spinler <mspinler@linux.ibm.com> wrote: > > 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. I did not notice the discussion as well. From my understanding, the exprtk is defined in the json config (/usr/share/phosphor-virtual-sensor/virtual_sensor_config.json), and technically we could use any expression that exprtk supports. e.g. we use max() in our system (see below example), which is not upstreamed yet but it works well. "Expression": "max(T0, T1, T2)" -- BRs, Lei YU ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-05 2:34 ` Lei Yu @ 2021-01-05 14:18 ` Matt Spinler 0 siblings, 0 replies; 14+ messages in thread From: Matt Spinler @ 2021-01-05 14:18 UTC (permalink / raw) To: Lei Yu; +Cc: OpenBMC Maillist, Vijay Khemka On 1/4/2021 8:34 PM, Lei Yu wrote: > On Tue, Jan 5, 2021 at 6:58 AM Matt Spinler <mspinler@linux.ibm.com> wrote: >>> 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. > I did not notice the discussion as well. > From my understanding, the exprtk is defined in the json config > (/usr/share/phosphor-virtual-sensor/virtual_sensor_config.json), and > technically we could use any expression that exprtk supports. > e.g. we use max() in our system (see below example), which is not > upstreamed yet but it works well. > "Expression": "max(T0, T1, T2)" Yea, I agree, what you have here is exactly how PVS was designed to be used. My goal was to add support to have its config be able to be defined in the entity manager JSON instead, which is what is driving the work of needing selectable expressions based on the name. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-04 22:57 ` Matt Spinler 2021-01-05 2:34 ` Lei Yu @ 2021-01-05 14:27 ` Patrick Williams 2021-01-05 15:56 ` Matt Spinler 1 sibling, 1 reply; 14+ messages in thread From: Patrick Williams @ 2021-01-05 14:27 UTC (permalink / raw) To: Matt Spinler; +Cc: OpenBMC Maillist, Vijay Khemka [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-05 14:27 ` Patrick Williams @ 2021-01-05 15:56 ` Matt Spinler 2021-01-05 17:18 ` Vijay Khemka ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Matt Spinler @ 2021-01-05 15:56 UTC (permalink / raw) To: Patrick Williams; +Cc: OpenBMC Maillist, Vijay Khemka 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>> 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. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-05 15:56 ` Matt Spinler @ 2021-01-05 17:18 ` Vijay Khemka 2021-01-05 17:28 ` Matt Spinler 2021-01-05 17:38 ` Ed Tanous 2021-01-05 20:20 ` Patrick Williams 2 siblings, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2021-01-05 17:18 UTC (permalink / raw) To: Matt Spinler; +Cc: OpenBMC Maillist [-- Attachment #1: Type: text/plain, Size: 5085 bytes --] 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@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>> 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. > > > > [-- Attachment #2: Type: text/html, Size: 6618 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-05 17:18 ` Vijay Khemka @ 2021-01-05 17:28 ` Matt Spinler 0 siblings, 0 replies; 14+ messages in thread From: Matt Spinler @ 2021-01-05 17:28 UTC (permalink / raw) To: Vijay Khemka; +Cc: OpenBMC Maillist 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. > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-05 15:56 ` Matt Spinler 2021-01-05 17:18 ` Vijay Khemka @ 2021-01-05 17:38 ` Ed Tanous 2021-01-05 20:23 ` Patrick Williams 2021-01-05 20:20 ` Patrick Williams 2 siblings, 1 reply; 14+ messages in thread From: Ed Tanous @ 2021-01-05 17:38 UTC (permalink / raw) To: Matt Spinler; +Cc: OpenBMC Maillist, Vijay Khemka On Tue, Jan 5, 2021 at 7:59 AM Matt Spinler <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>> 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. > These kinds of corner cases are exactly why IMO C++ is easier in the long run. Those 3 conditions are trivial to add to a C++ based daemon, but would require a lot of complex expertk code to define if the corner cases were found later. In C++ they just end up as an extra branch. > 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. Maybe we could call it "AirIntakeTempAggregator" or something of that sort? > > > 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. > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-05 17:38 ` Ed Tanous @ 2021-01-05 20:23 ` Patrick Williams 0 siblings, 0 replies; 14+ messages in thread From: Patrick Williams @ 2021-01-05 20:23 UTC (permalink / raw) To: Ed Tanous; +Cc: OpenBMC Maillist, Matt Spinler, Vijay Khemka [-- Attachment #1: Type: text/plain, Size: 1232 bytes --] On Tue, Jan 05, 2021 at 09:38:12AM -0800, Ed Tanous wrote: > On Tue, Jan 5, 2021 at 7:59 AM Matt Spinler <mspinler@linux.ibm.com> wrote: > > > > 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. > > > > These kinds of corner cases are exactly why IMO C++ is easier in the > long run. Those 3 conditions are trivial to add to a C++ based > daemon, but would require a lot of complex expertk code to define if > the corner cases were found later. In C++ they just end up as an > extra branch. I would agree. If these are the kinds of problems that we end up solving with PVS, exprtk is not appropriate. The initial problem attempting to be solved was "I have a small set of sensors that I need to do some relatively simple math on". The moment your "virtual sensor" has an if-condition, exprtk is probably not the right screwdriver to hammer with. -- Patrick Williams [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-05 15:56 ` Matt Spinler 2021-01-05 17:18 ` Vijay Khemka 2021-01-05 17:38 ` Ed Tanous @ 2021-01-05 20:20 ` Patrick Williams 2 siblings, 0 replies; 14+ messages in thread From: Patrick Williams @ 2021-01-05 20:20 UTC (permalink / raw) To: Matt Spinler; +Cc: OpenBMC Maillist, Vijay Khemka [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-04 20:54 ` Vijay Khemka 2021-01-04 22:57 ` Matt Spinler @ 2021-01-05 17:31 ` Ed Tanous 1 sibling, 0 replies; 14+ messages in thread From: Ed Tanous @ 2021-01-05 17:31 UTC (permalink / raw) To: Vijay Khemka; +Cc: OpenBMC Maillist, Matt Spinler On Mon, Jan 4, 2021 at 12:55 PM Vijay Khemka <vijaykhemkalinux@gmail.com> wrote: > > > > On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com> wrote: >> >> Hi, >> >> Just putting on the list what was decided after some lengthy discussions >> on discord. >> >> 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. We had a lengthy discussion on discord the other day (hooray for having chat history), and previously on your design review for this feature. Those two places are probably the best way to get caught up. > >> >> 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 >> >> } >> >> >> The minInput/maxInput are needed so we don't use garbage sensor readings >> in the median >> algorithm. PVS will look for this config to be provided on D-Bus by >> entity-manager, and if >> it's there it will calculate the median (in C++, not exprtk) and use it >> as the virtual sensor value. >> >> Thanks, >> Matt >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hardcoded median function in phosphor-virtual-sensor 2021-01-04 17:48 hardcoded median function in phosphor-virtual-sensor Matt Spinler 2021-01-04 20:54 ` Vijay Khemka @ 2021-01-05 17:30 ` Ed Tanous 1 sibling, 0 replies; 14+ messages in thread From: Ed Tanous @ 2021-01-05 17:30 UTC (permalink / raw) To: Matt Spinler; +Cc: OpenBMC Maillist On Mon, Jan 4, 2021 at 9:49 AM Matt Spinler <mspinler@linux.ibm.com> wrote: > > Hi, > > Just putting on the list what was decided after some lengthy discussions > on discord. > > 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. > > 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" I'm kinda thinking we just go with Type: "MedianSensor". Technically there are already other "virtual" sensors that have their own primary type, so let's be consistent with them, and just declare this as a primary type: "Median" > > Name: "MySensorName" > > Subtype: "Median" > > Sensors: [ "Sensor1", "Sensor2", .... ] Just call this "Inputs" to be consistent in naming with the PID type and stewise type. > > ThresholdsWithHysteresis [ ] Should this just forward on thresholds from the sensors themselves to reduce the amount of configuration, and to be consistent? I'm assuming this could be omitted, and we could just forward on the median sensors threshold state if we wanted to. If we really do need it, it should be called "Thresholds" to be consistent with the other sensors. > > minInput: 0 > > maxInput: 100 > > } > > > The minInput/maxInput are needed so we don't use garbage sensor readings > in the median > algorithm. Doesn't the median algorithm already give you that behavior without having to declare a new range? If a sensor goes out of range, it's very unlikely to be the median, so pre-gating the sensors just seems like extra work. Overall, this kind of config feels like something each sensor itself should own, as they already have more information than the median sensor. If the sensor knows it has "garbage" values, it should post some form of NAN that the virtual sensor can see and act on. What is the behavior if all sensors go out of this range or are invalid? How will the MinOutput and MaxOutput for the sensor be calculated? I would assume based on the max/min of the inputs, but it's probably worth calling out explicitly. Do we need to add a PowerState parameter so the thresholds don't trip on power cycles? > PVS will look for this config to be provided on D-Bus by > entity-manager, and if > it's there it will calculate the median (in C++, not exprtk) and use it > as the virtual sensor value. > > Thanks, > Matt > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-01-05 20:25 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-04 17:48 hardcoded median function in phosphor-virtual-sensor Matt Spinler 2021-01-04 20:54 ` Vijay Khemka 2021-01-04 22:57 ` Matt Spinler 2021-01-05 2:34 ` Lei Yu 2021-01-05 14:18 ` Matt Spinler 2021-01-05 14:27 ` Patrick Williams 2021-01-05 15:56 ` Matt Spinler 2021-01-05 17:18 ` Vijay Khemka 2021-01-05 17:28 ` Matt Spinler 2021-01-05 17:38 ` Ed Tanous 2021-01-05 20:23 ` Patrick Williams 2021-01-05 20:20 ` Patrick Williams 2021-01-05 17:31 ` Ed Tanous 2021-01-05 17:30 ` Ed Tanous
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.