puff: make fan control better behaved

[tofurky]

The stock fan control has a few issues leading to erratic behavior such as stalls and constant cycling on/off.

Most of this is due to the tendency of the algorithm to make the temperature rapidly oscillate above and below the "fan off" temperature.

However, the fan on duffy (Asus Chromebox 4) additionally has a stall speed of approximately 7% PWM.

The algorithm, when frequently stopping and starting per the aforementioned oscillation, can wind up choosing PWM values below this threshold, causing the fan to stall. Due to a delayed response, it'll then be commanded to quickly burst towards maximum RPM, crossing the "fan off" boundary and just as quickly stopping again.

A few changes make the fan much better behaved:

• Add a 3C "fan off" hysteresis to avoid oscillating
• Ensure applied PWM is always greater than stall speed
• When stopping the fan, set RPM to 0 immediately to avoid stale/misleading values being used for subsequent decisions
• Slightly raise the "fan off" temperature and lower the min fan speed

Also, while we're in there, fix a typo in the CML header guard.

Fixes MrChromebox/firmware/issues/678

[tofurky]

I've been using this (just used cbfstool to replace ecrw/ecrw.hash on a backup .rom) and the issue described at MrChromebox/firmware#678 hasn't recurred. It tends to settle at a RPM close to the minimum now.

Previously, putting my ear to it, it was constantly starting and stopping (like every couple seconds) which can't be good for the bearings etc.

I wrapped the functional changes in #ifdef BOARD_PUFF (in my early testing it was unconditional); not sure if it matters if the change would just go into the puff-specific branch. But left it as a draft for the time being.

[MrChromebox]

I think we want to make sure that all puff-based devices (some of which use other board definitions) also benefit from these changes, so I would drop the ifdefs and adjust the min rpm and fan off temp for others as well (ambassador, dooly, genesis, moonbuggy, scout)

[tofurky]

I think we want to make sure that all puff-based devices (some of which use other board definitions) also benefit from these changes, so I would drop the ifdefs and adjust the min rpm and fan off temp for others as well (ambassador, dooly, genesis, moonbuggy, scout)

Thanks for taking a look.

I can do as suggested, but I won't be able to test any of those other changes.

They're all built from the same branch as this, then? And, do you know off hand if the fan specs etc. are similar enough between them that they can use the same settings?

The one I have is the somewhat dinky Celeron which is just dual core and lower TDP. Adding the hysteresis and immediately setting the RPM to 0 should help all boards, though.

[MrChromebox]

They're all built from the same branch as this, then? And, do you know off hand if the fan specs etc. are similar enough between them that they can use the same settings?

yes. fan specs, no idea as never seen any of those boards and they aren't documented/published

Adding the hysteresis and immediately setting the RPM to 0 should help all boards, though.

sounds good to me. we can probably do the same for the Fizz boards/branch too

[tofurky]

I think we want to make sure that all puff-based devices (some of which use other board definitions) also benefit from these changes, so I would drop the ifdefs and adjust the min rpm and fan off temp for others as well (ambassador, dooly, genesis, moonbuggy, scout)

I started looking at these other boards and it seems most of them have their own "custom" fan control:

$ git grep CONFIG_FAN_RPM_CUSTOM
baseboard/kalista/baseboard.h:#define CONFIG_FAN_RPM_CUSTOM
board/ambassador/board.h:#define CONFIG_FAN_RPM_CUSTOM
board/endeavour/board.h:#define CONFIG_FAN_RPM_CUSTOM
board/ezkinil/board.h:#define CONFIG_FAN_RPM_CUSTOM
board/fizz/board.h:#define CONFIG_FAN_RPM_CUSTOM
board/genesis/board.h:#define CONFIG_FAN_RPM_CUSTOM
board/moonbuggy/board.h:#define CONFIG_FAN_RPM_CUSTOM
board/morphius/board.h:#define CONFIG_FAN_RPM_CUSTOM
board/scout/board.h:#define CONFIG_FAN_RPM_CUSTOM

This includes e.g. from board/ambassador/board.c:

static const struct fan_step_1_1 fan_table0[] = {
	{ .decreasing_temp_ratio_threshold = TEMP_TO_RATIO(35),
	  .increasing_temp_ratio_threshold = TEMP_TO_RATIO(41),
	  .rpm = 2500 },
	{ .decreasing_temp_ratio_threshold = TEMP_TO_RATIO(40),
	  .increasing_temp_ratio_threshold = TEMP_TO_RATIO(44),
	  .rpm = 2900 },
	{ .decreasing_temp_ratio_threshold = TEMP_TO_RATIO(42),
	  .increasing_temp_ratio_threshold = TEMP_TO_RATIO(46),
	  .rpm = 3400 },
	{ .decreasing_temp_ratio_threshold = TEMP_TO_RATIO(44),
	  .increasing_temp_ratio_threshold = TEMP_TO_RATIO(48),
	  .rpm = 3900 },
	{ .decreasing_temp_ratio_threshold = TEMP_TO_RATIO(46),
	  .increasing_temp_ratio_threshold = TEMP_TO_RATIO(50),
	  .rpm = 4400 },
	{ .decreasing_temp_ratio_threshold = TEMP_TO_RATIO(48),
	  .increasing_temp_ratio_threshold = TEMP_TO_RATIO(52),
	  .rpm = 4900 },
	{ .decreasing_temp_ratio_threshold = TEMP_TO_RATIO(50),
	  .increasing_temp_ratio_threshold = TEMP_TO_RATIO(55),
	  .rpm = 5200 },
};
#define NUM_FAN_LEVELS ARRAY_SIZE(fan_table0)

static const struct fan_step_1_1 *fan_table = fan_table0;

int fan_percent_to_rpm(int fan_index, int temp_ratio)
{
	return temp_ratio_to_rpm_hysteresis(fan_table, NUM_FAN_LEVELS,
					    fan_index, temp_ratio, NULL);
}

I'm going to see if adding "custom" control for puff is enough to resolve the issue. It seems that temp_ratio_to_rpm_hysterisis() is smarter than the "default" one as used by puff and dooly:

int temp_ratio_to_rpm_hysteresis(const struct fan_step_1_1 *fan_table,
				 int num_fan_levels, int fan_index,
				 int temp_ratio, void (*on_change)(void))
{
	static int previous_temp_ratio;
	const int previous_rpm = fan_get_rpm_target(FAN_CH(fan_index));
	int rpm;

	if (temp_ratio <= fan_table[0].decreasing_temp_ratio_threshold) {
		rpm = 0;
	} else if (previous_rpm == 0 &&
		   temp_ratio < fan_table[0].increasing_temp_ratio_threshold) {
		rpm = 0;
	} else {
		/*
		 * Comparing temp_ratio and previous_temp_ratio, trichotomy:
		 *  1. decreasing path. (check the decreasing threshold)
		 *  2. increasing path. (check the increasing threshold)
		 *  3. invariant path. (return the current RPM)
		 */
		if (temp_ratio < previous_temp_ratio) {
			int i = num_fan_levels - 1;

			while (i > 0 &&
			       fan_table[i].decreasing_temp_ratio_threshold >=
				       temp_ratio) {
				i--;
			}
			rpm = fan_table[i].rpm;
		} else if (temp_ratio > previous_temp_ratio) {
			int i = 0;

			while (i < num_fan_levels &&
			       fan_table[i].increasing_temp_ratio_threshold <=
				       temp_ratio) {
				i++;
			}
			if (i > 0) {
				i--;
			}
			rpm = fan_table[i].rpm;
		} else {
			rpm = previous_rpm;
		}
	}

	previous_temp_ratio = temp_ratio;

	if (rpm != previous_rpm) {
		cprints(CC_THERMAL, "Setting fan %d RPM to %d", fan_index, rpm);
		if (on_change) {
			on_change();
		}
	}

	return rpm;
}

[MrChromebox]

I'm going to see if adding "custom" control for puff is enough to resolve the issue. It seems that temp_ratio_to_rpm_hysterisis() is smarter than the "default" one as used by puff and dooly:

SGTM!