adding PID controller feature for Thermal Management #856
Conversation
hisandeepsingh
requested review from
nicola-mazzucato-arm,
souvikkc,
tarek-arm,
girishpathak,
leandro-arm,
AhmedImbabyGadallah and
mohamedasaker-arm
as code owners
|
Can one of the admins verify this patch? |
|
jenkins: ok to test |
| @@ -177,7 +177,13 @@ struct mod_thermal_mgmt_dev_config { | |||
|
|
|||
| /*! Integral term (PI loop) */ | |||
| int32_t k_integral; | |||
| } pi_controller; | |||
| #ifdef PID | |||
There was a problem hiding this comment.
Can you please rename it to THERMAL_HAS_PID_CONTROLLER ?
| #ifdef PID | ||
| pid_control(id); | ||
| #else | ||
| pi_control(id); |
There was a problem hiding this comment.
Can you please run clang-format or check tools/check_tabs.py script?
There was a problem hiding this comment.
yes, please run the formatter on this change. There are a couple of lines that need formatting.
| @@ -34,6 +34,68 @@ struct mod_thermal_mgmt_actor_ctx *get_actor_ctx( | |||
| return &dev_ctx->actor_ctx_table[actor]; | |||
| } | |||
|
|
|||
|
|
|||
| #ifdef PID | |||
There was a problem hiding this comment.
IMO there's no need to create two implementations that are almost identical but the kd part.
I would suggest to add the derivative part in the pi_control as is, no additional #IFDEF.
Platforms that do not care about the derivative part can simply leave that coefficient to zero.
Then rename pi_controller() to pid_controller() please
| #ifdef PID | ||
| pid_control(id); | ||
| #else | ||
| pi_control(id); |
There was a problem hiding this comment.
yes, please run the formatter on this change. There are a couple of lines that need formatting.
The current implementation of Thermal Management provides a PI controller, without a derivative term. Improve the controller by adding the derivative term, thus becoming a PID control. At the same time, update the TC2 configuration as required by this change. This change originates from: ARM-software#856 and has been lightly reworked to improve readability, update unit testing and comply with coding style. Signed-off-by: Sandeep Singh <hisandeepsingh@hotmail.com> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Co-authored-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Change-Id: Ie0b667c680476714d4e6ad17afa3623ca36a56b8
The current implementation of Thermal Management provides a PI controller, without a derivative term. Improve the controller by adding the derivative term, thus becoming a PID control. At the same time, update the TC2 configuration as required by this change. This change originates from: #856 and has been lightly reworked to improve readability, update unit testing and comply with coding style. Signed-off-by: Sandeep Singh <hisandeepsingh@hotmail.com> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Co-authored-by: Nicola Mazzucato <nicola.mazzucato@arm.com> Change-Id: Ie0b667c680476714d4e6ad17afa3623ca36a56b8
Adding Derivative term in PI Controller .
Included the PID changes protected inside flag. Please review it and let me know if its looks good then we can do respective changes in test cases and in thermal doc thermal_mgmt.md