Re: [phosphor-pid-control] scaling issue

[Patrick Venture <venture@google.com>]

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