[phosphor-pid-control] scaling issue

[phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

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

Hi All,


After commit [1], I found my temp sensor reading would be re-scaled by multiplying 1 over 255, making temperature into unfamiliar unit. Also the fan rpm reading would lie in [0,1] interval, letting the fan input to be 0 (since the input value of fan is from an integer array [2]). Are these normal behaviors? Or do I miss something?


[1] https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c

[2] https://github.com/openbmc/phosphor-pid-control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86


Thanks,


Hank Liou

Quanta Computer Inc.


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

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi All,
>
>
> After commit [1], I found my temp sensor reading would be re-scaled by multiplying 1 over 255, making temperature into unfamiliar unit. Also the fan rpm reading would lie in [0,1] interval, letting the fan input to be 0 (since the input value of fan is from an integer array [2]). Are these normal behaviors? Or do I miss something?

Are you using dbus configuration or json?  If json, can you attach
your config.  Since you're saying it was working and now isn't, I'm
assuming there's something about the config being treated differently
with the code changes in an unexpected way.

>
>
> [1] https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>
> [2] https://github.com/openbmc/phosphor-pid-control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>
>
> Thanks,
>
>
> Hank Liou
>
> Quanta Computer Inc.
>
>

RE: [phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

Hi Patrick,

>-----Original Message-----
>From: Patrick Venture [mailto:venture@google.com]
>Sent: Wednesday, August 21, 2019 10:32 PM
>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
><james.feist@linux.intel.com>
>Cc: openbmc@lists.ozlabs.org
>Subject: Re: [phosphor-pid-control] scaling issue
>
>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
><Hank.Liou@quantatw.com> wrote:
>>
>> Hi All,
>>
>>
>> After commit [1], I found my temp sensor reading would be re-scaled by
>multiplying 1 over 255, making temperature into unfamiliar unit. Also the fan
>rpm reading would lie in [0,1] interval, letting the fan input to be 0 (since the
>input value of fan is from an integer array [2]). Are these normal behaviors?
>Or do I miss something?
>
>Are you using dbus configuration or json?  If json, can you attach your config.
>Since you're saying it was working and now isn't, I'm assuming there's
>something about the config being treated differently with the code changes in
>an unexpected way.

I found pid control will first read min and max from dbus and then (before commit [1]) revise them if 

info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)

After value initialization, the min and max would be the ones in json file (Json file [3]). However, after commit [1] the min and max values of config would not be fed into the fan control process. The scaling issue is originated from commit [4] with the aim to treat fan rpm as percentage. It seems that commit [1] unexpectedly affects temp sensors in the sense that the temp is the integer reading not percentage. Before commit [1], it would not re-scale the temp reading since my min and max are 0 [6].



There is another issue with commit [1]. Now the fan rpm would be something like

1500 / 20000 = 0.075

where rpm max 20000 is from dbus. However the fan input function would transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0 not percentage. Is there something wrong?

[1] https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
[2] https://github.com/openbmc/phosphor-pid-control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
[3]
       {
            "name": "fan1",
            "type": "fan",
            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
            "min": 0,
            "max": 255
        },
        {
            "name": "temp1",
            "type": "temp",
            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
            "writePath": "",
            "min": 0,
            "max": 0
        }
[4] https://github.com/openbmc/phosphor-pid-control/commit/75eb769d351434547899186f73ff70ae00d7934a
[5] https://github.com/openbmc/phosphor-pid-control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L64
[6] https://github.com/openbmc/phosphor-pid-control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassive.cpp#L158

>
>>
>>
>> [1]
>> https://github.com/openbmc/phosphor-pid-
>control/commit/fc2e803f5d92569
>> 44e18c7c878a441606b1f121c
>>
>> [2]
>> https://github.com/openbmc/phosphor-pid-
>control/blob/a7ec8350d17b70153
>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>>
>>
>> Thanks,
>>
>>
>> Hank Liou
>>
>> Quanta Computer Inc.
>>
>>

Sincerely,
Hank

RE: [phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

Hi Patrick,

I think it's OK to parse the config min&max for temp sensors.

Any suggestion?

Thanks,
Hank

>-----Original Message-----
>From: openbmc [mailto:openbmc-
>bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
>(劉晉翰)
>Sent: Friday, August 23, 2019 4:31 PM
>To: Patrick Venture <venture@google.com>; James Feist
><james.feist@linux.intel.com>
>Cc: openbmc@lists.ozlabs.org
>Subject: RE: [phosphor-pid-control] scaling issue
>
>Hi Patrick,
>
>>-----Original Message-----
>>From: Patrick Venture [mailto:venture@google.com]
>>Sent: Wednesday, August 21, 2019 10:32 PM
>>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
>><james.feist@linux.intel.com>
>>Cc: openbmc@lists.ozlabs.org
>>Subject: Re: [phosphor-pid-control] scaling issue
>>
>>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>><Hank.Liou@quantatw.com> wrote:
>>>
>>> Hi All,
>>>
>>>
>>> After commit [1], I found my temp sensor reading would be re-scaled
>>> by
>>multiplying 1 over 255, making temperature into unfamiliar unit. Also
>>the fan rpm reading would lie in [0,1] interval, letting the fan input
>>to be 0 (since the input value of fan is from an integer array [2]). Are these
>normal behaviors?
>>Or do I miss something?
>>
>>Are you using dbus configuration or json?  If json, can you attach your config.
>>Since you're saying it was working and now isn't, I'm assuming there's
>>something about the config being treated differently with the code
>>changes in an unexpected way.
>
>I found pid control will first read min and max from dbus and then (before
>commit [1]) revise them if
>
>info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
>
>After value initialization, the min and max would be the ones in json file (Json
>file [3]). However, after commit [1] the min and max values of config would
>not be fed into the fan control process. The scaling issue is originated from
>commit [4] with the aim to treat fan rpm as percentage. It seems that commit
>[1] unexpectedly affects temp sensors in the sense that the temp is the
>integer reading not percentage. Before commit [1], it would not re-scale the
>temp reading since my min and max are 0 [6].
>
>
>
>There is another issue with commit [1]. Now the fan rpm would be something
>like
>
>1500 / 20000 = 0.075
>
>where rpm max 20000 is from dbus. However the fan input function would
>transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
>not percentage. Is there something wrong?
>
>[1] https://github.com/openbmc/phosphor-pid-
>control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>[2] https://github.com/openbmc/phosphor-pid-
>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>r.cpp#L86
>[3]
>       {
>            "name": "fan1",
>            "type": "fan",
>            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
>            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
>            "min": 0,
>            "max": 255
>        },
>        {
>            "name": "temp1",
>            "type": "temp",
>            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
>            "writePath": "",
>            "min": 0,
>            "max": 0
>        }
>[4] https://github.com/openbmc/phosphor-pid-
>control/commit/75eb769d351434547899186f73ff70ae00d7934a
>[5] https://github.com/openbmc/phosphor-pid-
>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>r.cpp#L64
>[6] https://github.com/openbmc/phosphor-pid-
>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
>ve.cpp#L158
>
>>
>>>
>>>
>>> [1]
>>> https://github.com/openbmc/phosphor-pid-
>>control/commit/fc2e803f5d92569
>>> 44e18c7c878a441606b1f121c
>>>
>>> [2]
>>> https://github.com/openbmc/phosphor-pid-
>>control/blob/a7ec8350d17b70153
>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>>>
>>>
>>> Thanks,
>>>
>>>
>>> Hank Liou
>>>
>>> Quanta Computer Inc.
>>>
>>>
>
>Sincerely,
>Hank

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi Patrick,
>
> I think it's OK to parse the config min&max for temp sensors.

So, iirc, the min/max in the  json is only for sensors that write, and
not read.  Sorry, I'm ramping up on a new team and slower to catch up
to emails.

Yeah, the min/max in the json are for basically converting a
percentage value to a PWM values -- that's its initial goal.
Temperature sensors typically don't have minimum and maximum values in
the code's use-cases.  I added James to this because they use the
daemon for other cases -- where perhaps that'll make sense.

>
> Any suggestion?
>
> Thanks,
> Hank
>
> >-----Original Message-----
> >From: openbmc [mailto:openbmc-
> >bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
> >(劉晉翰)
> >Sent: Friday, August 23, 2019 4:31 PM
> >To: Patrick Venture <venture@google.com>; James Feist
> ><james.feist@linux.intel.com>
> >Cc: openbmc@lists.ozlabs.org
> >Subject: RE: [phosphor-pid-control] scaling issue
> >
> >Hi Patrick,
> >
> >>-----Original Message-----
> >>From: Patrick Venture [mailto:venture@google.com]
> >>Sent: Wednesday, August 21, 2019 10:32 PM
> >>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> >><james.feist@linux.intel.com>
> >>Cc: openbmc@lists.ozlabs.org
> >>Subject: Re: [phosphor-pid-control] scaling issue
> >>
> >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> >><Hank.Liou@quantatw.com> wrote:
> >>>
> >>> Hi All,
> >>>
> >>>
> >>> After commit [1], I found my temp sensor reading would be re-scaled
> >>> by
> >>multiplying 1 over 255, making temperature into unfamiliar unit. Also
> >>the fan rpm reading would lie in [0,1] interval, letting the fan input
> >>to be 0 (since the input value of fan is from an integer array [2]). Are these
> >normal behaviors?
> >>Or do I miss something?
> >>
> >>Are you using dbus configuration or json?  If json, can you attach your config.
> >>Since you're saying it was working and now isn't, I'm assuming there's
> >>something about the config being treated differently with the code
> >>changes in an unexpected way.
> >
> >I found pid control will first read min and max from dbus and then (before
> >commit [1]) revise them if
> >
> >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> >
> >After value initialization, the min and max would be the ones in json file (Json
> >file [3]). However, after commit [1] the min and max values of config would
> >not be fed into the fan control process. The scaling issue is originated from
> >commit [4] with the aim to treat fan rpm as percentage. It seems that commit
> >[1] unexpectedly affects temp sensors in the sense that the temp is the
> >integer reading not percentage. Before commit [1], it would not re-scale the
> >temp reading since my min and max are 0 [6].
> >
> >
> >
> >There is another issue with commit [1]. Now the fan rpm would be something
> >like
> >
> >1500 / 20000 = 0.075
> >
> >where rpm max 20000 is from dbus. However the fan input function would
> >transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
> >not percentage. Is there something wrong?
> >
> >[1] https://github.com/openbmc/phosphor-pid-
> >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >[2] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L86
> >[3]
> >       {
> >            "name": "fan1",
> >            "type": "fan",
> >            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> >            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> >            "min": 0,
> >            "max": 255
> >        },
> >        {
> >            "name": "temp1",
> >            "type": "temp",
> >            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
> >            "writePath": "",
> >            "min": 0,
> >            "max": 0
> >        }
> >[4] https://github.com/openbmc/phosphor-pid-
> >control/commit/75eb769d351434547899186f73ff70ae00d7934a
> >[5] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L64
> >[6] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
> >ve.cpp#L158
> >
> >>
> >>>
> >>>
> >>> [1]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/commit/fc2e803f5d92569
> >>> 44e18c7c878a441606b1f121c
> >>>
> >>> [2]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/blob/a7ec8350d17b70153
> >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> >>>
> >>>
> >>> Thanks,
> >>>
> >>>
> >>> Hank Liou
> >>>
> >>> Quanta Computer Inc.
> >>>
> >>>
> >
> >Sincerely,
> >Hank

Re: [phosphor-pid-control] scaling issue

[James Feist]

On 8/29/19 7:24 AM, Patrick Venture wrote:
> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>>
>> Hi Patrick,
>>
>> I think it's OK to parse the config min&max for temp sensors.
> 
> So, iirc, the min/max in the  json is only for sensors that write, and
> not read.  Sorry, I'm ramping up on a new team and slower to catch up
> to emails.
> 
> Yeah, the min/max in the json are for basically converting a
> percentage value to a PWM values -- that's its initial goal.
> Temperature sensors typically don't have minimum and maximum values in
> the code's use-cases.  I added James to this because they use the
> daemon for other cases -- where perhaps that'll make sense.

I believe the history of min/max being removed for passive sensors was 
here: 
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/23470

The only other use case we have for min / max is for fan RPM so that we 
can create a PID based on Tach %, so that the same PID can be reused on 
multiple platforms with different sized fans.

-James

> 
>>
>> Any suggestion?
>>
>> Thanks,
>> Hank
>>
>>> -----Original Message-----
>>> From: openbmc [mailto:openbmc-
>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
>>> (劉晉翰)
>>> Sent: Friday, August 23, 2019 4:31 PM
>>> To: Patrick Venture <venture@google.com>; James Feist
>>> <james.feist@linux.intel.com>
>>> Cc: openbmc@lists.ozlabs.org
>>> Subject: RE: [phosphor-pid-control] scaling issue
>>>
>>> Hi Patrick,
>>>
>>>> -----Original Message-----
>>>> From: Patrick Venture [mailto:venture@google.com]
>>>> Sent: Wednesday, August 21, 2019 10:32 PM
>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
>>>> <james.feist@linux.intel.com>
>>>> Cc: openbmc@lists.ozlabs.org
>>>> Subject: Re: [phosphor-pid-control] scaling issue
>>>>
>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>>>> <Hank.Liou@quantatw.com> wrote:
>>>>>
>>>>> Hi All,
>>>>>
>>>>>
>>>>> After commit [1], I found my temp sensor reading would be re-scaled
>>>>> by
>>>> multiplying 1 over 255, making temperature into unfamiliar unit. Also
>>>> the fan rpm reading would lie in [0,1] interval, letting the fan input
>>>> to be 0 (since the input value of fan is from an integer array [2]). Are these
>>> normal behaviors?
>>>> Or do I miss something?
>>>>
>>>> Are you using dbus configuration or json?  If json, can you attach your config.
>>>> Since you're saying it was working and now isn't, I'm assuming there's
>>>> something about the config being treated differently with the code
>>>> changes in an unexpected way.
>>>
>>> I found pid control will first read min and max from dbus and then (before
>>> commit [1]) revise them if
>>>
>>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
>>>
>>> After value initialization, the min and max would be the ones in json file (Json
>>> file [3]). However, after commit [1] the min and max values of config would
>>> not be fed into the fan control process. The scaling issue is originated from
>>> commit [4] with the aim to treat fan rpm as percentage. It seems that commit
>>> [1] unexpectedly affects temp sensors in the sense that the temp is the
>>> integer reading not percentage. Before commit [1], it would not re-scale the
>>> temp reading since my min and max are 0 [6].
>>>
>>>
>>>
>>> There is another issue with commit [1]. Now the fan rpm would be something
>>> like
>>>
>>> 1500 / 20000 = 0.075
>>>
>>> where rpm max 20000 is from dbus. However the fan input function would
>>> transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
>>> not percentage. Is there something wrong?
>>>
>>> [1] https://github.com/openbmc/phosphor-pid-
>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>>> [2] https://github.com/openbmc/phosphor-pid-
>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>>> r.cpp#L86
>>> [3]
>>>        {
>>>             "name": "fan1",
>>>             "type": "fan",
>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
>>>             "min": 0,
>>>             "max": 255
>>>         },
>>>         {
>>>             "name": "temp1",
>>>             "type": "temp",
>>>             "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
>>>             "writePath": "",
>>>             "min": 0,
>>>             "max": 0
>>>         }
>>> [4] https://github.com/openbmc/phosphor-pid-
>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
>>> [5] https://github.com/openbmc/phosphor-pid-
>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>>> r.cpp#L64
>>> [6] https://github.com/openbmc/phosphor-pid-
>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
>>> ve.cpp#L158
>>>
>>>>
>>>>>
>>>>>
>>>>> [1]
>>>>> https://github.com/openbmc/phosphor-pid-
>>>> control/commit/fc2e803f5d92569
>>>>> 44e18c7c878a441606b1f121c
>>>>>
>>>>> [2]
>>>>> https://github.com/openbmc/phosphor-pid-
>>>> control/blob/a7ec8350d17b70153
>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>
>>>>> Hank Liou
>>>>>
>>>>> Quanta Computer Inc.
>>>>>
>>>>>
>>>
>>> Sincerely,
>>> Hank

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Thu, Aug 29, 2019 at 9:14 AM James Feist <james.feist@linux.intel.com> wrote:
>
> On 8/29/19 7:24 AM, Patrick Venture wrote:
> > On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
> >>
> >> Hi Patrick,
> >>
> >> I think it's OK to parse the config min&max for temp sensors.
> >
> > So, iirc, the min/max in the  json is only for sensors that write, and
> > not read.  Sorry, I'm ramping up on a new team and slower to catch up
> > to emails.
> >
> > Yeah, the min/max in the json are for basically converting a
> > percentage value to a PWM values -- that's its initial goal.
> > Temperature sensors typically don't have minimum and maximum values in
> > the code's use-cases.  I added James to this because they use the
> > daemon for other cases -- where perhaps that'll make sense.
>
> I believe the history of min/max being removed for passive sensors was
> here:
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/23470
>
> The only other use case we have for min / max is for fan RPM so that we
> can create a PID based on Tach %, so that the same PID can be reused on
> multiple platforms with different sized fans.
>
> -James
>
> >
> >>
> >> Any suggestion?
> >>
> >> Thanks,
> >> Hank
> >>
> >>> -----Original Message-----
> >>> From: openbmc [mailto:openbmc-
> >>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
> >>> (劉晉翰)
> >>> Sent: Friday, August 23, 2019 4:31 PM
> >>> To: Patrick Venture <venture@google.com>; James Feist
> >>> <james.feist@linux.intel.com>
> >>> Cc: openbmc@lists.ozlabs.org
> >>> Subject: RE: [phosphor-pid-control] scaling issue
> >>>
> >>> Hi Patrick,
> >>>
> >>>> -----Original Message-----
> >>>> From: Patrick Venture [mailto:venture@google.com]
> >>>> Sent: Wednesday, August 21, 2019 10:32 PM
> >>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> >>>> <james.feist@linux.intel.com>
> >>>> Cc: openbmc@lists.ozlabs.org
> >>>> Subject: Re: [phosphor-pid-control] scaling issue
> >>>>
> >>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> >>>> <Hank.Liou@quantatw.com> wrote:
> >>>>>
> >>>>> Hi All,
> >>>>>
> >>>>>
> >>>>> After commit [1], I found my temp sensor reading would be re-scaled
> >>>>> by
> >>>> multiplying 1 over 255, making temperature into unfamiliar unit. Also
> >>>> the fan rpm reading would lie in [0,1] interval, letting the fan input
> >>>> to be 0 (since the input value of fan is from an integer array [2]). Are these
> >>> normal behaviors?
> >>>> Or do I miss something?
> >>>>
> >>>> Are you using dbus configuration or json?  If json, can you attach your config.
> >>>> Since you're saying it was working and now isn't, I'm assuming there's
> >>>> something about the config being treated differently with the code
> >>>> changes in an unexpected way.
> >>>
> >>> I found pid control will first read min and max from dbus and then (before
> >>> commit [1]) revise them if
> >>>
> >>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> >>>
> >>> After value initialization, the min and max would be the ones in json file (Json
> >>> file [3]). However, after commit [1] the min and max values of config would
> >>> not be fed into the fan control process. The scaling issue is originated from
> >>> commit [4] with the aim to treat fan rpm as percentage. It seems that commit
> >>> [1] unexpectedly affects temp sensors in the sense that the temp is the
> >>> integer reading not percentage. Before commit [1], it would not re-scale the
> >>> temp reading since my min and max are 0 [6].
> >>>
> >>>
> >>>
> >>> There is another issue with commit [1]. Now the fan rpm would be something
> >>> like
> >>>
> >>> 1500 / 20000 = 0.075
> >>>
> >>> where rpm max 20000 is from dbus. However the fan input function would
> >>> transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
> >>> not percentage. Is there something wrong?
> >>>
> >>> [1] https://github.com/openbmc/phosphor-pid-
> >>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >>> [2] https://github.com/openbmc/phosphor-pid-
> >>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >>> r.cpp#L86
> >>> [3]
> >>>        {
> >>>             "name": "fan1",
> >>>             "type": "fan",
> >>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> >>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> >>>             "min": 0,
> >>>             "max": 255
> >>>         },
> >>>         {
> >>>             "name": "temp1",
> >>>             "type": "temp",
> >>>             "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
> >>>             "writePath": "",
> >>>             "min": 0,
> >>>             "max": 0
> >>>         }
> >>> [4] https://github.com/openbmc/phosphor-pid-
> >>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
> >>> [5] https://github.com/openbmc/phosphor-pid-
> >>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >>> r.cpp#L64
> >>> [6] https://github.com/openbmc/phosphor-pid-
> >>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
> >>> ve.cpp#L158
> >>>
> >>>>
> >>>>>
> >>>>>
> >>>>> [1]
> >>>>> https://github.com/openbmc/phosphor-pid-
> >>>> control/commit/fc2e803f5d92569
> >>>>> 44e18c7c878a441606b1f121c
> >>>>>
> >>>>> [2]
> >>>>> https://github.com/openbmc/phosphor-pid-
> >>>> control/blob/a7ec8350d17b70153
> >>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> >>>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>>
> >>>>> Hank Liou
> >>>>>
> >>>>> Quanta Computer Inc.
> >>>>>
> >>>>>
> >>>
> >>> Sincerely,
> >>> Hank

Hank, do you need to use the min/max on the temperature sensors?  Try
your build with those entries deleted from the json file (they're
optional).

James, I think I should add a warning and ignore those fields in that
case.  What do you think?  (I honestly thought they were already
ignored for temperature sensors -- have to look at the code to
verify).

Re: [phosphor-pid-control] scaling issue

[James Feist]

On 8/29/19 9:21 AM, Patrick Venture wrote:
> On Thu, Aug 29, 2019 at 9:14 AM James Feist <james.feist@linux.intel.com> wrote:
>>
>> On 8/29/19 7:24 AM, Patrick Venture wrote:
>>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>>>>
>>>> Hi Patrick,
>>>>
>>>> I think it's OK to parse the config min&max for temp sensors.
>>>
>>> So, iirc, the min/max in the  json is only for sensors that write, and
>>> not read.  Sorry, I'm ramping up on a new team and slower to catch up
>>> to emails.
>>>
>>> Yeah, the min/max in the json are for basically converting a
>>> percentage value to a PWM values -- that's its initial goal.
>>> Temperature sensors typically don't have minimum and maximum values in
>>> the code's use-cases.  I added James to this because they use the
>>> daemon for other cases -- where perhaps that'll make sense.
>>
>> I believe the history of min/max being removed for passive sensors was
>> here:
>> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/23470
>>
>> The only other use case we have for min / max is for fan RPM so that we
>> can create a PID based on Tach %, so that the same PID can be reused on
>> multiple platforms with different sized fans.
>>
>> -James
>>
>>>
>>>>
>>>> Any suggestion?
>>>>
>>>> Thanks,
>>>> Hank
>>>>
>>>>> -----Original Message-----
>>>>> From: openbmc [mailto:openbmc-
>>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
>>>>> (劉晉翰)
>>>>> Sent: Friday, August 23, 2019 4:31 PM
>>>>> To: Patrick Venture <venture@google.com>; James Feist
>>>>> <james.feist@linux.intel.com>
>>>>> Cc: openbmc@lists.ozlabs.org
>>>>> Subject: RE: [phosphor-pid-control] scaling issue
>>>>>
>>>>> Hi Patrick,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Patrick Venture [mailto:venture@google.com]
>>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
>>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
>>>>>> <james.feist@linux.intel.com>
>>>>>> Cc: openbmc@lists.ozlabs.org
>>>>>> Subject: Re: [phosphor-pid-control] scaling issue
>>>>>>
>>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>>>>>> <Hank.Liou@quantatw.com> wrote:
>>>>>>>
>>>>>>> Hi All,
>>>>>>>
>>>>>>>
>>>>>>> After commit [1], I found my temp sensor reading would be re-scaled
>>>>>>> by
>>>>>> multiplying 1 over 255, making temperature into unfamiliar unit. Also
>>>>>> the fan rpm reading would lie in [0,1] interval, letting the fan input
>>>>>> to be 0 (since the input value of fan is from an integer array [2]). Are these
>>>>> normal behaviors?
>>>>>> Or do I miss something?
>>>>>>
>>>>>> Are you using dbus configuration or json?  If json, can you attach your config.
>>>>>> Since you're saying it was working and now isn't, I'm assuming there's
>>>>>> something about the config being treated differently with the code
>>>>>> changes in an unexpected way.
>>>>>
>>>>> I found pid control will first read min and max from dbus and then (before
>>>>> commit [1]) revise them if
>>>>>
>>>>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
>>>>>
>>>>> After value initialization, the min and max would be the ones in json file (Json
>>>>> file [3]). However, after commit [1] the min and max values of config would
>>>>> not be fed into the fan control process. The scaling issue is originated from
>>>>> commit [4] with the aim to treat fan rpm as percentage. It seems that commit
>>>>> [1] unexpectedly affects temp sensors in the sense that the temp is the
>>>>> integer reading not percentage. Before commit [1], it would not re-scale the
>>>>> temp reading since my min and max are 0 [6].
>>>>>
>>>>>
>>>>>
>>>>> There is another issue with commit [1]. Now the fan rpm would be something
>>>>> like
>>>>>
>>>>> 1500 / 20000 = 0.075
>>>>>
>>>>> where rpm max 20000 is from dbus. However the fan input function would
>>>>> transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
>>>>> not percentage. Is there something wrong?
>>>>>
>>>>> [1] https://github.com/openbmc/phosphor-pid-
>>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>>>>> [2] https://github.com/openbmc/phosphor-pid-
>>>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>>>>> r.cpp#L86
>>>>> [3]
>>>>>         {
>>>>>              "name": "fan1",
>>>>>              "type": "fan",
>>>>>              "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
>>>>>              "writePath": "/sys/class/hwmon/hwmon1/pwm1",
>>>>>              "min": 0,
>>>>>              "max": 255
>>>>>          },
>>>>>          {
>>>>>              "name": "temp1",
>>>>>              "type": "temp",
>>>>>              "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
>>>>>              "writePath": "",
>>>>>              "min": 0,
>>>>>              "max": 0
>>>>>          }
>>>>> [4] https://github.com/openbmc/phosphor-pid-
>>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
>>>>> [5] https://github.com/openbmc/phosphor-pid-
>>>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>>>>> r.cpp#L64
>>>>> [6] https://github.com/openbmc/phosphor-pid-
>>>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
>>>>> ve.cpp#L158
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>> https://github.com/openbmc/phosphor-pid-
>>>>>> control/commit/fc2e803f5d92569
>>>>>>> 44e18c7c878a441606b1f121c
>>>>>>>
>>>>>>> [2]
>>>>>>> https://github.com/openbmc/phosphor-pid-
>>>>>> control/blob/a7ec8350d17b70153
>>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>
>>>>>>> Hank Liou
>>>>>>>
>>>>>>> Quanta Computer Inc.
>>>>>>>
>>>>>>>
>>>>>
>>>>> Sincerely,
>>>>> Hank
> 
> Hank, do you need to use the min/max on the temperature sensors?  Try
> your build with those entries deleted from the json file (they're
> optional).
> 
> James, I think I should add a warning and ignore those fields in that
> case.  What do you think?  (I honestly thought they were already
> ignored for temperature sensors -- have to look at the code to
> verify).

I don't have much of an opinion here as I don't use the json 
configuration, but more warnings never hurt.

> 

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
the srcrev bump should propagate into openbmc/openbmc in a day or two.

On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi Patrick,
>
> I think it's OK to parse the config min&max for temp sensors.
>
> Any suggestion?
>
> Thanks,
> Hank
>
> >-----Original Message-----
> >From: openbmc [mailto:openbmc-
> >bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
> >(劉晉翰)
> >Sent: Friday, August 23, 2019 4:31 PM
> >To: Patrick Venture <venture@google.com>; James Feist
> ><james.feist@linux.intel.com>
> >Cc: openbmc@lists.ozlabs.org
> >Subject: RE: [phosphor-pid-control] scaling issue
> >
> >Hi Patrick,
> >
> >>-----Original Message-----
> >>From: Patrick Venture [mailto:venture@google.com]
> >>Sent: Wednesday, August 21, 2019 10:32 PM
> >>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> >><james.feist@linux.intel.com>
> >>Cc: openbmc@lists.ozlabs.org
> >>Subject: Re: [phosphor-pid-control] scaling issue
> >>
> >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> >><Hank.Liou@quantatw.com> wrote:
> >>>
> >>> Hi All,
> >>>
> >>>
> >>> After commit [1], I found my temp sensor reading would be re-scaled
> >>> by
> >>multiplying 1 over 255, making temperature into unfamiliar unit. Also
> >>the fan rpm reading would lie in [0,1] interval, letting the fan input
> >>to be 0 (since the input value of fan is from an integer array [2]). Are these
> >normal behaviors?
> >>Or do I miss something?
> >>
> >>Are you using dbus configuration or json?  If json, can you attach your config.
> >>Since you're saying it was working and now isn't, I'm assuming there's
> >>something about the config being treated differently with the code
> >>changes in an unexpected way.
> >
> >I found pid control will first read min and max from dbus and then (before
> >commit [1]) revise them if
> >
> >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> >
> >After value initialization, the min and max would be the ones in json file (Json
> >file [3]). However, after commit [1] the min and max values of config would
> >not be fed into the fan control process. The scaling issue is originated from
> >commit [4] with the aim to treat fan rpm as percentage. It seems that commit
> >[1] unexpectedly affects temp sensors in the sense that the temp is the
> >integer reading not percentage. Before commit [1], it would not re-scale the
> >temp reading since my min and max are 0 [6].
> >
> >
> >
> >There is another issue with commit [1]. Now the fan rpm would be something
> >like
> >
> >1500 / 20000 = 0.075
> >
> >where rpm max 20000 is from dbus. However the fan input function would
> >transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
> >not percentage. Is there something wrong?
> >
> >[1] https://github.com/openbmc/phosphor-pid-
> >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >[2] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L86
> >[3]
> >       {
> >            "name": "fan1",
> >            "type": "fan",
> >            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> >            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> >            "min": 0,
> >            "max": 255
> >        },
> >        {
> >            "name": "temp1",
> >            "type": "temp",
> >            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
> >            "writePath": "",
> >            "min": 0,
> >            "max": 0
> >        }
> >[4] https://github.com/openbmc/phosphor-pid-
> >control/commit/75eb769d351434547899186f73ff70ae00d7934a
> >[5] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L64
> >[6] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
> >ve.cpp#L158
> >
> >>
> >>>
> >>>
> >>> [1]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/commit/fc2e803f5d92569
> >>> 44e18c7c878a441606b1f121c
> >>>
> >>> [2]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/blob/a7ec8350d17b70153
> >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> >>>
> >>>
> >>> Thanks,
> >>>
> >>>
> >>> Hank Liou
> >>>
> >>> Quanta Computer Inc.
> >>>
> >>>
> >
> >Sincerely,
> >Hank

Re: [phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

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

Hi Patrick,


Since we use phosphor-sel-logger [1] at the same time, we do assign min and max of temp sensors to Dbus (max: 127, min: -128). So in the present case, our temp value will be divided by 0.255 (also due to exponent is -3 here). This will cause re-scaling problem. Therefore there should be an statement to distinguish sensor types. If it is "temp", then one assigns 0 to the min and max from Dbus.


[1] https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59<https://g>


Hank


________________________________
From: Patrick Venture <venture@google.com>
Sent: Friday, August 30, 2019 1:47 AM
To: Hank Liou (劉晉翰)
Cc: James Feist; openbmc@lists.ozlabs.org
Subject: Re: [phosphor-pid-control] scaling issue

PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
the srcrev bump should propagate into openbmc/openbmc in a day or two.

On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi Patrick,
>
> I think it's OK to parse the config min&max for temp sensors.
>
> Any suggestion?
>
> Thanks,
> Hank
>
> >-----Original Message-----
> >From: openbmc [mailto:openbmc-
> >bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
> >(劉晉翰)
> >Sent: Friday, August 23, 2019 4:31 PM
> >To: Patrick Venture <venture@google.com>; James Feist
> ><james.feist@linux.intel.com>
> >Cc: openbmc@lists.ozlabs.org
> >Subject: RE: [phosphor-pid-control] scaling issue
> >
> >Hi Patrick,
> >
> >>-----Original Message-----
> >>From: Patrick Venture [mailto:venture@google.com]
> >>Sent: Wednesday, August 21, 2019 10:32 PM
> >>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> >><james.feist@linux.intel.com>
> >>Cc: openbmc@lists.ozlabs.org
> >>Subject: Re: [phosphor-pid-control] scaling issue
> >>
> >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> >><Hank.Liou@quantatw.com> wrote:
> >>>
> >>> Hi All,
> >>>
> >>>
> >>> After commit [1], I found my temp sensor reading would be re-scaled
> >>> by
> >>multiplying 1 over 255, making temperature into unfamiliar unit. Also
> >>the fan rpm reading would lie in [0,1] interval, letting the fan input
> >>to be 0 (since the input value of fan is from an integer array [2]). Are these
> >normal behaviors?
> >>Or do I miss something?
> >>
> >>Are you using dbus configuration or json?  If json, can you attach your config.
> >>Since you're saying it was working and now isn't, I'm assuming there's
> >>something about the config being treated differently with the code
> >>changes in an unexpected way.
> >
> >I found pid control will first read min and max from dbus and then (before
> >commit [1]) revise them if
> >
> >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> >
> >After value initialization, the min and max would be the ones in json file (Json
> >file [3]). However, after commit [1] the min and max values of config would
> >not be fed into the fan control process. The scaling issue is originated from
> >commit [4] with the aim to treat fan rpm as percentage. It seems that commit
> >[1] unexpectedly affects temp sensors in the sense that the temp is the
> >integer reading not percentage. Before commit [1], it would not re-scale the
> >temp reading since my min and max are 0 [6].
> >
> >
> >
> >There is another issue with commit [1]. Now the fan rpm would be something
> >like
> >
> >1500 / 20000 = 0.075
> >
> >where rpm max 20000 is from dbus. However the fan input function would
> >transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
> >not percentage. Is there something wrong?
> >
> >[1] https://github.com/openbmc/phosphor-pid-
> >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >[2] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L86
> >[3]
> >       {
> >            "name": "fan1",
> >            "type": "fan",
> >            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> >            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> >            "min": 0,
> >            "max": 255
> >        },
> >        {
> >            "name": "temp1",
> >            "type": "temp",
> >            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
> >            "writePath": "",
> >            "min": 0,
> >            "max": 0
> >        }
> >[4] https://github.com/openbmc/phosphor-pid-
> >control/commit/75eb769d351434547899186f73ff70ae00d7934a
> >[5] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L64
> >[6] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
> >ve.cpp#L158
> >
> >>
> >>>
> >>>
> >>> [1]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/commit/fc2e803f5d92569
> >>> 44e18c7c878a441606b1f121c
> >>>
> >>> [2]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/blob/a7ec8350d17b70153
> >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> >>>
> >>>
> >>> Thanks,
> >>>
> >>>
> >>> Hank Liou
> >>>
> >>> Quanta Computer Inc.
> >>>
> >>>
> >
> >Sincerely,
> >Hank

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

Re: [phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

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

Hi Patrick,


Sorry for not clearly stating our problem. We have the following issue:


temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as input for temp sensor ->

stepwise fan table output 100 duty -> full speed fan


As a result, fan will be at full speed while temp is low. Before the commit fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will use config min/max, which is 0 in our case. This would not trigger scaling function, namely scaleSensorReading, in util.cpp. However, after such commit, min/max would be non-zero (-128/127 from DBus). This will trigger scaling function.


[1] https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c


Hank

________________________________
From: Hank Liou (劉晉翰)
Sent: Monday, September 2, 2019 4:52 PM
To: Patrick Venture
Cc: James Feist; openbmc@lists.ozlabs.org
Subject: Re: [phosphor-pid-control] scaling issue


Hi Patrick,


Since we use phosphor-sel-logger [1] at the same time, we do assign min and max of temp sensors to Dbus (max: 127, min: -128). So in the present case, our temp value will be divided by 0.255 (also due to exponent is -3 here). This will cause re-scaling problem. Therefore there should be an statement to distinguish sensor types. If it is "temp", then one assigns 0 to the min and max from Dbus.


[1] https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59<https://g>


Hank


________________________________
From: Patrick Venture <venture@google.com>
Sent: Friday, August 30, 2019 1:47 AM
To: Hank Liou (劉晉翰)
Cc: James Feist; openbmc@lists.ozlabs.org
Subject: Re: [phosphor-pid-control] scaling issue

PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
the srcrev bump should propagate into openbmc/openbmc in a day or two.

On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi Patrick,
>
> I think it's OK to parse the config min&max for temp sensors.
>
> Any suggestion?
>
> Thanks,
> Hank
>
> >-----Original Message-----
> >From: openbmc [mailto:openbmc-
> >bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
> >(劉晉翰)
> >Sent: Friday, August 23, 2019 4:31 PM
> >To: Patrick Venture <venture@google.com>; James Feist
> ><james.feist@linux.intel.com>
> >Cc: openbmc@lists.ozlabs.org
> >Subject: RE: [phosphor-pid-control] scaling issue
> >
> >Hi Patrick,
> >
> >>-----Original Message-----
> >>From: Patrick Venture [mailto:venture@google.com]
> >>Sent: Wednesday, August 21, 2019 10:32 PM
> >>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> >><james.feist@linux.intel.com>
> >>Cc: openbmc@lists.ozlabs.org
> >>Subject: Re: [phosphor-pid-control] scaling issue
> >>
> >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> >><Hank.Liou@quantatw.com> wrote:
> >>>
> >>> Hi All,
> >>>
> >>>
> >>> After commit [1], I found my temp sensor reading would be re-scaled
> >>> by
> >>multiplying 1 over 255, making temperature into unfamiliar unit. Also
> >>the fan rpm reading would lie in [0,1] interval, letting the fan input
> >>to be 0 (since the input value of fan is from an integer array [2]). Are these
> >normal behaviors?
> >>Or do I miss something?
> >>
> >>Are you using dbus configuration or json?  If json, can you attach your config.
> >>Since you're saying it was working and now isn't, I'm assuming there's
> >>something about the config being treated differently with the code
> >>changes in an unexpected way.
> >
> >I found pid control will first read min and max from dbus and then (before
> >commit [1]) revise them if
> >
> >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> >
> >After value initialization, the min and max would be the ones in json file (Json
> >file [3]). However, after commit [1] the min and max values of config would
> >not be fed into the fan control process. The scaling issue is originated from
> >commit [4] with the aim to treat fan rpm as percentage. It seems that commit
> >[1] unexpectedly affects temp sensors in the sense that the temp is the
> >integer reading not percentage. Before commit [1], it would not re-scale the
> >temp reading since my min and max are 0 [6].
> >
> >
> >
> >There is another issue with commit [1]. Now the fan rpm would be something
> >like
> >
> >1500 / 20000 = 0.075
> >
> >where rpm max 20000 is from dbus. However the fan input function would
> >transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
> >not percentage. Is there something wrong?
> >
> >[1] https://github.com/openbmc/phosphor-pid-
> >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >[2] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L86
> >[3]
> >       {
> >            "name": "fan1",
> >            "type": "fan",
> >            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> >            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> >            "min": 0,
> >            "max": 255
> >        },
> >        {
> >            "name": "temp1",
> >            "type": "temp",
> >            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
> >            "writePath": "",
> >            "min": 0,
> >            "max": 0
> >        }
> >[4] https://github.com/openbmc/phosphor-pid-
> >control/commit/75eb769d351434547899186f73ff70ae00d7934a
> >[5] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> >r.cpp#L64
> >[6] https://github.com/openbmc/phosphor-pid-
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
> >ve.cpp#L158
> >
> >>
> >>>
> >>>
> >>> [1]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/commit/fc2e803f5d92569
> >>> 44e18c7c878a441606b1f121c
> >>>
> >>> [2]
> >>> https://github.com/openbmc/phosphor-pid-
> >>control/blob/a7ec8350d17b70153
> >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> >>>
> >>>
> >>> Thanks,
> >>>
> >>>
> >>> Hank Liou
> >>>
> >>> Quanta Computer Inc.
> >>>
> >>>
> >
> >Sincerely,
> >Hank

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

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi Patrick,
>
>
> Sorry for not clearly stating our problem. We have the following issue:
>
>
> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as input for temp sensor ->
>
> stepwise fan table output 100 duty -> full speed fan

Ok, so you're getting a dbus-based min/max value and you want to
ignore it?  In the json configuration, if you set the values to 0,
they are set to the default (0), so there'd be no clean way to know to
ignore dbus in this case, without adding a small check to only
consider min/max from dbus when sensor is not temp/margin.  Basically,
only care about min/max on dbus if type is "fan"

If that's right, James do you have a cycle to look at this one-liner?

>
>
> As a result, fan will be at full speed while temp is low. Before the commit fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will use config min/max, which is 0 in our case. This would not trigger scaling function, namely scaleSensorReading, in util.cpp. However, after such commit, min/max would be non-zero (-128/127 from DBus). This will trigger scaling function.
>
>
> [1] https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>
>
> Hank
>
>
> ________________________________
> From: Hank Liou (劉晉翰)
> Sent: Monday, September 2, 2019 4:52 PM
> To: Patrick Venture
> Cc: James Feist; openbmc@lists.ozlabs.org
> Subject: Re: [phosphor-pid-control] scaling issue
>
>
> Hi Patrick,
>
>
> Since we use phosphor-sel-logger [1] at the same time, we do assign min and max of temp sensors to Dbus (max: 127, min: -128). So in the present case, our temp value will be divided by 0.255 (also due to exponent is -3 here). This will cause re-scaling problem. Therefore there should be an statement to distinguish sensor types. If it is "temp", then one assigns 0 to the min and max from Dbus.
>
>
> [1] https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
>
>
> Hank
>
>
> ________________________________
> From: Patrick Venture <venture@google.com>
> Sent: Friday, August 30, 2019 1:47 AM
> To: Hank Liou (劉晉翰)
> Cc: James Feist; openbmc@lists.ozlabs.org
> Subject: Re: [phosphor-pid-control] scaling issue
>
> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
> the srcrev bump should propagate into openbmc/openbmc in a day or two.
>
> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
> >
> > Hi Patrick,
> >
> > I think it's OK to parse the config min&max for temp sensors.
> >
> > Any suggestion?
> >
> > Thanks,
> > Hank
> >
> > >-----Original Message-----
> > >From: openbmc [mailto:openbmc-
> > >bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
> > >(劉晉翰)
> > >Sent: Friday, August 23, 2019 4:31 PM
> > >To: Patrick Venture <venture@google.com>; James Feist
> > ><james.feist@linux.intel.com>
> > >Cc: openbmc@lists.ozlabs.org
> > >Subject: RE: [phosphor-pid-control] scaling issue
> > >
> > >Hi Patrick,
> > >
> > >>-----Original Message-----
> > >>From: Patrick Venture [mailto:venture@google.com]
> > >>Sent: Wednesday, August 21, 2019 10:32 PM
> > >>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> > >><james.feist@linux.intel.com>
> > >>Cc: openbmc@lists.ozlabs.org
> > >>Subject: Re: [phosphor-pid-control] scaling issue
> > >>
> > >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> > >><Hank.Liou@quantatw.com> wrote:
> > >>>
> > >>> Hi All,
> > >>>
> > >>>
> > >>> After commit [1], I found my temp sensor reading would be re-scaled
> > >>> by
> > >>multiplying 1 over 255, making temperature into unfamiliar unit. Also
> > >>the fan rpm reading would lie in [0,1] interval, letting the fan input
> > >>to be 0 (since the input value of fan is from an integer array [2]). Are these
> > >normal behaviors?
> > >>Or do I miss something?
> > >>
> > >>Are you using dbus configuration or json?  If json, can you attach your config.
> > >>Since you're saying it was working and now isn't, I'm assuming there's
> > >>something about the config being treated differently with the code
> > >>changes in an unexpected way.
> > >
> > >I found pid control will first read min and max from dbus and then (before
> > >commit [1]) revise them if
> > >
> > >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> > >
> > >After value initialization, the min and max would be the ones in json file (Json
> > >file [3]). However, after commit [1] the min and max values of config would
> > >not be fed into the fan control process. The scaling issue is originated from
> > >commit [4] with the aim to treat fan rpm as percentage. It seems that commit
> > >[1] unexpectedly affects temp sensors in the sense that the temp is the
> > >integer reading not percentage. Before commit [1], it would not re-scale the
> > >temp reading since my min and max are 0 [6].
> > >
> > >
> > >
> > >There is another issue with commit [1]. Now the fan rpm would be something
> > >like
> > >
> > >1500 / 20000 = 0.075
> > >
> > >where rpm max 20000 is from dbus. However the fan input function would
> > >transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
> > >not percentage. Is there something wrong?
> > >
> > >[1] https://github.com/openbmc/phosphor-pid-
> > >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> > >[2] https://github.com/openbmc/phosphor-pid-
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> > >r.cpp#L86
> > >[3]
> > >       {
> > >            "name": "fan1",
> > >            "type": "fan",
> > >            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> > >            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> > >            "min": 0,
> > >            "max": 255
> > >        },
> > >        {
> > >            "name": "temp1",
> > >            "type": "temp",
> > >            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
> > >            "writePath": "",
> > >            "min": 0,
> > >            "max": 0
> > >        }
> > >[4] https://github.com/openbmc/phosphor-pid-
> > >control/commit/75eb769d351434547899186f73ff70ae00d7934a
> > >[5] https://github.com/openbmc/phosphor-pid-
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> > >r.cpp#L64
> > >[6] https://github.com/openbmc/phosphor-pid-
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
> > >ve.cpp#L158
> > >
> > >>
> > >>>
> > >>>
> > >>> [1]
> > >>> https://github.com/openbmc/phosphor-pid-
> > >>control/commit/fc2e803f5d92569
> > >>> 44e18c7c878a441606b1f121c
> > >>>
> > >>> [2]
> > >>> https://github.com/openbmc/phosphor-pid-
> > >>control/blob/a7ec8350d17b70153
> > >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> > >>>
> > >>>
> > >>> Thanks,
> > >>>
> > >>>
> > >>> Hank Liou
> > >>>
> > >>> Quanta Computer Inc.
> > >>>
> > >>>
> > >
> > >Sincerely,
> > >Hank

Re: [phosphor-pid-control] scaling issue

[Kun Yi]

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

Sorry about the regression issue, Hank.
IIRC I proposed to have separate groups of min/max values in D-Bus and json
files since they convey different semantics.
At the end the min/max value was merged in the current way, because the
assumption that min/max values in json is only used for fan pwm and nothing
else -- which obviously is not true now.
Should we revisit the approach? Having something like 'fanPwmScale' as a
separate config param is a cleaner approach in my opinion.

On Mon, Sep 9, 2019 at 9:59 AM Patrick Venture <venture@google.com> wrote:

> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>
> wrote:
> >
> > Hi Patrick,
> >
> >
> > Sorry for not clearly stating our problem. We have the following issue:
> >
> >
> > temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as
> input for temp sensor ->
> >
> > stepwise fan table output 100 duty -> full speed fan
>
> Ok, so you're getting a dbus-based min/max value and you want to
> ignore it?  In the json configuration, if you set the values to 0,
> they are set to the default (0), so there'd be no clean way to know to
> ignore dbus in this case, without adding a small check to only
> consider min/max from dbus when sensor is not temp/margin.  Basically,
> only care about min/max on dbus if type is "fan"
>
> If that's right, James do you have a cycle to look at this one-liner?
>
> >
> >
> > As a result, fan will be at full speed while temp is low. Before the
> commit fc2e803 [1], this won't happen. The root cause is that, before
> fc2e803, pid will use config min/max, which is 0 in our case. This would
> not trigger scaling function, namely scaleSensorReading, in util.cpp.
> However, after such commit, min/max would be non-zero (-128/127 from DBus).
> This will trigger scaling function.
> >
> >
> > [1]
> https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >
> >
> > Hank
> >
> >
> > ________________________________
> > From: Hank Liou (劉晉翰)
> > Sent: Monday, September 2, 2019 4:52 PM
> > To: Patrick Venture
> > Cc: James Feist; openbmc@lists.ozlabs.org
> > Subject: Re: [phosphor-pid-control] scaling issue
> >
> >
> > Hi Patrick,
> >
> >
> > Since we use phosphor-sel-logger [1] at the same time, we do assign min
> and max of temp sensors to Dbus (max: 127, min: -128). So in the present
> case, our temp value will be divided by 0.255 (also due to exponent is -3
> here). This will cause re-scaling problem. Therefore there should be an
> statement to distinguish sensor types. If it is "temp", then one assigns 0
> to the min and max from Dbus.
> >
> >
> > [1]
> https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
> >
> >
> > Hank
> >
> >
> > ________________________________
> > From: Patrick Venture <venture@google.com>
> > Sent: Friday, August 30, 2019 1:47 AM
> > To: Hank Liou (劉晉翰)
> > Cc: James Feist; openbmc@lists.ozlabs.org
> > Subject: Re: [phosphor-pid-control] scaling issue
> >
> > PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
> > the srcrev bump should propagate into openbmc/openbmc in a day or two.
> >
> > On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>
> wrote:
> > >
> > > Hi Patrick,
> > >
> > > I think it's OK to parse the config min&max for temp sensors.
> > >
> > > Any suggestion?
> > >
> > > Thanks,
> > > Hank
> > >
> > > >-----Original Message-----
> > > >From: openbmc [mailto:openbmc-
> > > >bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank
> Liou
> > > >(劉晉翰)
> > > >Sent: Friday, August 23, 2019 4:31 PM
> > > >To: Patrick Venture <venture@google.com>; James Feist
> > > ><james.feist@linux.intel.com>
> > > >Cc: openbmc@lists.ozlabs.org
> > > >Subject: RE: [phosphor-pid-control] scaling issue
> > > >
> > > >Hi Patrick,
> > > >
> > > >>-----Original Message-----
> > > >>From: Patrick Venture [mailto:venture@google.com]
> > > >>Sent: Wednesday, August 21, 2019 10:32 PM
> > > >>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> > > >><james.feist@linux.intel.com>
> > > >>Cc: openbmc@lists.ozlabs.org
> > > >>Subject: Re: [phosphor-pid-control] scaling issue
> > > >>
> > > >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> > > >><Hank.Liou@quantatw.com> wrote:
> > > >>>
> > > >>> Hi All,
> > > >>>
> > > >>>
> > > >>> After commit [1], I found my temp sensor reading would be re-scaled
> > > >>> by
> > > >>multiplying 1 over 255, making temperature into unfamiliar unit. Also
> > > >>the fan rpm reading would lie in [0,1] interval, letting the fan
> input
> > > >>to be 0 (since the input value of fan is from an integer array [2]).
> Are these
> > > >normal behaviors?
> > > >>Or do I miss something?
> > > >>
> > > >>Are you using dbus configuration or json?  If json, can you attach
> your config.
> > > >>Since you're saying it was working and now isn't, I'm assuming
> there's
> > > >>something about the config being treated differently with the code
> > > >>changes in an unexpected way.
> > > >
> > > >I found pid control will first read min and max from dbus and then
> (before
> > > >commit [1]) revise them if
> > > >
> > > >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> > > >
> > > >After value initialization, the min and max would be the ones in json
> file (Json
> > > >file [3]). However, after commit [1] the min and max values of config
> would
> > > >not be fed into the fan control process. The scaling issue is
> originated from
> > > >commit [4] with the aim to treat fan rpm as percentage. It seems that
> commit
> > > >[1] unexpectedly affects temp sensors in the sense that the temp is
> the
> > > >integer reading not percentage. Before commit [1], it would not
> re-scale the
> > > >temp reading since my min and max are 0 [6].
> > > >
> > > >
> > > >
> > > >There is another issue with commit [1]. Now the fan rpm would be
> something
> > > >like
> > > >
> > > >1500 / 20000 = 0.075
> > > >
> > > >where rpm max 20000 is from dbus. However the fan input function would
> > > >transfer double into int, which is 0 for 0.075 (see [5]). Hence the
> fan input is 0
> > > >not percentage. Is there something wrong?
> > > >
> > > >[1] https://github.com/openbmc/phosphor-pid-
> > > >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> > > >[2] https://github.com/openbmc/phosphor-pid-
> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> > > >r.cpp#L86
> > > >[3]
> > > >       {
> > > >            "name": "fan1",
> > > >            "type": "fan",
> > > >            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> > > >            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> > > >            "min": 0,
> > > >            "max": 255
> > > >        },
> > > >        {
> > > >            "name": "temp1",
> > > >            "type": "temp",
> > > >            "readPath":
> "/xyz/openbmc_project/sensors/temperature/temp1",
> > > >            "writePath": "",
> > > >            "min": 0,
> > > >            "max": 0
> > > >        }
> > > >[4] https://github.com/openbmc/phosphor-pid-
> > > >control/commit/75eb769d351434547899186f73ff70ae00d7934a
> > > >[5] https://github.com/openbmc/phosphor-pid-
> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
> > > >r.cpp#L64
> > > >[6] https://github.com/openbmc/phosphor-pid-
> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
> > > >ve.cpp#L158
> > > >
> > > >>
> > > >>>
> > > >>>
> > > >>> [1]
> > > >>> https://github.com/openbmc/phosphor-pid-
> > > >>control/commit/fc2e803f5d92569
> > > >>> 44e18c7c878a441606b1f121c
> > > >>>
> > > >>> [2]
> > > >>> https://github.com/openbmc/phosphor-pid-
> > > >>control/blob/a7ec8350d17b70153
> > > >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> > > >>>
> > > >>>
> > > >>> Thanks,
> > > >>>
> > > >>>
> > > >>> Hank Liou
> > > >>>
> > > >>> Quanta Computer Inc.
> > > >>>
> > > >>>
> > > >
> > > >Sincerely,
> > > >Hank
>


-- 
Regards,
Kun

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

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Mon, Sep 9, 2019 at 10:12 AM Kun Yi <kunyi@google.com> wrote:
>
> Sorry about the regression issue, Hank.
> IIRC I proposed to have separate groups of min/max values in D-Bus and json files since they convey different semantics.
> At the end the min/max value was merged in the current way, because the assumption that min/max values in json is only used for fan pwm and nothing else -- which obviously is not true now.
> Should we revisit the approach? Having something like 'fanPwmScale' as a separate config param is a cleaner approach in my opinion.

Kun, it was my understanding that the issue they're seeing is that
they're reading min/max from dbus.  But maybe I have if flipped.

I disagree that fanPwmScale is cleaner because it's now specific to
fan pwm.  In theory, using min/max could have some future purpose.
Perhaps minWrite/maxWrite would be more ... separate.  However, I
think the issue seen is unrelated.

>
> On Mon, Sep 9, 2019 at 9:59 AM Patrick Venture <venture@google.com> wrote:
>>
>> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>> >
>> > Hi Patrick,
>> >
>> >
>> > Sorry for not clearly stating our problem. We have the following issue:
>> >
>> >
>> > temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as input for temp sensor ->
>> >
>> > stepwise fan table output 100 duty -> full speed fan
>>
>> Ok, so you're getting a dbus-based min/max value and you want to
>> ignore it?  In the json configuration, if you set the values to 0,
>> they are set to the default (0), so there'd be no clean way to know to
>> ignore dbus in this case, without adding a small check to only
>> consider min/max from dbus when sensor is not temp/margin.  Basically,
>> only care about min/max on dbus if type is "fan"
>>
>> If that's right, James do you have a cycle to look at this one-liner?
>>
>> >
>> >
>> > As a result, fan will be at full speed while temp is low. Before the commit fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will use config min/max, which is 0 in our case. This would not trigger scaling function, namely scaleSensorReading, in util.cpp. However, after such commit, min/max would be non-zero (-128/127 from DBus). This will trigger scaling function.
>> >
>> >
>> > [1] https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>> >
>> >
>> > Hank
>> >
>> >
>> > ________________________________
>> > From: Hank Liou (劉晉翰)
>> > Sent: Monday, September 2, 2019 4:52 PM
>> > To: Patrick Venture
>> > Cc: James Feist; openbmc@lists.ozlabs.org
>> > Subject: Re: [phosphor-pid-control] scaling issue
>> >
>> >
>> > Hi Patrick,
>> >
>> >
>> > Since we use phosphor-sel-logger [1] at the same time, we do assign min and max of temp sensors to Dbus (max: 127, min: -128). So in the present case, our temp value will be divided by 0.255 (also due to exponent is -3 here). This will cause re-scaling problem. Therefore there should be an statement to distinguish sensor types. If it is "temp", then one assigns 0 to the min and max from Dbus.
>> >
>> >
>> > [1] https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
>> >
>> >
>> > Hank
>> >
>> >
>> > ________________________________
>> > From: Patrick Venture <venture@google.com>
>> > Sent: Friday, August 30, 2019 1:47 AM
>> > To: Hank Liou (劉晉翰)
>> > Cc: James Feist; openbmc@lists.ozlabs.org
>> > Subject: Re: [phosphor-pid-control] scaling issue
>> >
>> > PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
>> > the srcrev bump should propagate into openbmc/openbmc in a day or two.
>> >
>> > On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>> > >
>> > > Hi Patrick,
>> > >
>> > > I think it's OK to parse the config min&max for temp sensors.
>> > >
>> > > Any suggestion?
>> > >
>> > > Thanks,
>> > > Hank
>> > >
>> > > >-----Original Message-----
>> > > >From: openbmc [mailto:openbmc-
>> > > >bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
>> > > >(劉晉翰)
>> > > >Sent: Friday, August 23, 2019 4:31 PM
>> > > >To: Patrick Venture <venture@google.com>; James Feist
>> > > ><james.feist@linux.intel.com>
>> > > >Cc: openbmc@lists.ozlabs.org
>> > > >Subject: RE: [phosphor-pid-control] scaling issue
>> > > >
>> > > >Hi Patrick,
>> > > >
>> > > >>-----Original Message-----
>> > > >>From: Patrick Venture [mailto:venture@google.com]
>> > > >>Sent: Wednesday, August 21, 2019 10:32 PM
>> > > >>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
>> > > >><james.feist@linux.intel.com>
>> > > >>Cc: openbmc@lists.ozlabs.org
>> > > >>Subject: Re: [phosphor-pid-control] scaling issue
>> > > >>
>> > > >>On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>> > > >><Hank.Liou@quantatw.com> wrote:
>> > > >>>
>> > > >>> Hi All,
>> > > >>>
>> > > >>>
>> > > >>> After commit [1], I found my temp sensor reading would be re-scaled
>> > > >>> by
>> > > >>multiplying 1 over 255, making temperature into unfamiliar unit. Also
>> > > >>the fan rpm reading would lie in [0,1] interval, letting the fan input
>> > > >>to be 0 (since the input value of fan is from an integer array [2]). Are these
>> > > >normal behaviors?
>> > > >>Or do I miss something?
>> > > >>
>> > > >>Are you using dbus configuration or json?  If json, can you attach your config.
>> > > >>Since you're saying it was working and now isn't, I'm assuming there's
>> > > >>something about the config being treated differently with the code
>> > > >>changes in an unexpected way.
>> > > >
>> > > >I found pid control will first read min and max from dbus and then (before
>> > > >commit [1]) revise them if
>> > > >
>> > > >info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
>> > > >
>> > > >After value initialization, the min and max would be the ones in json file (Json
>> > > >file [3]). However, after commit [1] the min and max values of config would
>> > > >not be fed into the fan control process. The scaling issue is originated from
>> > > >commit [4] with the aim to treat fan rpm as percentage. It seems that commit
>> > > >[1] unexpectedly affects temp sensors in the sense that the temp is the
>> > > >integer reading not percentage. Before commit [1], it would not re-scale the
>> > > >temp reading since my min and max are 0 [6].
>> > > >
>> > > >
>> > > >
>> > > >There is another issue with commit [1]. Now the fan rpm would be something
>> > > >like
>> > > >
>> > > >1500 / 20000 = 0.075
>> > > >
>> > > >where rpm max 20000 is from dbus. However the fan input function would
>> > > >transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
>> > > >not percentage. Is there something wrong?
>> > > >
>> > > >[1] https://github.com/openbmc/phosphor-pid-
>> > > >control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>> > > >[2] https://github.com/openbmc/phosphor-pid-
>> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>> > > >r.cpp#L86
>> > > >[3]
>> > > >       {
>> > > >            "name": "fan1",
>> > > >            "type": "fan",
>> > > >            "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
>> > > >            "writePath": "/sys/class/hwmon/hwmon1/pwm1",
>> > > >            "min": 0,
>> > > >            "max": 255
>> > > >        },
>> > > >        {
>> > > >            "name": "temp1",
>> > > >            "type": "temp",
>> > > >            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
>> > > >            "writePath": "",
>> > > >            "min": 0,
>> > > >            "max": 0
>> > > >        }
>> > > >[4] https://github.com/openbmc/phosphor-pid-
>> > > >control/commit/75eb769d351434547899186f73ff70ae00d7934a
>> > > >[5] https://github.com/openbmc/phosphor-pid-
>> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>> > > >r.cpp#L64
>> > > >[6] https://github.com/openbmc/phosphor-pid-
>> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
>> > > >ve.cpp#L158
>> > > >
>> > > >>
>> > > >>>
>> > > >>>
>> > > >>> [1]
>> > > >>> https://github.com/openbmc/phosphor-pid-
>> > > >>control/commit/fc2e803f5d92569
>> > > >>> 44e18c7c878a441606b1f121c
>> > > >>>
>> > > >>> [2]
>> > > >>> https://github.com/openbmc/phosphor-pid-
>> > > >>control/blob/a7ec8350d17b70153
>> > > >>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>> > > >>>
>> > > >>>
>> > > >>> Thanks,
>> > > >>>
>> > > >>>
>> > > >>> Hank Liou
>> > > >>>
>> > > >>> Quanta Computer Inc.
>> > > >>>
>> > > >>>
>> > > >
>> > > >Sincerely,
>> > > >Hank
>
>
>
> --
> Regards,
> Kun

Re: [phosphor-pid-control] scaling issue

[James Feist]

On 9/9/19 9:57 AM, Patrick Venture wrote:
> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>>
>> Hi Patrick,
>>
>>
>> Sorry for not clearly stating our problem. We have the following issue:
>>
>>
>> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as input for temp sensor ->
>>
>> stepwise fan table output 100 duty -> full speed fan
> 
> Ok, so you're getting a dbus-based min/max value and you want to
> ignore it?  

What is providing the dbus-based min/max?

> In the json configuration, if you set the values to 0,
> they are set to the default (0), so there'd be no clean way to know to
> ignore dbus in this case, without adding a small check to only
> consider min/max from dbus when sensor is not temp/margin.  Basically,
> only care about min/max on dbus if type is "fan"

Why is the dbus-based min/max even there if you don't plan on using it?

> 
> If that's right, James do you have a cycle to look at this one-liner?
> 
>>
>>
>> As a result, fan will be at full speed while temp is low. Before the commit fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will use config min/max, which is 0 in our case. This would not trigger scaling function, namely scaleSensorReading, in util.cpp. However, after such commit, min/max would be non-zero (-128/127 from DBus). This will trigger scaling function.
>>
>>
>> [1] https://github.com/openbmc/phosphor-pid-control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>>
>>
>> Hank
>>
>>
>> ________________________________
>> From: Hank Liou (劉晉翰)
>> Sent: Monday, September 2, 2019 4:52 PM
>> To: Patrick Venture
>> Cc: James Feist; openbmc@lists.ozlabs.org
>> Subject: Re: [phosphor-pid-control] scaling issue
>>
>>
>> Hi Patrick,
>>
>>
>> Since we use phosphor-sel-logger [1] at the same time, we do assign min and max of temp sensors to Dbus (max: 127, min: -128). So in the present case, our temp value will be divided by 0.255 (also due to exponent is -3 here). This will cause re-scaling problem. Therefore there should be an statement to distinguish sensor types. If it is "temp", then one assigns 0 to the min and max from Dbus.
>>
>>
>> [1] https://github.com/openbmc/phosphor-sel-logger/blob/3d300fca24b30864b3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
>>
>>
>> Hank
>>
>>
>> ________________________________
>> From: Patrick Venture <venture@google.com>
>> Sent: Friday, August 30, 2019 1:47 AM
>> To: Hank Liou (劉晉翰)
>> Cc: James Feist; openbmc@lists.ozlabs.org
>> Subject: Re: [phosphor-pid-control] scaling issue
>>
>> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
>> the srcrev bump should propagate into openbmc/openbmc in a day or two.
>>
>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>>>
>>> Hi Patrick,
>>>
>>> I think it's OK to parse the config min&max for temp sensors.
>>>
>>> Any suggestion?
>>>
>>> Thanks,
>>> Hank
>>>
>>>> -----Original Message-----
>>>> From: openbmc [mailto:openbmc-
>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank Liou
>>>> (劉晉翰)
>>>> Sent: Friday, August 23, 2019 4:31 PM
>>>> To: Patrick Venture <venture@google.com>; James Feist
>>>> <james.feist@linux.intel.com>
>>>> Cc: openbmc@lists.ozlabs.org
>>>> Subject: RE: [phosphor-pid-control] scaling issue
>>>>
>>>> Hi Patrick,
>>>>
>>>>> -----Original Message-----
>>>>> From: Patrick Venture [mailto:venture@google.com]
>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
>>>>> <james.feist@linux.intel.com>
>>>>> Cc: openbmc@lists.ozlabs.org
>>>>> Subject: Re: [phosphor-pid-control] scaling issue
>>>>>
>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>>>>> <Hank.Liou@quantatw.com> wrote:
>>>>>>
>>>>>> Hi All,
>>>>>>
>>>>>>
>>>>>> After commit [1], I found my temp sensor reading would be re-scaled
>>>>>> by
>>>>> multiplying 1 over 255, making temperature into unfamiliar unit. Also
>>>>> the fan rpm reading would lie in [0,1] interval, letting the fan input
>>>>> to be 0 (since the input value of fan is from an integer array [2]). Are these
>>>> normal behaviors?
>>>>> Or do I miss something?
>>>>>
>>>>> Are you using dbus configuration or json?  If json, can you attach your config.
>>>>> Since you're saying it was working and now isn't, I'm assuming there's
>>>>> something about the config being treated differently with the code
>>>>> changes in an unexpected way.
>>>>
>>>> I found pid control will first read min and max from dbus and then (before
>>>> commit [1]) revise them if
>>>>
>>>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
>>>>
>>>> After value initialization, the min and max would be the ones in json file (Json
>>>> file [3]). However, after commit [1] the min and max values of config would
>>>> not be fed into the fan control process. The scaling issue is originated from
>>>> commit [4] with the aim to treat fan rpm as percentage. It seems that commit
>>>> [1] unexpectedly affects temp sensors in the sense that the temp is the
>>>> integer reading not percentage. Before commit [1], it would not re-scale the
>>>> temp reading since my min and max are 0 [6].
>>>>
>>>>
>>>>
>>>> There is another issue with commit [1]. Now the fan rpm would be something
>>>> like
>>>>
>>>> 1500 / 20000 = 0.075
>>>>
>>>> where rpm max 20000 is from dbus. However the fan input function would
>>>> transfer double into int, which is 0 for 0.075 (see [5]). Hence the fan input is 0
>>>> not percentage. Is there something wrong?
>>>>
>>>> [1] https://github.com/openbmc/phosphor-pid-
>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>>>> [2] https://github.com/openbmc/phosphor-pid-
>>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>>>> r.cpp#L86
>>>> [3]
>>>>        {
>>>>             "name": "fan1",
>>>>             "type": "fan",
>>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
>>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
>>>>             "min": 0,
>>>>             "max": 255
>>>>         },
>>>>         {
>>>>             "name": "temp1",
>>>>             "type": "temp",
>>>>             "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
>>>>             "writePath": "",
>>>>             "min": 0,
>>>>             "max": 0
>>>>         }
>>>> [4] https://github.com/openbmc/phosphor-pid-
>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
>>>> [5] https://github.com/openbmc/phosphor-pid-
>>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontrolle
>>>> r.cpp#L64
>>>> [6] https://github.com/openbmc/phosphor-pid-
>>>> control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspassi
>>>> ve.cpp#L158
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> [1]
>>>>>> https://github.com/openbmc/phosphor-pid-
>>>>> control/commit/fc2e803f5d92569
>>>>>> 44e18c7c878a441606b1f121c
>>>>>>
>>>>>> [2]
>>>>>> https://github.com/openbmc/phosphor-pid-
>>>>> control/blob/a7ec8350d17b70153
>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>
>>>>>> Hank Liou
>>>>>>
>>>>>> Quanta Computer Inc.
>>>>>>
>>>>>>
>>>>
>>>> Sincerely,
>>>> Hank

RE: [phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

>-----Original Message-----
>From: James Feist [mailto:james.feist@linux.intel.com]
>Sent: Tuesday, September 10, 2019 1:27 AM
>To: Patrick Venture <venture@google.com>; Hank Liou (劉晉翰)
><Hank.Liou@quantatw.com>
>Cc: openbmc@lists.ozlabs.org
>Subject: Re: [phosphor-pid-control] scaling issue
>
>On 9/9/19 9:57 AM, Patrick Venture wrote:
>> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰)
><Hank.Liou@quantatw.com> wrote:
>>>
>>> Hi Patrick,
>>>
>>>
>>> Sorry for not clearly stating our problem. We have the following issue:
>>>
>>>
>>> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as
>>> input for temp sensor ->
>>>
>>> stepwise fan table output 100 duty -> full speed fan
>>
>> Ok, so you're getting a dbus-based min/max value and you want to
>> ignore it?

Yes, we want to ignore it.

>
>What is providing the dbus-based min/max?

We use dbus min/max for phosphor-sel-logger utility. The dbus min/max is provided by config file of phosphor-hwmon.
In our case, they are -128/127 respectively.

I have an observation:
I found that while fan readpath is of the form "/sys/...", the fan input would not be rescaled. The same is for other sensors since the input value would not enter rescaling function.
However, in our case we don't have a sys path for some thermal sensors.

>
>> In the json configuration, if you set the values to 0, they are set to
>> the default (0), so there'd be no clean way to know to ignore dbus in
>> this case, without adding a small check to only consider min/max from
>> dbus when sensor is not temp/margin.  Basically, only care about
>> min/max on dbus if type is "fan"
>

I agree with the option to add a check since the min/max is only for fan now.
 
>Why is the dbus-based min/max even there if you don't plan on using it?
>
>>
>> If that's right, James do you have a cycle to look at this one-liner?
>>
>>>
>>>
>>> As a result, fan will be at full speed while temp is low. Before the commit
>fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will
>use config min/max, which is 0 in our case. This would not trigger scaling
>function, namely scaleSensorReading, in util.cpp. However, after such commit,
>min/max would be non-zero (-128/127 from DBus). This will trigger scaling
>function.
>>>
>>>
>>> [1]
>>> https://github.com/openbmc/phosphor-pid-
>control/commit/fc2e803f5d9256
>>> 944e18c7c878a441606b1f121c
>>>
>>>
>>> Hank
>>>
>>>
>>> ________________________________
>>> From: Hank Liou (劉晉翰)
>>> Sent: Monday, September 2, 2019 4:52 PM
>>> To: Patrick Venture
>>> Cc: James Feist; openbmc@lists.ozlabs.org
>>> Subject: Re: [phosphor-pid-control] scaling issue
>>>
>>>
>>> Hi Patrick,
>>>
>>>
>>> Since we use phosphor-sel-logger [1] at the same time, we do assign min
>and max of temp sensors to Dbus (max: 127, min: -128). So in the present case,
>our temp value will be divided by 0.255 (also due to exponent is -3 here). This
>will cause re-scaling problem. Therefore there should be an statement to
>distinguish sensor types. If it is "temp", then one assigns 0 to the min and max
>from Dbus.
>>>
>>>
>>> [1]
>>> https://github.com/openbmc/phosphor-sel-
>logger/blob/3d300fca24b30864b
>>> 3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
>>>
>>>
>>> Hank
>>>
>>>
>>> ________________________________
>>> From: Patrick Venture <venture@google.com>
>>> Sent: Friday, August 30, 2019 1:47 AM
>>> To: Hank Liou (劉晉翰)
>>> Cc: James Feist; openbmc@lists.ozlabs.org
>>> Subject: Re: [phosphor-pid-control] scaling issue
>>>
>>> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
>>> the srcrev bump should propagate into openbmc/openbmc in a day or two.
>>>
>>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰)
><Hank.Liou@quantatw.com> wrote:
>>>>
>>>> Hi Patrick,
>>>>
>>>> I think it's OK to parse the config min&max for temp sensors.
>>>>
>>>> Any suggestion?
>>>>
>>>> Thanks,
>>>> Hank
>>>>
>>>>> -----Original Message-----
>>>>> From: openbmc [mailto:openbmc-
>>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank
>>>>> bounces+Liou
>>>>> (劉晉翰)
>>>>> Sent: Friday, August 23, 2019 4:31 PM
>>>>> To: Patrick Venture <venture@google.com>; James Feist
>>>>> <james.feist@linux.intel.com>
>>>>> Cc: openbmc@lists.ozlabs.org
>>>>> Subject: RE: [phosphor-pid-control] scaling issue
>>>>>
>>>>> Hi Patrick,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Patrick Venture [mailto:venture@google.com]
>>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
>>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
>>>>>> <james.feist@linux.intel.com>
>>>>>> Cc: openbmc@lists.ozlabs.org
>>>>>> Subject: Re: [phosphor-pid-control] scaling issue
>>>>>>
>>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>>>>>> <Hank.Liou@quantatw.com> wrote:
>>>>>>>
>>>>>>> Hi All,
>>>>>>>
>>>>>>>
>>>>>>> After commit [1], I found my temp sensor reading would be
>>>>>>> re-scaled by
>>>>>> multiplying 1 over 255, making temperature into unfamiliar unit.
>>>>>> Also the fan rpm reading would lie in [0,1] interval, letting the
>>>>>> fan input to be 0 (since the input value of fan is from an integer
>>>>>> array [2]). Are these
>>>>> normal behaviors?
>>>>>> Or do I miss something?
>>>>>>
>>>>>> Are you using dbus configuration or json?  If json, can you attach your
>config.
>>>>>> Since you're saying it was working and now isn't, I'm assuming
>>>>>> there's something about the config being treated differently with
>>>>>> the code changes in an unexpected way.
>>>>>
>>>>> I found pid control will first read min and max from dbus and then
>>>>> (before commit [1]) revise them if
>>>>>
>>>>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
>>>>>
>>>>> After value initialization, the min and max would be the ones in
>>>>> json file (Json file [3]). However, after commit [1] the min and
>>>>> max values of config would not be fed into the fan control process.
>>>>> The scaling issue is originated from commit [4] with the aim to
>>>>> treat fan rpm as percentage. It seems that commit [1] unexpectedly
>>>>> affects temp sensors in the sense that the temp is the integer
>>>>> reading not percentage. Before commit [1], it would not re-scale the
>temp reading since my min and max are 0 [6].
>>>>>
>>>>>
>>>>>
>>>>> There is another issue with commit [1]. Now the fan rpm would be
>>>>> something like
>>>>>
>>>>> 1500 / 20000 = 0.075
>>>>>
>>>>> where rpm max 20000 is from dbus. However the fan input function
>>>>> would transfer double into int, which is 0 for 0.075 (see [5]).
>>>>> Hence the fan input is 0 not percentage. Is there something wrong?
>>>>>
>>>>> [1] https://github.com/openbmc/phosphor-pid-
>>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>>>>> [2] https://github.com/openbmc/phosphor-pid-
>>>>>
>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
>>>>> lle
>>>>> r.cpp#L86
>>>>> [3]
>>>>>        {
>>>>>             "name": "fan1",
>>>>>             "type": "fan",
>>>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
>>>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
>>>>>             "min": 0,
>>>>>             "max": 255
>>>>>         },
>>>>>         {
>>>>>             "name": "temp1",
>>>>>             "type": "temp",
>>>>>             "readPath":
>"/xyz/openbmc_project/sensors/temperature/temp1",
>>>>>             "writePath": "",
>>>>>             "min": 0,
>>>>>             "max": 0
>>>>>         }
>>>>> [4] https://github.com/openbmc/phosphor-pid-
>>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
>>>>> [5] https://github.com/openbmc/phosphor-pid-
>>>>>
>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
>>>>> lle
>>>>> r.cpp#L64
>>>>> [6] https://github.com/openbmc/phosphor-pid-
>>>>>
>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspass
>>>>> i
>>>>> ve.cpp#L158
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>> https://github.com/openbmc/phosphor-pid-
>>>>>> control/commit/fc2e803f5d92569
>>>>>>> 44e18c7c878a441606b1f121c
>>>>>>>
>>>>>>> [2]
>>>>>>> https://github.com/openbmc/phosphor-pid-
>>>>>> control/blob/a7ec8350d17b70153
>>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>
>>>>>>> Hank Liou
>>>>>>>
>>>>>>> Quanta Computer Inc.
>>>>>>>
>>>>>>>
>>>>>
>>>>> Sincerely,
>>>>> Hank

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Tue, Sep 10, 2019 at 1:02 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> >-----Original Message-----
> >From: James Feist [mailto:james.feist@linux.intel.com]
> >Sent: Tuesday, September 10, 2019 1:27 AM
> >To: Patrick Venture <venture@google.com>; Hank Liou (劉晉翰)
> ><Hank.Liou@quantatw.com>
> >Cc: openbmc@lists.ozlabs.org
> >Subject: Re: [phosphor-pid-control] scaling issue
> >
> >On 9/9/19 9:57 AM, Patrick Venture wrote:
> >> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰)
> ><Hank.Liou@quantatw.com> wrote:
> >>>
> >>> Hi Patrick,
> >>>
> >>>
> >>> Sorry for not clearly stating our problem. We have the following issue:
> >>>
> >>>
> >>> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as
> >>> input for temp sensor ->
> >>>
> >>> stepwise fan table output 100 duty -> full speed fan
> >>
> >> Ok, so you're getting a dbus-based min/max value and you want to
> >> ignore it?
>
> Yes, we want to ignore it.

It looks like the sensor value scaling should be ignored for non-fans.
Per https://github.com/openbmc/phosphor-pid-control/commit/75eb769d351434547899186f73ff70ae00d7934a,
this change was only meant to deal with a fan case where the goal
looks like it was to treat the fan values as percentages on reading
the sensors, and not only on writing them by leveraging the min/max
json sometimes?

I can change the dbuspassive constructor to set the values back to
their input values (ignoring dbus) if the type is not "fan", but I'm
not sure that makes sense because the real use-case isn't clear to me,
and I know the scaling from dbus is gone:
https://github.com/openbmc/phosphor-pid-control/search?utf8=%E2%9C%93&q=inheritValueFromDbus&type=

I'll submit the patch real quick, but I'm not sure it's the right
long-term fix.  I'd like to clean this up if possible so that we don't
leverage "unused" features of a configuration and instead explicitly
use them (or not).

>
> >
> >What is providing the dbus-based min/max?
>
> We use dbus min/max for phosphor-sel-logger utility. The dbus min/max is provided by config file of phosphor-hwmon.
> In our case, they are -128/127 respectively.
>
> I have an observation:
> I found that while fan readpath is of the form "/sys/...", the fan input would not be rescaled. The same is for other sensors since the input value would not enter rescaling function.
> However, in our case we don't have a sys path for some thermal sensors.
>
> >
> >> In the json configuration, if you set the values to 0, they are set to
> >> the default (0), so there'd be no clean way to know to ignore dbus in
> >> this case, without adding a small check to only consider min/max from
> >> dbus when sensor is not temp/margin.  Basically, only care about
> >> min/max on dbus if type is "fan"
> >
>
> I agree with the option to add a check since the min/max is only for fan now.
>
> >Why is the dbus-based min/max even there if you don't plan on using it?
> >
> >>
> >> If that's right, James do you have a cycle to look at this one-liner?
> >>
> >>>
> >>>
> >>> As a result, fan will be at full speed while temp is low. Before the commit
> >fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will
> >use config min/max, which is 0 in our case. This would not trigger scaling
> >function, namely scaleSensorReading, in util.cpp. However, after such commit,
> >min/max would be non-zero (-128/127 from DBus). This will trigger scaling
> >function.
> >>>
> >>>
> >>> [1]
> >>> https://github.com/openbmc/phosphor-pid-
> >control/commit/fc2e803f5d9256
> >>> 944e18c7c878a441606b1f121c
> >>>
> >>>
> >>> Hank
> >>>
> >>>
> >>> ________________________________
> >>> From: Hank Liou (劉晉翰)
> >>> Sent: Monday, September 2, 2019 4:52 PM
> >>> To: Patrick Venture
> >>> Cc: James Feist; openbmc@lists.ozlabs.org
> >>> Subject: Re: [phosphor-pid-control] scaling issue
> >>>
> >>>
> >>> Hi Patrick,
> >>>
> >>>
> >>> Since we use phosphor-sel-logger [1] at the same time, we do assign min
> >and max of temp sensors to Dbus (max: 127, min: -128). So in the present case,
> >our temp value will be divided by 0.255 (also due to exponent is -3 here). This
> >will cause re-scaling problem. Therefore there should be an statement to
> >distinguish sensor types. If it is "temp", then one assigns 0 to the min and max
> >from Dbus.
> >>>
> >>>
> >>> [1]
> >>> https://github.com/openbmc/phosphor-sel-
> >logger/blob/3d300fca24b30864b
> >>> 3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
> >>>
> >>>
> >>> Hank
> >>>
> >>>
> >>> ________________________________
> >>> From: Patrick Venture <venture@google.com>
> >>> Sent: Friday, August 30, 2019 1:47 AM
> >>> To: Hank Liou (劉晉翰)
> >>> Cc: James Feist; openbmc@lists.ozlabs.org
> >>> Subject: Re: [phosphor-pid-control] scaling issue
> >>>
> >>> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
> >>> the srcrev bump should propagate into openbmc/openbmc in a day or two.
> >>>
> >>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰)
> ><Hank.Liou@quantatw.com> wrote:
> >>>>
> >>>> Hi Patrick,
> >>>>
> >>>> I think it's OK to parse the config min&max for temp sensors.
> >>>>
> >>>> Any suggestion?
> >>>>
> >>>> Thanks,
> >>>> Hank
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: openbmc [mailto:openbmc-
> >>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank
> >>>>> bounces+Liou
> >>>>> (劉晉翰)
> >>>>> Sent: Friday, August 23, 2019 4:31 PM
> >>>>> To: Patrick Venture <venture@google.com>; James Feist
> >>>>> <james.feist@linux.intel.com>
> >>>>> Cc: openbmc@lists.ozlabs.org
> >>>>> Subject: RE: [phosphor-pid-control] scaling issue
> >>>>>
> >>>>> Hi Patrick,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Patrick Venture [mailto:venture@google.com]
> >>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
> >>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> >>>>>> <james.feist@linux.intel.com>
> >>>>>> Cc: openbmc@lists.ozlabs.org
> >>>>>> Subject: Re: [phosphor-pid-control] scaling issue
> >>>>>>
> >>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> >>>>>> <Hank.Liou@quantatw.com> wrote:
> >>>>>>>
> >>>>>>> Hi All,
> >>>>>>>
> >>>>>>>
> >>>>>>> After commit [1], I found my temp sensor reading would be
> >>>>>>> re-scaled by
> >>>>>> multiplying 1 over 255, making temperature into unfamiliar unit.
> >>>>>> Also the fan rpm reading would lie in [0,1] interval, letting the
> >>>>>> fan input to be 0 (since the input value of fan is from an integer
> >>>>>> array [2]). Are these
> >>>>> normal behaviors?
> >>>>>> Or do I miss something?
> >>>>>>
> >>>>>> Are you using dbus configuration or json?  If json, can you attach your
> >config.
> >>>>>> Since you're saying it was working and now isn't, I'm assuming
> >>>>>> there's something about the config being treated differently with
> >>>>>> the code changes in an unexpected way.
> >>>>>
> >>>>> I found pid control will first read min and max from dbus and then
> >>>>> (before commit [1]) revise them if
> >>>>>
> >>>>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> >>>>>
> >>>>> After value initialization, the min and max would be the ones in
> >>>>> json file (Json file [3]). However, after commit [1] the min and
> >>>>> max values of config would not be fed into the fan control process.
> >>>>> The scaling issue is originated from commit [4] with the aim to
> >>>>> treat fan rpm as percentage. It seems that commit [1] unexpectedly
> >>>>> affects temp sensors in the sense that the temp is the integer
> >>>>> reading not percentage. Before commit [1], it would not re-scale the
> >temp reading since my min and max are 0 [6].
> >>>>>
> >>>>>
> >>>>>
> >>>>> There is another issue with commit [1]. Now the fan rpm would be
> >>>>> something like
> >>>>>
> >>>>> 1500 / 20000 = 0.075
> >>>>>
> >>>>> where rpm max 20000 is from dbus. However the fan input function
> >>>>> would transfer double into int, which is 0 for 0.075 (see [5]).
> >>>>> Hence the fan input is 0 not percentage. Is there something wrong?
> >>>>>
> >>>>> [1] https://github.com/openbmc/phosphor-pid-
> >>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >>>>> [2] https://github.com/openbmc/phosphor-pid-
> >>>>>
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
> >>>>> lle
> >>>>> r.cpp#L86
> >>>>> [3]
> >>>>>        {
> >>>>>             "name": "fan1",
> >>>>>             "type": "fan",
> >>>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> >>>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> >>>>>             "min": 0,
> >>>>>             "max": 255
> >>>>>         },
> >>>>>         {
> >>>>>             "name": "temp1",
> >>>>>             "type": "temp",
> >>>>>             "readPath":
> >"/xyz/openbmc_project/sensors/temperature/temp1",
> >>>>>             "writePath": "",
> >>>>>             "min": 0,
> >>>>>             "max": 0
> >>>>>         }
> >>>>> [4] https://github.com/openbmc/phosphor-pid-
> >>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
> >>>>> [5] https://github.com/openbmc/phosphor-pid-
> >>>>>
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
> >>>>> lle
> >>>>> r.cpp#L64
> >>>>> [6] https://github.com/openbmc/phosphor-pid-
> >>>>>
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspass
> >>>>> i
> >>>>> ve.cpp#L158
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> [1]
> >>>>>>> https://github.com/openbmc/phosphor-pid-
> >>>>>> control/commit/fc2e803f5d92569
> >>>>>>> 44e18c7c878a441606b1f121c
> >>>>>>>
> >>>>>>> [2]
> >>>>>>> https://github.com/openbmc/phosphor-pid-
> >>>>>> control/blob/a7ec8350d17b70153
> >>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> >>>>>>>
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>>
> >>>>>>>
> >>>>>>> Hank Liou
> >>>>>>>
> >>>>>>> Quanta Computer Inc.
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>> Sincerely,
> >>>>> Hank

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Tue, Sep 10, 2019 at 7:39 AM Patrick Venture <venture@google.com> wrote:
>
> On Tue, Sep 10, 2019 at 1:02 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
> >
> > >-----Original Message-----
> > >From: James Feist [mailto:james.feist@linux.intel.com]
> > >Sent: Tuesday, September 10, 2019 1:27 AM
> > >To: Patrick Venture <venture@google.com>; Hank Liou (劉晉翰)
> > ><Hank.Liou@quantatw.com>
> > >Cc: openbmc@lists.ozlabs.org
> > >Subject: Re: [phosphor-pid-control] scaling issue
> > >
> > >On 9/9/19 9:57 AM, Patrick Venture wrote:
> > >> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰)
> > ><Hank.Liou@quantatw.com> wrote:
> > >>>
> > >>> Hi Patrick,
> > >>>
> > >>>
> > >>> Sorry for not clearly stating our problem. We have the following issue:
> > >>>
> > >>>
> > >>> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as
> > >>> input for temp sensor ->
> > >>>
> > >>> stepwise fan table output 100 duty -> full speed fan
> > >>
> > >> Ok, so you're getting a dbus-based min/max value and you want to
> > >> ignore it?
> >
> > Yes, we want to ignore it.
>
> It looks like the sensor value scaling should be ignored for non-fans.
> Per https://github.com/openbmc/phosphor-pid-control/commit/75eb769d351434547899186f73ff70ae00d7934a,
> this change was only meant to deal with a fan case where the goal
> looks like it was to treat the fan values as percentages on reading
> the sensors, and not only on writing them by leveraging the min/max
> json sometimes?
>
> I can change the dbuspassive constructor to set the values back to
> their input values (ignoring dbus) if the type is not "fan", but I'm
> not sure that makes sense because the real use-case isn't clear to me,
> and I know the scaling from dbus is gone:
> https://github.com/openbmc/phosphor-pid-control/search?utf8=%E2%9C%93&q=inheritValueFromDbus&type=
>
> I'll submit the patch real quick, but I'm not sure it's the right
> long-term fix.  I'd like to clean this up if possible so that we don't
> leverage "unused" features of a configuration and instead explicitly
> use them (or not).

This patch should address what you're seeing.  Overall, I think there
should be a larger change addressing the goal of the original change.

https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/25070

>
> >
> > >
> > >What is providing the dbus-based min/max?
> >
> > We use dbus min/max for phosphor-sel-logger utility. The dbus min/max is provided by config file of phosphor-hwmon.
> > In our case, they are -128/127 respectively.
> >
> > I have an observation:
> > I found that while fan readpath is of the form "/sys/...", the fan input would not be rescaled. The same is for other sensors since the input value would not enter rescaling function.
> > However, in our case we don't have a sys path for some thermal sensors.
> >
> > >
> > >> In the json configuration, if you set the values to 0, they are set to
> > >> the default (0), so there'd be no clean way to know to ignore dbus in
> > >> this case, without adding a small check to only consider min/max from
> > >> dbus when sensor is not temp/margin.  Basically, only care about
> > >> min/max on dbus if type is "fan"
> > >
> >
> > I agree with the option to add a check since the min/max is only for fan now.
> >
> > >Why is the dbus-based min/max even there if you don't plan on using it?
> > >
> > >>
> > >> If that's right, James do you have a cycle to look at this one-liner?
> > >>
> > >>>
> > >>>
> > >>> As a result, fan will be at full speed while temp is low. Before the commit
> > >fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will
> > >use config min/max, which is 0 in our case. This would not trigger scaling
> > >function, namely scaleSensorReading, in util.cpp. However, after such commit,
> > >min/max would be non-zero (-128/127 from DBus). This will trigger scaling
> > >function.
> > >>>
> > >>>
> > >>> [1]
> > >>> https://github.com/openbmc/phosphor-pid-
> > >control/commit/fc2e803f5d9256
> > >>> 944e18c7c878a441606b1f121c
> > >>>
> > >>>
> > >>> Hank
> > >>>
> > >>>
> > >>> ________________________________
> > >>> From: Hank Liou (劉晉翰)
> > >>> Sent: Monday, September 2, 2019 4:52 PM
> > >>> To: Patrick Venture
> > >>> Cc: James Feist; openbmc@lists.ozlabs.org
> > >>> Subject: Re: [phosphor-pid-control] scaling issue
> > >>>
> > >>>
> > >>> Hi Patrick,
> > >>>
> > >>>
> > >>> Since we use phosphor-sel-logger [1] at the same time, we do assign min
> > >and max of temp sensors to Dbus (max: 127, min: -128). So in the present case,
> > >our temp value will be divided by 0.255 (also due to exponent is -3 here). This
> > >will cause re-scaling problem. Therefore there should be an statement to
> > >distinguish sensor types. If it is "temp", then one assigns 0 to the min and max
> > >from Dbus.
> > >>>
> > >>>
> > >>> [1]
> > >>> https://github.com/openbmc/phosphor-sel-
> > >logger/blob/3d300fca24b30864b
> > >>> 3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
> > >>>
> > >>>
> > >>> Hank
> > >>>
> > >>>
> > >>> ________________________________
> > >>> From: Patrick Venture <venture@google.com>
> > >>> Sent: Friday, August 30, 2019 1:47 AM
> > >>> To: Hank Liou (劉晉翰)
> > >>> Cc: James Feist; openbmc@lists.ozlabs.org
> > >>> Subject: Re: [phosphor-pid-control] scaling issue
> > >>>
> > >>> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
> > >>> the srcrev bump should propagate into openbmc/openbmc in a day or two.
> > >>>
> > >>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰)
> > ><Hank.Liou@quantatw.com> wrote:
> > >>>>
> > >>>> Hi Patrick,
> > >>>>
> > >>>> I think it's OK to parse the config min&max for temp sensors.
> > >>>>
> > >>>> Any suggestion?
> > >>>>
> > >>>> Thanks,
> > >>>> Hank
> > >>>>
> > >>>>> -----Original Message-----
> > >>>>> From: openbmc [mailto:openbmc-
> > >>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank
> > >>>>> bounces+Liou
> > >>>>> (劉晉翰)
> > >>>>> Sent: Friday, August 23, 2019 4:31 PM
> > >>>>> To: Patrick Venture <venture@google.com>; James Feist
> > >>>>> <james.feist@linux.intel.com>
> > >>>>> Cc: openbmc@lists.ozlabs.org
> > >>>>> Subject: RE: [phosphor-pid-control] scaling issue
> > >>>>>
> > >>>>> Hi Patrick,
> > >>>>>
> > >>>>>> -----Original Message-----
> > >>>>>> From: Patrick Venture [mailto:venture@google.com]
> > >>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
> > >>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> > >>>>>> <james.feist@linux.intel.com>
> > >>>>>> Cc: openbmc@lists.ozlabs.org
> > >>>>>> Subject: Re: [phosphor-pid-control] scaling issue
> > >>>>>>
> > >>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> > >>>>>> <Hank.Liou@quantatw.com> wrote:
> > >>>>>>>
> > >>>>>>> Hi All,
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> After commit [1], I found my temp sensor reading would be
> > >>>>>>> re-scaled by
> > >>>>>> multiplying 1 over 255, making temperature into unfamiliar unit.
> > >>>>>> Also the fan rpm reading would lie in [0,1] interval, letting the
> > >>>>>> fan input to be 0 (since the input value of fan is from an integer
> > >>>>>> array [2]). Are these
> > >>>>> normal behaviors?
> > >>>>>> Or do I miss something?
> > >>>>>>
> > >>>>>> Are you using dbus configuration or json?  If json, can you attach your
> > >config.
> > >>>>>> Since you're saying it was working and now isn't, I'm assuming
> > >>>>>> there's something about the config being treated differently with
> > >>>>>> the code changes in an unexpected way.
> > >>>>>
> > >>>>> I found pid control will first read min and max from dbus and then
> > >>>>> (before commit [1]) revise them if
> > >>>>>
> > >>>>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> > >>>>>
> > >>>>> After value initialization, the min and max would be the ones in
> > >>>>> json file (Json file [3]). However, after commit [1] the min and
> > >>>>> max values of config would not be fed into the fan control process.
> > >>>>> The scaling issue is originated from commit [4] with the aim to
> > >>>>> treat fan rpm as percentage. It seems that commit [1] unexpectedly
> > >>>>> affects temp sensors in the sense that the temp is the integer
> > >>>>> reading not percentage. Before commit [1], it would not re-scale the
> > >temp reading since my min and max are 0 [6].
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> There is another issue with commit [1]. Now the fan rpm would be
> > >>>>> something like
> > >>>>>
> > >>>>> 1500 / 20000 = 0.075
> > >>>>>
> > >>>>> where rpm max 20000 is from dbus. However the fan input function
> > >>>>> would transfer double into int, which is 0 for 0.075 (see [5]).
> > >>>>> Hence the fan input is 0 not percentage. Is there something wrong?
> > >>>>>
> > >>>>> [1] https://github.com/openbmc/phosphor-pid-
> > >>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> > >>>>> [2] https://github.com/openbmc/phosphor-pid-
> > >>>>>
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
> > >>>>> lle
> > >>>>> r.cpp#L86
> > >>>>> [3]
> > >>>>>        {
> > >>>>>             "name": "fan1",
> > >>>>>             "type": "fan",
> > >>>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> > >>>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> > >>>>>             "min": 0,
> > >>>>>             "max": 255
> > >>>>>         },
> > >>>>>         {
> > >>>>>             "name": "temp1",
> > >>>>>             "type": "temp",
> > >>>>>             "readPath":
> > >"/xyz/openbmc_project/sensors/temperature/temp1",
> > >>>>>             "writePath": "",
> > >>>>>             "min": 0,
> > >>>>>             "max": 0
> > >>>>>         }
> > >>>>> [4] https://github.com/openbmc/phosphor-pid-
> > >>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
> > >>>>> [5] https://github.com/openbmc/phosphor-pid-
> > >>>>>
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
> > >>>>> lle
> > >>>>> r.cpp#L64
> > >>>>> [6] https://github.com/openbmc/phosphor-pid-
> > >>>>>
> > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspass
> > >>>>> i
> > >>>>> ve.cpp#L158
> > >>>>>
> > >>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> [1]
> > >>>>>>> https://github.com/openbmc/phosphor-pid-
> > >>>>>> control/commit/fc2e803f5d92569
> > >>>>>>> 44e18c7c878a441606b1f121c
> > >>>>>>>
> > >>>>>>> [2]
> > >>>>>>> https://github.com/openbmc/phosphor-pid-
> > >>>>>> control/blob/a7ec8350d17b70153
> > >>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> Thanks,
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> Hank Liou
> > >>>>>>>
> > >>>>>>> Quanta Computer Inc.
> > >>>>>>>
> > >>>>>>>
> > >>>>>
> > >>>>> Sincerely,
> > >>>>> Hank

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Tue, Sep 10, 2019 at 7:52 AM Patrick Venture <venture@google.com> wrote:
>
> On Tue, Sep 10, 2019 at 7:39 AM Patrick Venture <venture@google.com> wrote:
> >
> > On Tue, Sep 10, 2019 at 1:02 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
> > >
> > > >-----Original Message-----
> > > >From: James Feist [mailto:james.feist@linux.intel.com]
> > > >Sent: Tuesday, September 10, 2019 1:27 AM
> > > >To: Patrick Venture <venture@google.com>; Hank Liou (劉晉翰)
> > > ><Hank.Liou@quantatw.com>
> > > >Cc: openbmc@lists.ozlabs.org
> > > >Subject: Re: [phosphor-pid-control] scaling issue
> > > >
> > > >On 9/9/19 9:57 AM, Patrick Venture wrote:
> > > >> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰)
> > > ><Hank.Liou@quantatw.com> wrote:
> > > >>>
> > > >>> Hi Patrick,
> > > >>>
> > > >>>
> > > >>> Sorry for not clearly stating our problem. We have the following issue:
> > > >>>
> > > >>>
> > > >>> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses 127.57 as
> > > >>> input for temp sensor ->
> > > >>>
> > > >>> stepwise fan table output 100 duty -> full speed fan
> > > >>
> > > >> Ok, so you're getting a dbus-based min/max value and you want to
> > > >> ignore it?
> > >
> > > Yes, we want to ignore it.
> >
> > It looks like the sensor value scaling should be ignored for non-fans.
> > Per https://github.com/openbmc/phosphor-pid-control/commit/75eb769d351434547899186f73ff70ae00d7934a,
> > this change was only meant to deal with a fan case where the goal
> > looks like it was to treat the fan values as percentages on reading
> > the sensors, and not only on writing them by leveraging the min/max
> > json sometimes?
> >
> > I can change the dbuspassive constructor to set the values back to
> > their input values (ignoring dbus) if the type is not "fan", but I'm
> > not sure that makes sense because the real use-case isn't clear to me,
> > and I know the scaling from dbus is gone:
> > https://github.com/openbmc/phosphor-pid-control/search?utf8=%E2%9C%93&q=inheritValueFromDbus&type=
> >
> > I'll submit the patch real quick, but I'm not sure it's the right
> > long-term fix.  I'd like to clean this up if possible so that we don't
> > leverage "unused" features of a configuration and instead explicitly
> > use them (or not).
>
> This patch should address what you're seeing.  Overall, I think there
> should be a larger change addressing the goal of the original change.
>
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/25070

I decided to take on the larger change because chances are we might
hit it some other way:
https://gerrit.openbmc-project.xyz/c/25072/

This lets you specify a json configuration option that will ignore the
MinValue/MaxValue properties on dbus.

>
> >
> > >
> > > >
> > > >What is providing the dbus-based min/max?
> > >
> > > We use dbus min/max for phosphor-sel-logger utility. The dbus min/max is provided by config file of phosphor-hwmon.
> > > In our case, they are -128/127 respectively.
> > >
> > > I have an observation:
> > > I found that while fan readpath is of the form "/sys/...", the fan input would not be rescaled. The same is for other sensors since the input value would not enter rescaling function.
> > > However, in our case we don't have a sys path for some thermal sensors.
> > >
> > > >
> > > >> In the json configuration, if you set the values to 0, they are set to
> > > >> the default (0), so there'd be no clean way to know to ignore dbus in
> > > >> this case, without adding a small check to only consider min/max from
> > > >> dbus when sensor is not temp/margin.  Basically, only care about
> > > >> min/max on dbus if type is "fan"
> > > >
> > >
> > > I agree with the option to add a check since the min/max is only for fan now.
> > >
> > > >Why is the dbus-based min/max even there if you don't plan on using it?
> > > >
> > > >>
> > > >> If that's right, James do you have a cycle to look at this one-liner?
> > > >>
> > > >>>
> > > >>>
> > > >>> As a result, fan will be at full speed while temp is low. Before the commit
> > > >fc2e803 [1], this won't happen. The root cause is that, before fc2e803, pid will
> > > >use config min/max, which is 0 in our case. This would not trigger scaling
> > > >function, namely scaleSensorReading, in util.cpp. However, after such commit,
> > > >min/max would be non-zero (-128/127 from DBus). This will trigger scaling
> > > >function.
> > > >>>
> > > >>>
> > > >>> [1]
> > > >>> https://github.com/openbmc/phosphor-pid-
> > > >control/commit/fc2e803f5d9256
> > > >>> 944e18c7c878a441606b1f121c
> > > >>>
> > > >>>
> > > >>> Hank
> > > >>>
> > > >>>
> > > >>> ________________________________
> > > >>> From: Hank Liou (劉晉翰)
> > > >>> Sent: Monday, September 2, 2019 4:52 PM
> > > >>> To: Patrick Venture
> > > >>> Cc: James Feist; openbmc@lists.ozlabs.org
> > > >>> Subject: Re: [phosphor-pid-control] scaling issue
> > > >>>
> > > >>>
> > > >>> Hi Patrick,
> > > >>>
> > > >>>
> > > >>> Since we use phosphor-sel-logger [1] at the same time, we do assign min
> > > >and max of temp sensors to Dbus (max: 127, min: -128). So in the present case,
> > > >our temp value will be divided by 0.255 (also due to exponent is -3 here). This
> > > >will cause re-scaling problem. Therefore there should be an statement to
> > > >distinguish sensor types. If it is "temp", then one assigns 0 to the min and max
> > > >from Dbus.
> > > >>>
> > > >>>
> > > >>> [1]
> > > >>> https://github.com/openbmc/phosphor-sel-
> > > >logger/blob/3d300fca24b30864b
> > > >>> 3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
> > > >>>
> > > >>>
> > > >>> Hank
> > > >>>
> > > >>>
> > > >>> ________________________________
> > > >>> From: Patrick Venture <venture@google.com>
> > > >>> Sent: Friday, August 30, 2019 1:47 AM
> > > >>> To: Hank Liou (劉晉翰)
> > > >>> Cc: James Feist; openbmc@lists.ozlabs.org
> > > >>> Subject: Re: [phosphor-pid-control] scaling issue
> > > >>>
> > > >>> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is merged, and
> > > >>> the srcrev bump should propagate into openbmc/openbmc in a day or two.
> > > >>>
> > > >>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰)
> > > ><Hank.Liou@quantatw.com> wrote:
> > > >>>>
> > > >>>> Hi Patrick,
> > > >>>>
> > > >>>> I think it's OK to parse the config min&max for temp sensors.
> > > >>>>
> > > >>>> Any suggestion?
> > > >>>>
> > > >>>> Thanks,
> > > >>>> Hank
> > > >>>>
> > > >>>>> -----Original Message-----
> > > >>>>> From: openbmc [mailto:openbmc-
> > > >>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf Of Hank
> > > >>>>> bounces+Liou
> > > >>>>> (劉晉翰)
> > > >>>>> Sent: Friday, August 23, 2019 4:31 PM
> > > >>>>> To: Patrick Venture <venture@google.com>; James Feist
> > > >>>>> <james.feist@linux.intel.com>
> > > >>>>> Cc: openbmc@lists.ozlabs.org
> > > >>>>> Subject: RE: [phosphor-pid-control] scaling issue
> > > >>>>>
> > > >>>>> Hi Patrick,
> > > >>>>>
> > > >>>>>> -----Original Message-----
> > > >>>>>> From: Patrick Venture [mailto:venture@google.com]
> > > >>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
> > > >>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James Feist
> > > >>>>>> <james.feist@linux.intel.com>
> > > >>>>>> Cc: openbmc@lists.ozlabs.org
> > > >>>>>> Subject: Re: [phosphor-pid-control] scaling issue
> > > >>>>>>
> > > >>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> > > >>>>>> <Hank.Liou@quantatw.com> wrote:
> > > >>>>>>>
> > > >>>>>>> Hi All,
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> After commit [1], I found my temp sensor reading would be
> > > >>>>>>> re-scaled by
> > > >>>>>> multiplying 1 over 255, making temperature into unfamiliar unit.
> > > >>>>>> Also the fan rpm reading would lie in [0,1] interval, letting the
> > > >>>>>> fan input to be 0 (since the input value of fan is from an integer
> > > >>>>>> array [2]). Are these
> > > >>>>> normal behaviors?
> > > >>>>>> Or do I miss something?
> > > >>>>>>
> > > >>>>>> Are you using dbus configuration or json?  If json, can you attach your
> > > >config.
> > > >>>>>> Since you're saying it was working and now isn't, I'm assuming
> > > >>>>>> there's something about the config being treated differently with
> > > >>>>>> the code changes in an unexpected way.
> > > >>>>>
> > > >>>>> I found pid control will first read min and max from dbus and then
> > > >>>>> (before commit [1]) revise them if
> > > >>>>>
> > > >>>>> info->min != conf::inheritValueFromDbus (in dbus/dbuspassive.cpp)
> > > >>>>>
> > > >>>>> After value initialization, the min and max would be the ones in
> > > >>>>> json file (Json file [3]). However, after commit [1] the min and
> > > >>>>> max values of config would not be fed into the fan control process.
> > > >>>>> The scaling issue is originated from commit [4] with the aim to
> > > >>>>> treat fan rpm as percentage. It seems that commit [1] unexpectedly
> > > >>>>> affects temp sensors in the sense that the temp is the integer
> > > >>>>> reading not percentage. Before commit [1], it would not re-scale the
> > > >temp reading since my min and max are 0 [6].
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>> There is another issue with commit [1]. Now the fan rpm would be
> > > >>>>> something like
> > > >>>>>
> > > >>>>> 1500 / 20000 = 0.075
> > > >>>>>
> > > >>>>> where rpm max 20000 is from dbus. However the fan input function
> > > >>>>> would transfer double into int, which is 0 for 0.075 (see [5]).
> > > >>>>> Hence the fan input is 0 not percentage. Is there something wrong?
> > > >>>>>
> > > >>>>> [1] https://github.com/openbmc/phosphor-pid-
> > > >>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> > > >>>>> [2] https://github.com/openbmc/phosphor-pid-
> > > >>>>>
> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
> > > >>>>> lle
> > > >>>>> r.cpp#L86
> > > >>>>> [3]
> > > >>>>>        {
> > > >>>>>             "name": "fan1",
> > > >>>>>             "type": "fan",
> > > >>>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> > > >>>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> > > >>>>>             "min": 0,
> > > >>>>>             "max": 255
> > > >>>>>         },
> > > >>>>>         {
> > > >>>>>             "name": "temp1",
> > > >>>>>             "type": "temp",
> > > >>>>>             "readPath":
> > > >"/xyz/openbmc_project/sensors/temperature/temp1",
> > > >>>>>             "writePath": "",
> > > >>>>>             "min": 0,
> > > >>>>>             "max": 0
> > > >>>>>         }
> > > >>>>> [4] https://github.com/openbmc/phosphor-pid-
> > > >>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
> > > >>>>> [5] https://github.com/openbmc/phosphor-pid-
> > > >>>>>
> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancontro
> > > >>>>> lle
> > > >>>>> r.cpp#L64
> > > >>>>> [6] https://github.com/openbmc/phosphor-pid-
> > > >>>>>
> > > >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspass
> > > >>>>> i
> > > >>>>> ve.cpp#L158
> > > >>>>>
> > > >>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> [1]
> > > >>>>>>> https://github.com/openbmc/phosphor-pid-
> > > >>>>>> control/commit/fc2e803f5d92569
> > > >>>>>>> 44e18c7c878a441606b1f121c
> > > >>>>>>>
> > > >>>>>>> [2]
> > > >>>>>>> https://github.com/openbmc/phosphor-pid-
> > > >>>>>> control/blob/a7ec8350d17b70153
> > > >>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> Thanks,
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> Hank Liou
> > > >>>>>>>
> > > >>>>>>> Quanta Computer Inc.
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>
> > > >>>>> Sincerely,
> > > >>>>> Hank

RE: [phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

Hi Patrick,

Thanks for your help. This change does solve the issue.

Hank

>-----Original Message-----
>From: Patrick Venture [mailto:venture@google.com]
>Sent: Wednesday, September 11, 2019 1:52 AM
>To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>
>Cc: James Feist <james.feist@linux.intel.com>; openbmc@lists.ozlabs.org
>Subject: Re: [phosphor-pid-control] scaling issue
>
>On Tue, Sep 10, 2019 at 7:52 AM Patrick Venture <venture@google.com>
>wrote:
>>
>> On Tue, Sep 10, 2019 at 7:39 AM Patrick Venture <venture@google.com>
>wrote:
>> >
>> > On Tue, Sep 10, 2019 at 1:02 AM Hank Liou (劉晉翰)
><Hank.Liou@quantatw.com> wrote:
>> > >
>> > > >-----Original Message-----
>> > > >From: James Feist [mailto:james.feist@linux.intel.com]
>> > > >Sent: Tuesday, September 10, 2019 1:27 AM
>> > > >To: Patrick Venture <venture@google.com>; Hank Liou (劉晉翰)
>> > > ><Hank.Liou@quantatw.com>
>> > > >Cc: openbmc@lists.ozlabs.org
>> > > >Subject: Re: [phosphor-pid-control] scaling issue
>> > > >
>> > > >On 9/9/19 9:57 AM, Patrick Venture wrote:
>> > > >> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰)
>> > > ><Hank.Liou@quantatw.com> wrote:
>> > > >>>
>> > > >>> Hi Patrick,
>> > > >>>
>> > > >>>
>> > > >>> Sorry for not clearly stating our problem. We have the following
>issue:
>> > > >>>
>> > > >>>
>> > > >>> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses
>> > > >>> 127.57 as input for temp sensor ->
>> > > >>>
>> > > >>> stepwise fan table output 100 duty -> full speed fan
>> > > >>
>> > > >> Ok, so you're getting a dbus-based min/max value and you want
>> > > >> to ignore it?
>> > >
>> > > Yes, we want to ignore it.
>> >
>> > It looks like the sensor value scaling should be ignored for non-fans.
>> > Per
>> > https://github.com/openbmc/phosphor-pid-
>control/commit/75eb769d35143
>> > 4547899186f73ff70ae00d7934a, this change was only meant to deal with
>> > a fan case where the goal looks like it was to treat the fan values
>> > as percentages on reading the sensors, and not only on writing them
>> > by leveraging the min/max json sometimes?
>> >
>> > I can change the dbuspassive constructor to set the values back to
>> > their input values (ignoring dbus) if the type is not "fan", but I'm
>> > not sure that makes sense because the real use-case isn't clear to
>> > me, and I know the scaling from dbus is gone:
>> > https://github.com/openbmc/phosphor-pid-
>control/search?utf8=%E2%9C%9
>> > 3&q=inheritValueFromDbus&type=
>> >
>> > I'll submit the patch real quick, but I'm not sure it's the right
>> > long-term fix.  I'd like to clean this up if possible so that we
>> > don't leverage "unused" features of a configuration and instead
>> > explicitly use them (or not).
>>
>> This patch should address what you're seeing.  Overall, I think there
>> should be a larger change addressing the goal of the original change.
>>
>> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/25
>> 070
>
>I decided to take on the larger change because chances are we might hit it
>some other way:
>https://gerrit.openbmc-project.xyz/c/25072/
>
>This lets you specify a json configuration option that will ignore the
>MinValue/MaxValue properties on dbus.
>
>>
>> >
>> > >
>> > > >
>> > > >What is providing the dbus-based min/max?
>> > >
>> > > We use dbus min/max for phosphor-sel-logger utility. The dbus min/max
>is provided by config file of phosphor-hwmon.
>> > > In our case, they are -128/127 respectively.
>> > >
>> > > I have an observation:
>> > > I found that while fan readpath is of the form "/sys/...", the fan input
>would not be rescaled. The same is for other sensors since the input value
>would not enter rescaling function.
>> > > However, in our case we don't have a sys path for some thermal sensors.
>> > >
>> > > >
>> > > >> In the json configuration, if you set the values to 0, they are
>> > > >> set to the default (0), so there'd be no clean way to know to
>> > > >> ignore dbus in this case, without adding a small check to only
>> > > >> consider min/max from dbus when sensor is not temp/margin.
>> > > >> Basically, only care about min/max on dbus if type is "fan"
>> > > >
>> > >
>> > > I agree with the option to add a check since the min/max is only for fan
>now.
>> > >
>> > > >Why is the dbus-based min/max even there if you don't plan on using it?
>> > > >
>> > > >>
>> > > >> If that's right, James do you have a cycle to look at this one-liner?
>> > > >>
>> > > >>>
>> > > >>>
>> > > >>> As a result, fan will be at full speed while temp is low.
>> > > >>> Before the commit
>> > > >fc2e803 [1], this won't happen. The root cause is that, before
>> > > >fc2e803, pid will use config min/max, which is 0 in our case.
>> > > >This would not trigger scaling function, namely
>> > > >scaleSensorReading, in util.cpp. However, after such commit,
>> > > >min/max would be non-zero (-128/127 from DBus). This will trigger
>scaling function.
>> > > >>>
>> > > >>>
>> > > >>> [1]
>> > > >>> https://github.com/openbmc/phosphor-pid-
>> > > >control/commit/fc2e803f5d9256
>> > > >>> 944e18c7c878a441606b1f121c
>> > > >>>
>> > > >>>
>> > > >>> Hank
>> > > >>>
>> > > >>>
>> > > >>> ________________________________
>> > > >>> From: Hank Liou (劉晉翰)
>> > > >>> Sent: Monday, September 2, 2019 4:52 PM
>> > > >>> To: Patrick Venture
>> > > >>> Cc: James Feist; openbmc@lists.ozlabs.org
>> > > >>> Subject: Re: [phosphor-pid-control] scaling issue
>> > > >>>
>> > > >>>
>> > > >>> Hi Patrick,
>> > > >>>
>> > > >>>
>> > > >>> Since we use phosphor-sel-logger [1] at the same time, we do
>> > > >>> assign min
>> > > >and max of temp sensors to Dbus (max: 127, min: -128). So in the
>> > > >present case, our temp value will be divided by 0.255 (also due
>> > > >to exponent is -3 here). This will cause re-scaling problem.
>> > > >Therefore there should be an statement to distinguish sensor
>> > > >types. If it is "temp", then one assigns 0 to the min and max from Dbus.
>> > > >>>
>> > > >>>
>> > > >>> [1]
>> > > >>> https://github.com/openbmc/phosphor-sel-
>> > > >logger/blob/3d300fca24b30864b
>> > > >>> 3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
>> > > >>>
>> > > >>>
>> > > >>> Hank
>> > > >>>
>> > > >>>
>> > > >>> ________________________________
>> > > >>> From: Patrick Venture <venture@google.com>
>> > > >>> Sent: Friday, August 30, 2019 1:47 AM
>> > > >>> To: Hank Liou (劉晉翰)
>> > > >>> Cc: James Feist; openbmc@lists.ozlabs.org
>> > > >>> Subject: Re: [phosphor-pid-control] scaling issue
>> > > >>>
>> > > >>> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is
>> > > >>> merged, and the srcrev bump should propagate into
>openbmc/openbmc in a day or two.
>> > > >>>
>> > > >>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰)
>> > > ><Hank.Liou@quantatw.com> wrote:
>> > > >>>>
>> > > >>>> Hi Patrick,
>> > > >>>>
>> > > >>>> I think it's OK to parse the config min&max for temp sensors.
>> > > >>>>
>> > > >>>> Any suggestion?
>> > > >>>>
>> > > >>>> Thanks,
>> > > >>>> Hank
>> > > >>>>
>> > > >>>>> -----Original Message-----
>> > > >>>>> From: openbmc [mailto:openbmc-
>> > > >>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf
>> > > >>>>> bounces+Of Hank Liou
>> > > >>>>> (劉晉翰)
>> > > >>>>> Sent: Friday, August 23, 2019 4:31 PM
>> > > >>>>> To: Patrick Venture <venture@google.com>; James Feist
>> > > >>>>> <james.feist@linux.intel.com>
>> > > >>>>> Cc: openbmc@lists.ozlabs.org
>> > > >>>>> Subject: RE: [phosphor-pid-control] scaling issue
>> > > >>>>>
>> > > >>>>> Hi Patrick,
>> > > >>>>>
>> > > >>>>>> -----Original Message-----
>> > > >>>>>> From: Patrick Venture [mailto:venture@google.com]
>> > > >>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
>> > > >>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James
>Feist
>> > > >>>>>> <james.feist@linux.intel.com>
>> > > >>>>>> Cc: openbmc@lists.ozlabs.org
>> > > >>>>>> Subject: Re: [phosphor-pid-control] scaling issue
>> > > >>>>>>
>> > > >>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>> > > >>>>>> <Hank.Liou@quantatw.com> wrote:
>> > > >>>>>>>
>> > > >>>>>>> Hi All,
>> > > >>>>>>>
>> > > >>>>>>>
>> > > >>>>>>> After commit [1], I found my temp sensor reading would be
>> > > >>>>>>> re-scaled by
>> > > >>>>>> multiplying 1 over 255, making temperature into unfamiliar unit.
>> > > >>>>>> Also the fan rpm reading would lie in [0,1] interval,
>> > > >>>>>> letting the fan input to be 0 (since the input value of fan
>> > > >>>>>> is from an integer array [2]). Are these
>> > > >>>>> normal behaviors?
>> > > >>>>>> Or do I miss something?
>> > > >>>>>>
>> > > >>>>>> Are you using dbus configuration or json?  If json, can you
>> > > >>>>>> attach your
>> > > >config.
>> > > >>>>>> Since you're saying it was working and now isn't, I'm
>> > > >>>>>> assuming there's something about the config being treated
>> > > >>>>>> differently with the code changes in an unexpected way.
>> > > >>>>>
>> > > >>>>> I found pid control will first read min and max from dbus
>> > > >>>>> and then (before commit [1]) revise them if
>> > > >>>>>
>> > > >>>>> info->min != conf::inheritValueFromDbus (in
>> > > >>>>> info->dbus/dbuspassive.cpp)
>> > > >>>>>
>> > > >>>>> After value initialization, the min and max would be the
>> > > >>>>> ones in json file (Json file [3]). However, after commit [1]
>> > > >>>>> the min and max values of config would not be fed into the fan
>control process.
>> > > >>>>> The scaling issue is originated from commit [4] with the aim
>> > > >>>>> to treat fan rpm as percentage. It seems that commit [1]
>> > > >>>>> unexpectedly affects temp sensors in the sense that the temp
>> > > >>>>> is the integer reading not percentage. Before commit [1], it
>> > > >>>>> would not re-scale the
>> > > >temp reading since my min and max are 0 [6].
>> > > >>>>>
>> > > >>>>>
>> > > >>>>>
>> > > >>>>> There is another issue with commit [1]. Now the fan rpm
>> > > >>>>> would be something like
>> > > >>>>>
>> > > >>>>> 1500 / 20000 = 0.075
>> > > >>>>>
>> > > >>>>> where rpm max 20000 is from dbus. However the fan input
>> > > >>>>> function would transfer double into int, which is 0 for 0.075 (see
>[5]).
>> > > >>>>> Hence the fan input is 0 not percentage. Is there something
>wrong?
>> > > >>>>>
>> > > >>>>> [1] https://github.com/openbmc/phosphor-pid-
>> > > >>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
>> > > >>>>> [2] https://github.com/openbmc/phosphor-pid-
>> > > >>>>>
>> > >
>>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancont
>> > > >ro
>> > > >>>>> lle
>> > > >>>>> r.cpp#L86
>> > > >>>>> [3]
>> > > >>>>>        {
>> > > >>>>>             "name": "fan1",
>> > > >>>>>             "type": "fan",
>> > > >>>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
>> > > >>>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
>> > > >>>>>             "min": 0,
>> > > >>>>>             "max": 255
>> > > >>>>>         },
>> > > >>>>>         {
>> > > >>>>>             "name": "temp1",
>> > > >>>>>             "type": "temp",
>> > > >>>>>             "readPath":
>> > > >"/xyz/openbmc_project/sensors/temperature/temp1",
>> > > >>>>>             "writePath": "",
>> > > >>>>>             "min": 0,
>> > > >>>>>             "max": 0
>> > > >>>>>         }
>> > > >>>>> [4] https://github.com/openbmc/phosphor-pid-
>> > > >>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
>> > > >>>>> [5] https://github.com/openbmc/phosphor-pid-
>> > > >>>>>
>> > >
>>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancont
>> > > >ro
>> > > >>>>> lle
>> > > >>>>> r.cpp#L64
>> > > >>>>> [6] https://github.com/openbmc/phosphor-pid-
>> > > >>>>>
>> > >
>>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspa
>> > > >ss
>> > > >>>>> i
>> > > >>>>> ve.cpp#L158
>> > > >>>>>
>> > > >>>>>>
>> > > >>>>>>>
>> > > >>>>>>>
>> > > >>>>>>> [1]
>> > > >>>>>>> https://github.com/openbmc/phosphor-pid-
>> > > >>>>>> control/commit/fc2e803f5d92569
>> > > >>>>>>> 44e18c7c878a441606b1f121c
>> > > >>>>>>>
>> > > >>>>>>> [2]
>> > > >>>>>>> https://github.com/openbmc/phosphor-pid-
>> > > >>>>>> control/blob/a7ec8350d17b70153
>> > > >>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
>> > > >>>>>>>
>> > > >>>>>>>
>> > > >>>>>>> Thanks,
>> > > >>>>>>>
>> > > >>>>>>>
>> > > >>>>>>> Hank Liou
>> > > >>>>>>>
>> > > >>>>>>> Quanta Computer Inc.
>> > > >>>>>>>
>> > > >>>>>>>
>> > > >>>>>
>> > > >>>>> Sincerely,
>> > > >>>>> Hank

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Mon, Sep 16, 2019 at 1:37 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi Patrick,
>
> Thanks for your help. This change does solve the issue.

No problem! I enjoy it when someone hits an edge case or a situation
not yet considered.  We don't have nearly enough variety of
configurations to really test quirks and unit-testing only covers so
much.

I saw your review requesting the configuration change, and +1'd it.
Presumably you're well on your way.

Thanks for your patience in this back-and-forth to resolve the issue.

>
> Hank
>
> >-----Original Message-----
> >From: Patrick Venture [mailto:venture@google.com]
> >Sent: Wednesday, September 11, 2019 1:52 AM
> >To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>
> >Cc: James Feist <james.feist@linux.intel.com>; openbmc@lists.ozlabs.org
> >Subject: Re: [phosphor-pid-control] scaling issue
> >
> >On Tue, Sep 10, 2019 at 7:52 AM Patrick Venture <venture@google.com>
> >wrote:
> >>
> >> On Tue, Sep 10, 2019 at 7:39 AM Patrick Venture <venture@google.com>
> >wrote:
> >> >
> >> > On Tue, Sep 10, 2019 at 1:02 AM Hank Liou (劉晉翰)
> ><Hank.Liou@quantatw.com> wrote:
> >> > >
> >> > > >-----Original Message-----
> >> > > >From: James Feist [mailto:james.feist@linux.intel.com]
> >> > > >Sent: Tuesday, September 10, 2019 1:27 AM
> >> > > >To: Patrick Venture <venture@google.com>; Hank Liou (劉晉翰)
> >> > > ><Hank.Liou@quantatw.com>
> >> > > >Cc: openbmc@lists.ozlabs.org
> >> > > >Subject: Re: [phosphor-pid-control] scaling issue
> >> > > >
> >> > > >On 9/9/19 9:57 AM, Patrick Venture wrote:
> >> > > >> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰)
> >> > > ><Hank.Liou@quantatw.com> wrote:
> >> > > >>>
> >> > > >>> Hi Patrick,
> >> > > >>>
> >> > > >>>
> >> > > >>> Sorry for not clearly stating our problem. We have the following
> >issue:
> >> > > >>>
> >> > > >>>
> >> > > >>> temp sensor gets 31(C) -> 31 / 0.255 = 121.57 -> pid uses
> >> > > >>> 127.57 as input for temp sensor ->
> >> > > >>>
> >> > > >>> stepwise fan table output 100 duty -> full speed fan
> >> > > >>
> >> > > >> Ok, so you're getting a dbus-based min/max value and you want
> >> > > >> to ignore it?
> >> > >
> >> > > Yes, we want to ignore it.
> >> >
> >> > It looks like the sensor value scaling should be ignored for non-fans.
> >> > Per
> >> > https://github.com/openbmc/phosphor-pid-
> >control/commit/75eb769d35143
> >> > 4547899186f73ff70ae00d7934a, this change was only meant to deal with
> >> > a fan case where the goal looks like it was to treat the fan values
> >> > as percentages on reading the sensors, and not only on writing them
> >> > by leveraging the min/max json sometimes?
> >> >
> >> > I can change the dbuspassive constructor to set the values back to
> >> > their input values (ignoring dbus) if the type is not "fan", but I'm
> >> > not sure that makes sense because the real use-case isn't clear to
> >> > me, and I know the scaling from dbus is gone:
> >> > https://github.com/openbmc/phosphor-pid-
> >control/search?utf8=%E2%9C%9
> >> > 3&q=inheritValueFromDbus&type=
> >> >
> >> > I'll submit the patch real quick, but I'm not sure it's the right
> >> > long-term fix.  I'd like to clean this up if possible so that we
> >> > don't leverage "unused" features of a configuration and instead
> >> > explicitly use them (or not).
> >>
> >> This patch should address what you're seeing.  Overall, I think there
> >> should be a larger change addressing the goal of the original change.
> >>
> >> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-pid-control/+/25
> >> 070
> >
> >I decided to take on the larger change because chances are we might hit it
> >some other way:
> >https://gerrit.openbmc-project.xyz/c/25072/
> >
> >This lets you specify a json configuration option that will ignore the
> >MinValue/MaxValue properties on dbus.
> >
> >>
> >> >
> >> > >
> >> > > >
> >> > > >What is providing the dbus-based min/max?
> >> > >
> >> > > We use dbus min/max for phosphor-sel-logger utility. The dbus min/max
> >is provided by config file of phosphor-hwmon.
> >> > > In our case, they are -128/127 respectively.
> >> > >
> >> > > I have an observation:
> >> > > I found that while fan readpath is of the form "/sys/...", the fan input
> >would not be rescaled. The same is for other sensors since the input value
> >would not enter rescaling function.
> >> > > However, in our case we don't have a sys path for some thermal sensors.
> >> > >
> >> > > >
> >> > > >> In the json configuration, if you set the values to 0, they are
> >> > > >> set to the default (0), so there'd be no clean way to know to
> >> > > >> ignore dbus in this case, without adding a small check to only
> >> > > >> consider min/max from dbus when sensor is not temp/margin.
> >> > > >> Basically, only care about min/max on dbus if type is "fan"
> >> > > >
> >> > >
> >> > > I agree with the option to add a check since the min/max is only for fan
> >now.
> >> > >
> >> > > >Why is the dbus-based min/max even there if you don't plan on using it?
> >> > > >
> >> > > >>
> >> > > >> If that's right, James do you have a cycle to look at this one-liner?
> >> > > >>
> >> > > >>>
> >> > > >>>
> >> > > >>> As a result, fan will be at full speed while temp is low.
> >> > > >>> Before the commit
> >> > > >fc2e803 [1], this won't happen. The root cause is that, before
> >> > > >fc2e803, pid will use config min/max, which is 0 in our case.
> >> > > >This would not trigger scaling function, namely
> >> > > >scaleSensorReading, in util.cpp. However, after such commit,
> >> > > >min/max would be non-zero (-128/127 from DBus). This will trigger
> >scaling function.
> >> > > >>>
> >> > > >>>
> >> > > >>> [1]
> >> > > >>> https://github.com/openbmc/phosphor-pid-
> >> > > >control/commit/fc2e803f5d9256
> >> > > >>> 944e18c7c878a441606b1f121c
> >> > > >>>
> >> > > >>>
> >> > > >>> Hank
> >> > > >>>
> >> > > >>>
> >> > > >>> ________________________________
> >> > > >>> From: Hank Liou (劉晉翰)
> >> > > >>> Sent: Monday, September 2, 2019 4:52 PM
> >> > > >>> To: Patrick Venture
> >> > > >>> Cc: James Feist; openbmc@lists.ozlabs.org
> >> > > >>> Subject: Re: [phosphor-pid-control] scaling issue
> >> > > >>>
> >> > > >>>
> >> > > >>> Hi Patrick,
> >> > > >>>
> >> > > >>>
> >> > > >>> Since we use phosphor-sel-logger [1] at the same time, we do
> >> > > >>> assign min
> >> > > >and max of temp sensors to Dbus (max: 127, min: -128). So in the
> >> > > >present case, our temp value will be divided by 0.255 (also due
> >> > > >to exponent is -3 here). This will cause re-scaling problem.
> >> > > >Therefore there should be an statement to distinguish sensor
> >> > > >types. If it is "temp", then one assigns 0 to the min and max from Dbus.
> >> > > >>>
> >> > > >>>
> >> > > >>> [1]
> >> > > >>> https://github.com/openbmc/phosphor-sel-
> >> > > >logger/blob/3d300fca24b30864b
> >> > > >>> 3e9a4f5768cfe5e769458ae/include/sensorutils.hpp#L59
> >> > > >>>
> >> > > >>>
> >> > > >>> Hank
> >> > > >>>
> >> > > >>>
> >> > > >>> ________________________________
> >> > > >>> From: Patrick Venture <venture@google.com>
> >> > > >>> Sent: Friday, August 30, 2019 1:47 AM
> >> > > >>> To: Hank Liou (劉晉翰)
> >> > > >>> Cc: James Feist; openbmc@lists.ozlabs.org
> >> > > >>> Subject: Re: [phosphor-pid-control] scaling issue
> >> > > >>>
> >> > > >>> PTAL - https://gerrit.openbmc-project.xyz/24827 - this is
> >> > > >>> merged, and the srcrev bump should propagate into
> >openbmc/openbmc in a day or two.
> >> > > >>>
> >> > > >>> On Wed, Aug 28, 2019 at 11:00 PM Hank Liou (劉晉翰)
> >> > > ><Hank.Liou@quantatw.com> wrote:
> >> > > >>>>
> >> > > >>>> Hi Patrick,
> >> > > >>>>
> >> > > >>>> I think it's OK to parse the config min&max for temp sensors.
> >> > > >>>>
> >> > > >>>> Any suggestion?
> >> > > >>>>
> >> > > >>>> Thanks,
> >> > > >>>> Hank
> >> > > >>>>
> >> > > >>>>> -----Original Message-----
> >> > > >>>>> From: openbmc [mailto:openbmc-
> >> > > >>>>> bounces+hank.liou=quantatw.com@lists.ozlabs.org] On Behalf
> >> > > >>>>> bounces+Of Hank Liou
> >> > > >>>>> (劉晉翰)
> >> > > >>>>> Sent: Friday, August 23, 2019 4:31 PM
> >> > > >>>>> To: Patrick Venture <venture@google.com>; James Feist
> >> > > >>>>> <james.feist@linux.intel.com>
> >> > > >>>>> Cc: openbmc@lists.ozlabs.org
> >> > > >>>>> Subject: RE: [phosphor-pid-control] scaling issue
> >> > > >>>>>
> >> > > >>>>> Hi Patrick,
> >> > > >>>>>
> >> > > >>>>>> -----Original Message-----
> >> > > >>>>>> From: Patrick Venture [mailto:venture@google.com]
> >> > > >>>>>> Sent: Wednesday, August 21, 2019 10:32 PM
> >> > > >>>>>> To: Hank Liou (劉晉翰) <Hank.Liou@quantatw.com>; James
> >Feist
> >> > > >>>>>> <james.feist@linux.intel.com>
> >> > > >>>>>> Cc: openbmc@lists.ozlabs.org
> >> > > >>>>>> Subject: Re: [phosphor-pid-control] scaling issue
> >> > > >>>>>>
> >> > > >>>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
> >> > > >>>>>> <Hank.Liou@quantatw.com> wrote:
> >> > > >>>>>>>
> >> > > >>>>>>> Hi All,
> >> > > >>>>>>>
> >> > > >>>>>>>
> >> > > >>>>>>> After commit [1], I found my temp sensor reading would be
> >> > > >>>>>>> re-scaled by
> >> > > >>>>>> multiplying 1 over 255, making temperature into unfamiliar unit.
> >> > > >>>>>> Also the fan rpm reading would lie in [0,1] interval,
> >> > > >>>>>> letting the fan input to be 0 (since the input value of fan
> >> > > >>>>>> is from an integer array [2]). Are these
> >> > > >>>>> normal behaviors?
> >> > > >>>>>> Or do I miss something?
> >> > > >>>>>>
> >> > > >>>>>> Are you using dbus configuration or json?  If json, can you
> >> > > >>>>>> attach your
> >> > > >config.
> >> > > >>>>>> Since you're saying it was working and now isn't, I'm
> >> > > >>>>>> assuming there's something about the config being treated
> >> > > >>>>>> differently with the code changes in an unexpected way.
> >> > > >>>>>
> >> > > >>>>> I found pid control will first read min and max from dbus
> >> > > >>>>> and then (before commit [1]) revise them if
> >> > > >>>>>
> >> > > >>>>> info->min != conf::inheritValueFromDbus (in
> >> > > >>>>> info->dbus/dbuspassive.cpp)
> >> > > >>>>>
> >> > > >>>>> After value initialization, the min and max would be the
> >> > > >>>>> ones in json file (Json file [3]). However, after commit [1]
> >> > > >>>>> the min and max values of config would not be fed into the fan
> >control process.
> >> > > >>>>> The scaling issue is originated from commit [4] with the aim
> >> > > >>>>> to treat fan rpm as percentage. It seems that commit [1]
> >> > > >>>>> unexpectedly affects temp sensors in the sense that the temp
> >> > > >>>>> is the integer reading not percentage. Before commit [1], it
> >> > > >>>>> would not re-scale the
> >> > > >temp reading since my min and max are 0 [6].
> >> > > >>>>>
> >> > > >>>>>
> >> > > >>>>>
> >> > > >>>>> There is another issue with commit [1]. Now the fan rpm
> >> > > >>>>> would be something like
> >> > > >>>>>
> >> > > >>>>> 1500 / 20000 = 0.075
> >> > > >>>>>
> >> > > >>>>> where rpm max 20000 is from dbus. However the fan input
> >> > > >>>>> function would transfer double into int, which is 0 for 0.075 (see
> >[5]).
> >> > > >>>>> Hence the fan input is 0 not percentage. Is there something
> >wrong?
> >> > > >>>>>
> >> > > >>>>> [1] https://github.com/openbmc/phosphor-pid-
> >> > > >>>>> control/commit/fc2e803f5d9256944e18c7c878a441606b1f121c
> >> > > >>>>> [2] https://github.com/openbmc/phosphor-pid-
> >> > > >>>>>
> >> > >
> >>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancont
> >> > > >ro
> >> > > >>>>> lle
> >> > > >>>>> r.cpp#L86
> >> > > >>>>> [3]
> >> > > >>>>>        {
> >> > > >>>>>             "name": "fan1",
> >> > > >>>>>             "type": "fan",
> >> > > >>>>>             "readPath": "/sys/class/hwmon/hwmon1/fan1_input",
> >> > > >>>>>             "writePath": "/sys/class/hwmon/hwmon1/pwm1",
> >> > > >>>>>             "min": 0,
> >> > > >>>>>             "max": 255
> >> > > >>>>>         },
> >> > > >>>>>         {
> >> > > >>>>>             "name": "temp1",
> >> > > >>>>>             "type": "temp",
> >> > > >>>>>             "readPath":
> >> > > >"/xyz/openbmc_project/sensors/temperature/temp1",
> >> > > >>>>>             "writePath": "",
> >> > > >>>>>             "min": 0,
> >> > > >>>>>             "max": 0
> >> > > >>>>>         }
> >> > > >>>>> [4] https://github.com/openbmc/phosphor-pid-
> >> > > >>>>> control/commit/75eb769d351434547899186f73ff70ae00d7934a
> >> > > >>>>> [5] https://github.com/openbmc/phosphor-pid-
> >> > > >>>>>
> >> > >
> >>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/pid/fancont
> >> > > >ro
> >> > > >>>>> lle
> >> > > >>>>> r.cpp#L64
> >> > > >>>>> [6] https://github.com/openbmc/phosphor-pid-
> >> > > >>>>>
> >> > >
> >>control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspa
> >> > > >ss
> >> > > >>>>> i
> >> > > >>>>> ve.cpp#L158
> >> > > >>>>>
> >> > > >>>>>>
> >> > > >>>>>>>
> >> > > >>>>>>>
> >> > > >>>>>>> [1]
> >> > > >>>>>>> https://github.com/openbmc/phosphor-pid-
> >> > > >>>>>> control/commit/fc2e803f5d92569
> >> > > >>>>>>> 44e18c7c878a441606b1f121c
> >> > > >>>>>>>
> >> > > >>>>>>> [2]
> >> > > >>>>>>> https://github.com/openbmc/phosphor-pid-
> >> > > >>>>>> control/blob/a7ec8350d17b70153
> >> > > >>>>>>> cebe666d3fbe88bddd02a1a/pid/fancontroller.cpp#L86
> >> > > >>>>>>>
> >> > > >>>>>>>
> >> > > >>>>>>> Thanks,
> >> > > >>>>>>>
> >> > > >>>>>>>
> >> > > >>>>>>> Hank Liou
> >> > > >>>>>>>
> >> > > >>>>>>> Quanta Computer Inc.
> >> > > >>>>>>>
> >> > > >>>>>>>
> >> > > >>>>>
> >> > > >>>>> Sincerely,
> >> > > >>>>> Hank

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Thu, Mar 28, 2019 at 7:17 PM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi Patrick,
>
>
> {
>     "sensors" : [
>         {
>             "name": "fan1",
>             "type": "fan",
>             "readPath": "/xyz/openbmc_project/sensors/fan_tach/fan1",
>             "writePath": "/sys/devices/platform/ahb/ahb:apb/1e786000.pwm-tacho-controller/hwmon/**/pwm1",
>             "min": 0,
>             "max": 255
>         },
>         {
>             "name": "temp1",
>             "type": "temp",
>             "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
>             "writePath": "",
>             "min": 0,
>             "max": 0
>         }
>     ],
>     "zones" : [
>         {
>             "id": 1,
>             "minThermalOutput": 3000.0,
^--- the minimum thermal output is higher than the maximum output for
the temperature PID loop.
>             "failsafePercent": 75.0,
>             "pids": [
>                 {
>                     "name": "fan1",
>                     "type": "fan",
>                     "inputs": ["fan1"],
>                     "setpoint": 90.0,
>                     "pid": {
>                         "samplePeriod": 0.1,
>                         "proportionalCoeff": -1.0,
>                         "integralCoeff": -0.2,

^--- you're setting the integral and proportional and the feedfwd.
feedfwd works separately if I understand PID loops properly

>                         "feedFwdOffsetCoeff": 0.0,
>                         "feedFwdGainCoeff": 0.010,
>                         "integralLimit_min": 0.0,
>                         "integralLimit_max": 0.0,
>                         "outLim_min": 30.0,
^--- this is setting the minimum to 30%
>                         "outLim_max": 100.0,
^--- this is setting the maximum to 30%
>                         "slewNeg": 0.0,
>                         "slewPos": 0.0
>                     }
>                 },
>                 {
>                     "name": "temp1",
>                     "type": "temp",
>                     "inputs": ["temp1"],
>                     "setpoint": 50.0,

^ --- this sets the goal temperature to be lower than or equal to 50.
50 of whatever temperature unit provided.  The value should be scaled
properly to be 50.000 by the code.

>                     "pid": {
>                         "samplePeriod": 0.1,
^-- so the mechanism for the PID loop only samples these at once per second
>                         "proportionalCoeff": -1.0,
>                         "integralCoeff": -0.2,
>                         "feedFwdOffsetCoeff": 0.0,
>                         "feedFwdGainCoeff": 0.010,
>                         "integralLimit_min": 0.0,
>                         "integralLimit_max": 0.0,
>                         "outLim_min": 500.0,
^--- this is setting the output minimum to 500rpm
>                         "outLim_max": 1500.0,
^--- this is setting the maximum to 1500rpm.
>                         "slewNeg": 0.0,
>                         "slewPos": 0.0
>                     }
>                 }
>             ]
>         }
>     ]
> }

I don't see any scaling issues given this configuration.  Because
you're not building from dbus configuration, but rather json.  Given
the numbers here, I would expect some wonky behavior -- however, that
just means we need more detailed documentation.

By the way, please don't top-post to email messages.  Inline replies
are easier to follow and the community standard.

>
> Sincerely,
>
>
> Hank Liou
>
>
> ________________________________
> From: Patrick Venture <venture@google.com>
> Sent: Thursday, March 28, 2019 10:33 PM
> To: Hank Liou (劉晉翰)
> Cc: openbmc@lists.ozlabs.org
> Subject: Re: [phosphor-pid-control] scaling issue
>
> On Thu, Mar 28, 2019 at 3:55 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
> >
> > Hi All,
> >
> >
> > This issue is related to the repository phosphor-pid-control.
> >
> >
> > It is in commit dbuspassive: allow scaling that one added scaling mechanism. However when it comes to pid.cpp, it may encounter scaling mismatch. That is to say, (in fan control stage) the pid takes the scaled values as inputs, but it utilizes unscaled setpoints resulting wrong error terms. Or just change the setpoint value?
>
> Can you share your configuration?  That might simplify the conversation.
>
> >
> >
> > Thanks,
> >
> >
> > Hank Liou
> >
> > Quanta Computer Inc.

Re: [phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

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

Hi Patrick,


{
    "sensors" : [
        {
            "name": "fan1",
            "type": "fan",
            "readPath": "/xyz/openbmc_project/sensors/fan_tach/fan1",
            "writePath": "/sys/devices/platform/ahb/ahb:apb/1e786000.pwm-tacho-controller/hwmon/**/pwm1",
            "min": 0,
            "max": 255
        },
        {
            "name": "temp1",
            "type": "temp",
            "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
            "writePath": "",
            "min": 0,
            "max": 0
        }
    ],
    "zones" : [
        {
            "id": 1,
            "minThermalOutput": 3000.0,
            "failsafePercent": 75.0,
            "pids": [
                {
                    "name": "fan1",
                    "type": "fan",
                    "inputs": ["fan1"],
                    "setpoint": 90.0,
                    "pid": {
                        "samplePeriod": 0.1,
                        "proportionalCoeff": -1.0,
                        "integralCoeff": -0.2,
                        "feedFwdOffsetCoeff": 0.0,
                        "feedFwdGainCoeff": 0.010,
                        "integralLimit_min": 0.0,
                        "integralLimit_max": 0.0,
                        "outLim_min": 30.0,
                        "outLim_max": 100.0,
                        "slewNeg": 0.0,
                        "slewPos": 0.0
                    }
                },
                {
                    "name": "temp1",
                    "type": "temp",
                    "inputs": ["temp1"],
                    "setpoint": 50.0,
                    "pid": {
                        "samplePeriod": 0.1,
                        "proportionalCoeff": -1.0,
                        "integralCoeff": -0.2,
                        "feedFwdOffsetCoeff": 0.0,
                        "feedFwdGainCoeff": 0.010,
                        "integralLimit_min": 0.0,
                        "integralLimit_max": 0.0,
                        "outLim_min": 500.0,
                        "outLim_max": 1500.0,
                        "slewNeg": 0.0,
                        "slewPos": 0.0
                    }
                }
            ]
        }
    ]
}


Sincerely,


Hank Liou

________________________________
From: Patrick Venture <venture@google.com>
Sent: Thursday, March 28, 2019 10:33 PM
To: Hank Liou (劉晉翰)
Cc: openbmc@lists.ozlabs.org
Subject: Re: [phosphor-pid-control] scaling issue

On Thu, Mar 28, 2019 at 3:55 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi All,
>
>
> This issue is related to the repository phosphor-pid-control.
>
>
> It is in commit dbuspassive: allow scaling that one added scaling mechanism. However when it comes to pid.cpp, it may encounter scaling mismatch. That is to say, (in fan control stage) the pid takes the scaled values as inputs, but it utilizes unscaled setpoints resulting wrong error terms. Or just change the setpoint value?

Can you share your configuration?  That might simplify the conversation.

>
>
> Thanks,
>
>
> Hank Liou
>
> Quanta Computer Inc.

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

Re: [phosphor-pid-control] scaling issue

[Patrick Venture]

On Thu, Mar 28, 2019 at 3:55 AM Hank Liou (劉晉翰) <Hank.Liou@quantatw.com> wrote:
>
> Hi All,
>
>
> This issue is related to the repository phosphor-pid-control.
>
>
> It is in commit dbuspassive: allow scaling that one added scaling mechanism. However when it comes to pid.cpp, it may encounter scaling mismatch. That is to say, (in fan control stage) the pid takes the scaled values as inputs, but it utilizes unscaled setpoints resulting wrong error terms. Or just change the setpoint value?

Can you share your configuration?  That might simplify the conversation.

>
>
> Thanks,
>
>
> Hank Liou
>
> Quanta Computer Inc.

[phosphor-pid-control] scaling issue

[Hank Liou (劉晉翰)]

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

Hi All,


This issue is related to the repository phosphor-pid-control<https://github.com/openbmc/phosphor-pid-control>.


It is in commit dbuspassive: allow scaling<https://github.com/openbmc/phosphor-pid-control/commit/75eb769d351434547899186f73ff70ae00d7934a> that one added scaling mechanism. However when it comes to pid.cpp<https://github.com/openbmc/phosphor-pid-control/blob/master/pid/ec/pid.cpp>, it may encounter scaling mismatch. That is to say, (in fan control stage) the pid takes the scaled values as inputs, but it utilizes unscaled setpoints resulting wrong error terms. Or just change the setpoint value?


Thanks,


Hank Liou

Quanta Computer Inc.

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