[phosphor-pid-control] scaling issue

Fri Aug 30 02:21:23 AEST 2019

On Thu, Aug 29, 2019 at 9:14 AM James Feist <james.feist at 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 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

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).