openbmc / phosphor-pid-control

OpenBMC PID-based Thermal Control Daemon
Apache License 2.0
16 stars 21 forks source link

proposal: Stepwise controller with more computation type and moving average process #28

Open huangalang opened 1 year ago

huangalang commented 1 year ago

we found that stepwise only supports one computation type: max() =>Get max value of _thermal_input

But for some cases we need more computation type, EX: average and sum up the inputs for something like power readings)

So we’re proposing to add more computation type , something like bellow

json config

"name": "sensor-group",
    "type": "stepwise",
    "computationType": "sum",
    "inputs": [
        "temp_first",
        "temp_second"
    ],
    "setpoint": 0.0,
    "pid": {
        "samplePeriod": 1,
        "isCeiling": false,
        "reading": {
            "0": 30.0,
            "1": 40.0,
            "2": 50.0,
            "3": 60.0,
            "4": 70.0
        },

In addition, for power readings, we’d also like to introduce moving average process on each _thermal_input before they are computed (max/sum/average) to reduce the fluctuation in power readings as bellow

double StepwiseController::inputProc(void)
value = 0;
for (const auto& in : _inputs)
{
-            value += _owner->getCachedValue(in);
+           value += movingAverage(in, _owner->getCachedValue(in));
}

All the processes put together will go like bellow

double StepwiseController::inputProc(void)
{
     if (_computationType == "sum"")
     {
         value = 0;
         for (const auto& in : _inputs)
        {
            value += movingAverage(in, _owner->getCachedValue(in));
         }
        ……  
     }
}
Krellan commented 1 year ago

This seems reasonable. I personally do not use the "stepwise" PID type, so don't have a lot of experience evaluating it.

blackcatevil commented 1 year ago

I agree that stepwise controller should support more type of input: power, margin, temp... If we support margin as input in future, we have to add min() for it.

But I would like to say that the average should not be implemented in phosphod-pid-control, all controllers need is given inputs only. The average can be done by virtual sensor which had existed in OpenBMC.

I believe people use stepwise controller because they don't need to take the sensor history into calculation.