[phosphor-pid-control] scaling issue
Patrick Venture
venture at google.com
Wed Sep 11 00:39:49 AEST 2019
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/75eb769d351434547899186f73ff70ae00d7934a,
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%93&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).
>
> >
> >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 Of Hank
> >>>>> bounces+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/fancontro
> >>>>> 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/fancontro
> >>>>> lle
> >>>>> r.cpp#L64
> >>>>> [6] https://github.com/openbmc/phosphor-pid-
> >>>>>
> >control/blob/a7ec8350d17b70153cebe666d3fbe88bddd02a1a/dbus/dbuspass
> >>>>> 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