[phosphor-pid-control] scaling issue

Patrick Venture venture at google.com
Tue Sep 10 03:23:59 AEST 2019 

On Mon, Sep 9, 2019 at 10:12 AM Kun Yi <kunyi at 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 at google.com> wrote:
>>
>> On Thu, Sep 5, 2019 at 12:25 AM Hank Liou (劉晉翰) <Hank.Liou at 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 at 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 at google.com>
>> > Sent: Friday, August 30, 2019 1:47 AM
>> > To: Hank Liou (劉晉翰)
>> > Cc: James Feist; openbmc at 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 at 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 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
>
>
>
> --
> Regards,
> Kun

More information about the openbmc mailing list