[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 at 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 at lists.ozlabs.org] On Behalf Of Hank Liou
>>> (劉晉翰)
>>> Sent: Friday, August 23, 2019 4:31 PM
>>> To: Patrick Venture <venture at google.com>; James Feist
>>> <james.feist at linux.intel.com>
>>> Cc: openbmc at lists.ozlabs.org
>>> Subject: RE: [phosphor-pid-control] scaling issue
>>>
>>> Hi Patrick,
>>>
>>>> -----Original Message-----
>>>> From: Patrick Venture [mailto:venture at google.com]
>>>> Sent: Wednesday, August 21, 2019 10:32 PM
>>>> To: Hank Liou (劉晉翰) <Hank.Liou at quantatw.com>; James Feist
>>>> <james.feist at linux.intel.com>
>>>> Cc: openbmc at lists.ozlabs.org
>>>> Subject: Re: [phosphor-pid-control] scaling issue
>>>>
>>>> On Wed, Aug 21, 2019 at 1:11 AM Hank Liou (劉晉翰)
>>>> <Hank.Liou at 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