[phosphor-pid-control] scaling issue

Patrick Venture venture at google.com
Tue Sep 17 01:44:43 AEST 2019 

On Mon, Sep 16, 2019 at 1:37 AM Hank Liou (劉晉翰) <Hank.Liou at 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 at google.com]
> >Sent: Wednesday, September 11, 2019 1:52 AM
> >To: Hank Liou (劉晉翰) <Hank.Liou at quantatw.com>
> >Cc: James Feist <james.feist at linux.intel.com>; openbmc at lists.ozlabs.org
> >Subject: Re: [phosphor-pid-control] scaling issue
> >
> >On Tue, Sep 10, 2019 at 7:52 AM Patrick Venture <venture at google.com>
> >wrote:
> >>
> >> On Tue, Sep 10, 2019 at 7:39 AM Patrick Venture <venture at google.com>
> >wrote:
> >> >
> >> > On Tue, Sep 10, 2019 at 1:02 AM Hank Liou (劉晉翰)
> ><Hank.Liou at quantatw.com> wrote:
> >> > >
> >> > > >-----Original Message-----
> >> > > >From: James Feist [mailto:james.feist at linux.intel.com]
> >> > > >Sent: Tuesday, September 10, 2019 1:27 AM
> >> > > >To: Patrick Venture <venture at google.com>; Hank Liou (劉晉翰)
> >> > > ><Hank.Liou at quantatw.com>
> >> > > >Cc: openbmc at 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 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?
> >> > >
> >> > > 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 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/3d300fca24b30864b
> >> > > >>> 3e9a4f5768cfe5e769458ae/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
> >> > > >>>>> bounces+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
> >> > > >>>>> 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

More information about the openbmc mailing list