[PATCH] tools/thermal: tmon: Fix sample data update in PID

[PATCH] tools/thermal: tmon: Fix sample data update in PID

[Meet Udeshi]

Fixed the order of update for `xk_1` and `xk_2` in the PID controller in
function `controller_handler()`.

The previous timestep data in the PID controller, `xk_1` and `xk_2`,
were updated in the wrong order. `xk_1` was overwritten before `xk_2`
was assigned. This caused both `xk_1` and `xk_2` to take the value of
`xk` when the function `controller_handler()` was called.
This means the D-term of the PID controller was simplified from
a second-order approximation using two previous timesteps to a
first-order approximation using one previous timestep.

    xk - 2 * xk_1 + xk_2 => xk - xk_1

This degraded the performance of the PID controller by making it more
noisy and less accurate.

This bug was found by reverse engineering the tmon binary using the
[REMaQE tool](https://arxiv.org/abs/2305.06902).

Signed-off-by: Meet Udeshi <mudeshi1209@gmail.com>
Fixes: 94f69966faf8 ("tools/thermal: Introduce tmon, a tool for thermal subsystem")
---
 tools/thermal/tmon/pid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/thermal/tmon/pid.c b/tools/thermal/tmon/pid.c
index da20088285bd..1c02eb675088 100644
--- a/tools/thermal/tmon/pid.c
+++ b/tools/thermal/tmon/pid.c
@@ -103,8 +103,8 @@ void controller_handler(const double xk, double *yk)
 	/* compute output */
 	*yk += p_term + i_term + d_term;
 	/* update sample data */
-	xk_1 = xk;
 	xk_2 = xk_1;
+	xk_1 = xk;
 
 	/* clamp output adjustment range */
 	if (*yk < -LIMIT_HIGH)
-- 
2.34.1

Re: [PATCH] tools/thermal: tmon: Fix sample data update in PID

[Meet Udeshi]

Hi Daniel,

Wondering if this patch is being reviewed by you (you are delegated for this patch on Patchwork).
I initially only sent this patch to the linux-pm mailing list, but I realised it may not have reached you.

I am new to the Linux patch submission procedure so I apologize for any inconvenience.

Regards,
Meet.

On Tue, 22 Aug 2023 at 14:50, Meet Udeshi <mudeshi1209@gmail.com> wrote:
> Fixed the order of update for `xk_1` and `xk_2` in the PID controller in
> function `controller_handler()`.
> 
> The previous timestep data in the PID controller, `xk_1` and `xk_2`,
> were updated in the wrong order. `xk_1` was overwritten before `xk_2`
> was assigned. This caused both `xk_1` and `xk_2` to take the value of
> `xk` when the function `controller_handler()` was called.
> This means the D-term of the PID controller was simplified from
> a second-order approximation using two previous timesteps to a
> first-order approximation using one previous timestep.
> 
>     xk - 2 * xk_1 + xk_2 => xk - xk_1
> 
> This degraded the performance of the PID controller by making it more
> noisy and less accurate.
> 
> This bug was found by reverse engineering the tmon binary using the
> [REMaQE tool](https://arxiv.org/abs/2305.06902).
> 
> Signed-off-by: Meet Udeshi <mudeshi1209@gmail.com>
> Fixes: 94f69966faf8 ("tools/thermal: Introduce tmon, a tool for thermal subsystem")
> ---
>  tools/thermal/tmon/pid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/thermal/tmon/pid.c b/tools/thermal/tmon/pid.c
> index da20088285bd..1c02eb675088 100644
> --- a/tools/thermal/tmon/pid.c
> +++ b/tools/thermal/tmon/pid.c
> @@ -103,8 +103,8 @@ void controller_handler(const double xk, double *yk)
>  	/* compute output */
>  	*yk += p_term + i_term + d_term;
>  	/* update sample data */
> -	xk_1 = xk;
>  	xk_2 = xk_1;
> +	xk_1 = xk;
>  
>  	/* clamp output adjustment range */
>  	if (*yk < -LIMIT_HIGH)
> -- 
> 2.34.1