[phosphor-pid-control] scaling issue

[Patrick Venture]

On Thu, Mar 28, 2019 at 7:17 PM Hank Liou (劉晉翰) <Hank.Liou at quantatw.com> wrote:
>
> Hi Patrick,
>
>
> {
>     "sensors" : [
>         {
>             "name": "fan1",
>             "type": "fan",
>             "readPath": "/xyz/openbmc_project/sensors/fan_tach/fan1",
>             "writePath": "/sys/devices/platform/ahb/ahb:apb/1e786000.pwm-tacho-controller/hwmon/**/pwm1",
>             "min": 0,
>             "max": 255
>         },
>         {
>             "name": "temp1",
>             "type": "temp",
>             "readPath": "/xyz/openbmc_project/sensors/temperature/temp1",
>             "writePath": "",
>             "min": 0,
>             "max": 0
>         }
>     ],
>     "zones" : [
>         {
>             "id": 1,
>             "minThermalOutput": 3000.0,
^--- the minimum thermal output is higher than the maximum output for
the temperature PID loop.
>             "failsafePercent": 75.0,
>             "pids": [
>                 {
>                     "name": "fan1",
>                     "type": "fan",
>                     "inputs": ["fan1"],
>                     "setpoint": 90.0,
>                     "pid": {
>                         "samplePeriod": 0.1,
>                         "proportionalCoeff": -1.0,
>                         "integralCoeff": -0.2,

^--- you're setting the integral and proportional and the feedfwd.
feedfwd works separately if I understand PID loops properly

>                         "feedFwdOffsetCoeff": 0.0,
>                         "feedFwdGainCoeff": 0.010,
>                         "integralLimit_min": 0.0,
>                         "integralLimit_max": 0.0,
>                         "outLim_min": 30.0,
^--- this is setting the minimum to 30%
>                         "outLim_max": 100.0,
^--- this is setting the maximum to 30%
>                         "slewNeg": 0.0,
>                         "slewPos": 0.0
>                     }
>                 },
>                 {
>                     "name": "temp1",
>                     "type": "temp",
>                     "inputs": ["temp1"],
>                     "setpoint": 50.0,

^ --- this sets the goal temperature to be lower than or equal to 50.
50 of whatever temperature unit provided.  The value should be scaled
properly to be 50.000 by the code.

>                     "pid": {
>                         "samplePeriod": 0.1,
^-- so the mechanism for the PID loop only samples these at once per second
>                         "proportionalCoeff": -1.0,
>                         "integralCoeff": -0.2,
>                         "feedFwdOffsetCoeff": 0.0,
>                         "feedFwdGainCoeff": 0.010,
>                         "integralLimit_min": 0.0,
>                         "integralLimit_max": 0.0,
>                         "outLim_min": 500.0,
^--- this is setting the output minimum to 500rpm
>                         "outLim_max": 1500.0,
^--- this is setting the maximum to 1500rpm.
>                         "slewNeg": 0.0,
>                         "slewPos": 0.0
>                     }
>                 }
>             ]
>         }
>     ]
> }

I don't see any scaling issues given this configuration.  Because
you're not building from dbus configuration, but rather json.  Given
the numbers here, I would expect some wonky behavior -- however, that
just means we need more detailed documentation.

By the way, please don't top-post to email messages.  Inline replies
are easier to follow and the community standard.

>
> Sincerely,
>
>
> Hank Liou
>
>
> ________________________________
> From: Patrick Venture <venture at google.com>
> Sent: Thursday, March 28, 2019 10:33 PM
> To: Hank Liou (劉晉翰)
> Cc: openbmc at lists.ozlabs.org
> Subject: Re: [phosphor-pid-control] scaling issue
>
> On Thu, Mar 28, 2019 at 3:55 AM Hank Liou (劉晉翰) <Hank.Liou at quantatw.com> wrote:
> >
> > Hi All,
> >
> >
> > This issue is related to the repository phosphor-pid-control.
> >
> >
> > It is in commit dbuspassive: allow scaling that one added scaling mechanism. However when it comes to pid.cpp, it may encounter scaling mismatch. That is to say, (in fan control stage) the pid takes the scaled values as inputs, but it utilizes unscaled setpoints resulting wrong error terms. Or just change the setpoint value?
>
> Can you share your configuration?  That might simplify the conversation.
>
> >
> >
> > Thanks,
> >
> >
> > Hank Liou
> >
> > Quanta Computer Inc.