[phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Harvey Wu (吳崇維)]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

Hi All,

        The current phosphor-virtual-sensor DbusParam value is from DBus “Value” property which interface is “xyz.openbmc_project.Sensor.Value”.
Because I want to get the property “CriticalHigh” value on DBus, could I add a new feature “Interface” property in json file? The property can be “CriticalHigh” , “CriticalLow” , “WarningHigh” or “WarningLow”.

The new setting in json likes below.
CPU_TEMP will get the property “Value” in /xyz/openbmc_project/sensors/temperature/cpu, and CPU_CriticalHigh will get the property “CriticalHigh” in /xyz/openbmc_project/sensors/temperature/cpu.

"DbusParam" :
[
        {
                "ParamName" : "CPU_TEMP",
                "Desc" :
                {
                        "Name" : "cpu",
                        "SensorType" : "temperature"
                }
        },
        {
                "ParamName" : "CPU_CriticalHigh",
                "Desc" :
                {
                        "Name" : "cpu",
                        "SensorType" : " temperature ",
                        “Interface”: “CriticalHigh”
                }
        }
]

Thanks.

Harvey Wu


[-- Attachment #2: Type: text/html, Size: 6559 bytes --]

Re: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Lei Yu]

On Wed, Dec 30, 2020 at 10:19 AM Harvey Wu (吳崇維) <Harvey.Wu@quantatw.com> wrote:
>
> Hi All,
>
>
>
>         The current phosphor-virtual-sensor DbusParam value is from DBus “Value” property which interface is “xyz.openbmc_project.Sensor.Value”.
>
> Because I want to get the property “CriticalHigh” value on DBus, could I add a new feature “Interface” property in json file? The property can be “CriticalHigh” , “CriticalLow” , “WarningHigh” or “WarningLow”.


According to https://github.com/openbmc/phosphor-virtual-sensor/blob/master/virtual_sensor_config.json#L8-L14,
the threshold properties are already supported.

-- 
BRs,
Lei YU

RE: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Harvey Wu (吳崇維)]

> On Wed, Dec 30, 2020 at 10:19 AM Harvey Wu (吳崇維)
> <Harvey.Wu@quantatw.com> wrote:
> >
> > Hi All,
> >
> >
> >
> >         The current phosphor-virtual-sensor DbusParam value is from
> DBus “Value” property which interface is
> “xyz.openbmc_project.Sensor.Value”.
> >
> > Because I want to get the property “CriticalHigh” value on DBus, could I
> add a new feature “Interface” property in json file? The property can be
> “CriticalHigh” , “CriticalLow” , “WarningHigh” or “WarningLow”.
> 
> 
> According to
> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Fopenbmc%2Fphosphor-virtual-sensor%2Fblob%2Fmaster%2Fvirtual_
> sensor_config.json%23L8-L14&amp;data=04%7C01%7CHarvey.Wu%40quantat
> w.com%7C1479ebd5aedd4b8a7c3308d8ac6a4393%7C179b032707fc4973ac73
> 8de7313561b2%7C1%7C0%7C637448919693850853%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C1000&amp;sdata=0KyrnRkZKr9M59DEg%2BnyUruiege%2B3EDXb
> mxzR%2B6xeng%3D&amp;reserved=0,
> the threshold properties are already supported.

The "Threshold" in json seems only can set the sensor critical or warning value, but I want to use the critical or warning value in "Expression".
After check the newest commit, this function seems doesn't implement in current source code.

> 
> --
> BRs,
> Lei YU


Harvey Wu

Re: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Vijay Khemka]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

I am not understanding why you want to use threshold values to calculate
new sensor values. We can certainly add this feature but it can not be
based on name rather we have to think differently to get. It would really
help to decide on design once we know it's actual usage. I can't think of
any situation here.

Regards
-Vijay

On Tue, Dec 29, 2020 at 6:49 PM Harvey Wu (吳崇維) <Harvey.Wu@quantatw.com>
wrote:

> > On Wed, Dec 30, 2020 at 10:19 AM Harvey Wu (吳崇維)
> > <Harvey.Wu@quantatw.com> wrote:
> > >
> > > Hi All,
> > >
> > >
> > >
> > >         The current phosphor-virtual-sensor DbusParam value is from
> > DBus “Value” property which interface is
> > “xyz.openbmc_project.Sensor.Value”.
> > >
> > > Because I want to get the property “CriticalHigh” value on DBus, could
> I
> > add a new feature “Interface” property in json file? The property can be
> > “CriticalHigh” , “CriticalLow” , “WarningHigh” or “WarningLow”.
> >
> >
> > According to
> > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .
> > com%2Fopenbmc%2Fphosphor-virtual-sensor%2Fblob%2Fmaster%2Fvirtual_
> > sensor_config.json%23L8-L14&amp;data=04%7C01%7CHarvey.Wu%40quantat
> > w.com%7C1479ebd5aedd4b8a7c3308d8ac6a4393%7C179b032707fc4973ac73
> > 8de7313561b2%7C1%7C0%7C637448919693850853%7CUnknown%7CTWFpb
> > GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> > Mn0%3D%7C1000&amp;sdata=0KyrnRkZKr9M59DEg%2BnyUruiege%2B3EDXb
> > mxzR%2B6xeng%3D&amp;reserved=0,
> > the threshold properties are already supported.
>
> The "Threshold" in json seems only can set the sensor critical or warning
> value, but I want to use the critical or warning value in "Expression".
> After check the newest commit, this function seems doesn't implement in
> current source code.
>
> >
> > --
> > BRs,
> > Lei YU
>
>
> Harvey Wu
>

[-- Attachment #2: Type: text/html, Size: 2711 bytes --]

RE: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Harvey Wu (吳崇維)]

Hi Vijay,

	Because I want to calculate the different value between CriticalHigh and current sensor value in CPU sensor, and then there are weighting and some const value to multiply or plus the different value.
But the CPU sensor critical high threshold temp is set as TjMax, and the TjMax value will be different according the different CPU process type, not a Const. That's why I want to add this feature.

For example, according the Document below, if the processor is i7-3770K, the TjMax is 105, and if the processor is U7500, the TjMax is 100.
https://www.kernel.org/doc/Documentation/hwmon/coretemp.rst 

If the Expression is "(TjMax - currentTemp) * scalar - Const", then it will calculate different results between these two CPU processor.


Harvey Wu

From: Vijay Khemka <vijaykhemkalinux@gmail.com> 
Sent: Wednesday, December 30, 2020 1:02 PM
To: Harvey Wu (吳崇維) <Harvey.Wu@quantatw.com>
Cc: Lei Yu <yulei.sh@bytedance.com>; openbmc@lists.ozlabs.org
Subject: Re: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

I am not understanding why you want to use threshold values to calculate new sensor values. We can certainly add this feature but it can not be based on name rather we have to think differently to get. It would really help to decide on design once we know it's actual usage. I can't think of any situation here.

Regards
-Vijay

On Tue, Dec 29, 2020 at 6:49 PM Harvey Wu (吳崇維) <mailto:Harvey.Wu@quantatw.com> wrote:
> On Wed, Dec 30, 2020 at 10:19 AM Harvey Wu (吳崇維)
> <mailto:Harvey.Wu@quantatw.com> wrote:
> >
> > Hi All,
> >
> >
> >
> >         The current phosphor-virtual-sensor DbusParam value is from
> DBus “Value” property which interface is
> “xyz.openbmc_project.Sensor.Value”.
> >
> > Because I want to get the property “CriticalHigh” value on DBus, could I
> add a new feature “Interface” property in json file? The property can be
> “CriticalHigh” , “CriticalLow” , “WarningHigh” or “WarningLow”.
> 
> 
> According to
> https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> com%2Fopenbmc%2Fphosphor-virtual-sensor%2Fblob%2Fmaster%2Fvirtual_
> sensor_config.json%23L8-L14&amp;data=04%7C01%7CHarvey.Wu%40quantat
> https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fw.com%2F&data=04%7C01%7CHarvey.Wu%40quantatw.com%7Cf75707a7431e4dca712e08d8ac80182b%7C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637449013448979318%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=kxT3i6moVPggqbY1zJiag18Cp9wGmyk1ctqrO8XlTUc%3D&reserved=0%7C1479ebd5aedd4b8a7c3308d8ac6a4393%7C179b032707fc4973ac73
> 8de7313561b2%7C1%7C0%7C637448919693850853%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C1000&amp;sdata=0KyrnRkZKr9M59DEg%2BnyUruiege%2B3EDXb
> mxzR%2B6xeng%3D&amp;reserved=0,
> the threshold properties are already supported.

The "Threshold" in json seems only can set the sensor critical or warning value, but I want to use the critical or warning value in "Expression".
After check the newest commit, this function seems doesn't implement in current source code.

> 
> --
> BRs,
> Lei YU


Harvey Wu

Re: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Vijay Khemka]

[-- Attachment #1: Type: text/plain, Size: 4128 bytes --]

On Tue, Dec 29, 2020 at 10:13 PM Harvey Wu (吳崇維) <Harvey.Wu@quantatw.com>
wrote:

> Hi Vijay,
>
>         Because I want to calculate the different value between
> CriticalHigh and current sensor value in CPU sensor, and then there are
> weighting and some const value to multiply or plus the different value.
> But the CPU sensor critical high threshold temp is set as TjMax, and the
> TjMax value will be different according the different CPU process type, not
> a Const. That's why I want to add this feature.
>

Yes I understand that critical values for cpus are received from CPUs and
not a constant values.  But just for this specific sensor we should not add
a new complex feature in a virtual sensor. Because every other sensor has
constant threshold values defined by user so it won't be used anywhere else
other than psu cpu sensor only.

I would rather expose TjMax on dbus if you really want to use this.further.
So exposing tjmax would be easy and straightforward rather than adding this
complex logic for only this sensor in a generic virtual sensor.

>
> For example, according the Document below, if the processor is i7-3770K,
> the TjMax is 105, and if the processor is U7500, the TjMax is 100.
> https://www.kernel.org/doc/Documentation/hwmon/coretemp.rst
>
> If the Expression is "(TjMax - currentTemp) * scalar - Const", then it
> will calculate different results between these two CPU processor.
>
>
> Harvey Wu
>
> From: Vijay Khemka <vijaykhemkalinux@gmail.com>
> Sent: Wednesday, December 30, 2020 1:02 PM
> To: Harvey Wu (吳崇維) <Harvey.Wu@quantatw.com>
> Cc: Lei Yu <yulei.sh@bytedance.com>; openbmc@lists.ozlabs.org
> Subject: Re: [phosphor-virtual-sensor] Add new feature to get Warning or
> Critical property
>
> I am not understanding why you want to use threshold values to calculate
> new sensor values. We can certainly add this feature but it can not be
> based on name rather we have to think differently to get. It would really
> help to decide on design once we know it's actual usage. I can't think of
> any situation here.
>
> Regards
> -Vijay
>
> On Tue, Dec 29, 2020 at 6:49 PM Harvey Wu (吳崇維) <mailto:
> Harvey.Wu@quantatw.com> wrote:
> > On Wed, Dec 30, 2020 at 10:19 AM Harvey Wu (吳崇維)
> > <mailto:Harvey.Wu@quantatw.com> wrote:
> > >
> > > Hi All,
> > >
> > >
> > >
> > >         The current phosphor-virtual-sensor DbusParam value is from
> > DBus “Value” property which interface is
> > “xyz.openbmc_project.Sensor.Value”.
> > >
> > > Because I want to get the property “CriticalHigh” value on DBus, could
> I
> > add a new feature “Interface” property in json file? The property can be
> > “CriticalHigh” , “CriticalLow” , “WarningHigh” or “WarningLow”.
> >
> >
> > According to
> > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .
> > com%2Fopenbmc%2Fphosphor-virtual-sensor%2Fblob%2Fmaster%2Fvirtual_
> > sensor_config.json%23L8-L14&amp;data=04%7C01%7CHarvey.Wu%40quantat
> >
> https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fw.com%2F&data=04%7C01%7CHarvey.Wu%40quantatw.com%7Cf75707a7431e4dca712e08d8ac80182b%7C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637449013448979318%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=kxT3i6moVPggqbY1zJiag18Cp9wGmyk1ctqrO8XlTUc%3D&reserved=0%7C1479ebd5aedd4b8a7c3308d8ac6a4393%7C179b032707fc4973ac73
> > 8de7313561b2%7C1%7C0%7C637448919693850853%7CUnknown%7CTWFpb
> > GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> > Mn0%3D%7C1000&amp;sdata=0KyrnRkZKr9M59DEg%2BnyUruiege%2B3EDXb
> > mxzR%2B6xeng%3D&amp;reserved=0,
> > the threshold properties are already supported.
>
> The "Threshold" in json seems only can set the sensor critical or warning
> value, but I want to use the critical or warning value in "Expression".
> After check the newest commit, this function seems doesn't implement in
> current source code.
>
> >
> > --
> > BRs,
> > Lei YU
>
>
> Harvey Wu
>

[-- Attachment #2: Type: text/html, Size: 6028 bytes --]

Re: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Ed Tanous]

On Tue, Dec 29, 2020 at 10:14 PM Harvey Wu (吳崇維) <Harvey.Wu@quantatw.com> wrote:
>
> Hi Vijay,
>
>         Because I want to calculate the different value between CriticalHigh and current sensor value in CPU sensor, and then there are weighting and some const value to multiply or plus the different value.
> But the CPU sensor critical high threshold temp is set as TjMax, and the TjMax value will be different according the different CPU process type, not a Const. That's why I want to add this feature.
>
> For example, according the Document below, if the processor is i7-3770K, the TjMax is 105, and if the processor is U7500, the TjMax is 100.
> https://www.kernel.org/doc/Documentation/hwmon/coretemp.rst

The intent is that these values will be read out of the chip by cpu
sensor, and the cpu sensor critical threshold will be set to 105, and
100 respectively.  phosphor-virtual-sensor does not need to be aware
of these thresholds at all, or would only forward on the value closest
to the threshold.

>
> If the Expression is "(TjMax - currentTemp) * scalar - Const", then it will calculate different results between these two CPU processor.

If we need this, please add this as a feature within CPUSensor.  With
that said, I'm still not quite following the use case.  Can you
provide more details on what expression you're hoping to write in
phosphor-virtual-sensor, and what the intended behavior is?  If your
goal is to implement fan boosting, I believe this is already covered
in phosphor-pid-control.


Could you make more clear what you're trying to accomplish?  That will
probably help us give more concrete suggestions.

>
>
> Harvey Wu
>
> From: Vijay Khemka <vijaykhemkalinux@gmail.com>
> Sent: Wednesday, December 30, 2020 1:02 PM
> To: Harvey Wu (吳崇維) <Harvey.Wu@quantatw.com>
> Cc: Lei Yu <yulei.sh@bytedance.com>; openbmc@lists.ozlabs.org
> Subject: Re: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property
>
> I am not understanding why you want to use threshold values to calculate new sensor values. We can certainly add this feature but it can not be based on name rather we have to think differently to get. It would really help to decide on design once we know it's actual usage. I can't think of any situation here.
>
> Regards
> -Vijay
>
> On Tue, Dec 29, 2020 at 6:49 PM Harvey Wu (吳崇維) <mailto:Harvey.Wu@quantatw.com> wrote:
> > On Wed, Dec 30, 2020 at 10:19 AM Harvey Wu (吳崇維)
> > <mailto:Harvey.Wu@quantatw.com> wrote:
> > >
> > > Hi All,
> > >
> > >
> > >
> > >         The current phosphor-virtual-sensor DbusParam value is from
> > DBus “Value” property which interface is
> > “xyz.openbmc_project.Sensor.Value”.
> > >
> > > Because I want to get the property “CriticalHigh” value on DBus, could I
> > add a new feature “Interface” property in json file? The property can be
> > “CriticalHigh” , “CriticalLow” , “WarningHigh” or “WarningLow”.
> >
> >
> > According to
> > https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.
> > com%2Fopenbmc%2Fphosphor-virtual-sensor%2Fblob%2Fmaster%2Fvirtual_
> > sensor_config.json%23L8-L14&amp;data=04%7C01%7CHarvey.Wu%40quantat
> > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fw.com%2F&data=04%7C01%7CHarvey.Wu%40quantatw.com%7Cf75707a7431e4dca712e08d8ac80182b%7C179b032707fc4973ac738de7313561b2%7C1%7C0%7C637449013448979318%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=kxT3i6moVPggqbY1zJiag18Cp9wGmyk1ctqrO8XlTUc%3D&reserved=0%7C1479ebd5aedd4b8a7c3308d8ac6a4393%7C179b032707fc4973ac73
> > 8de7313561b2%7C1%7C0%7C637448919693850853%7CUnknown%7CTWFpb
> > GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> > Mn0%3D%7C1000&amp;sdata=0KyrnRkZKr9M59DEg%2BnyUruiege%2B3EDXb
> > mxzR%2B6xeng%3D&amp;reserved=0,
> > the threshold properties are already supported.
>
> The "Threshold" in json seems only can set the sensor critical or warning value, but I want to use the critical or warning value in "Expression".
> After check the newest commit, this function seems doesn't implement in current source code.
>
> >
> > --
> > BRs,
> > Lei YU
>
>
> Harvey Wu

Re: [phosphor-virtual-sensor] Add new feature to get Warning or Critical property

[Ed Tanous]

On Wed, Dec 30, 2020 at 12:49 PM Vijay Khemka
<vijaykhemkalinux@gmail.com> wrote:
>
> Yes I understand that critical values for cpus are received from CPUs and not a constant values.  But just for this specific sensor we should not add a new complex feature in a virtual sensor.

+1.  This seems like a use case we've already considered and handle.