[PATCH v4] platform/chrome: cros_ec_sensorhub: Modify clock drift estimation

[PATCH v4] platform/chrome: cros_ec_sensorhub: Modify clock drift estimation

[Satyam Gupta]

From: Scott Collyer <scollyer@google.com>

Update the clock drift estimation and timestamp filter to prevent
monotonic backward skews and Out Of Order (OOO) timestamp errors during
sensor batching.

To keep the timestamp filter active across normal batching flush
intervals (which can be 1 to 2 seconds long), increase
TS_HISTORY_BORED_US from 0.5s to 2.0s. Because transitions between
active streaming and low-frequency batching may now fall within that 2.0s
window, detect Output Data Rate (ODR) or mode changes where the time
delta (dx) changes by more than 10x and reset the filter history so
streaming and batched samples are not mixed. Additionally, fix the
history shift loop starting index using min_t() so state->x_history[1] is
properly populated as soon as history_len reaches 2, and clamp the
calculated drift slope (m) within +/- MAX_DRIFT_PPM Parts Per Million
(PPM) so temporary jitter spikes cannot corrupt the median filter.

Finally, when a sample timestamp is strictly earlier than the newest
event seen for that sensor, drop the Out Of Order sample instead of
clamping its timestamp to last_ts, which previously caused downstream
batch spreading logic to misinterpret stale samples as a batch of
identical timestamps.

Signed-off-by: Scott Collyer <scollyer@google.com>
Signed-off-by: Satyam Gupta <sgsatyam@google.com>
---
Changes in v4:
- Reword commit description to clarify why the batching filter changes form a
  single logical change, and spell out Out Of Order (OOO) and Parts Per
  Million (PPM).
- Fix Signed-off-by capitalization.
- Drop redundant inline comment on TS_HISTORY_BORED_US.
- Use min_t(int, state->history_len, CROS_EC_SENSORHUB_TS_HISTORY_SIZE - 1)
  for history shift loop start index.
- Rename cros_ec_sensor_ring_check_for_past_timestamp() to
  cros_ec_sensor_ring_is_past_timestamp() and return true when the sample has
  a past timestamp.
 .../platform/chrome/cros_ec_sensorhub_ring.c  | 73 ++++++++++++++-----
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sensorhub_ring.c b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
index cc038af59218..20fa47cbee32 100644
--- a/drivers/platform/chrome/cros_ec_sensorhub_ring.c
+++ b/drivers/platform/chrome/cros_ec_sensorhub_ring.c
@@ -22,15 +22,21 @@
 
 /* Precision of fixed point for the m values from the filter */
 #define M_PRECISION BIT(23)
+#define MAX_DRIFT_PPM 200LL
+#define MAX_M ((s64)(MAX_DRIFT_PPM * M_PRECISION / 1000000))
 
 /* Only activate the filter once we have at least this many elements. */
 #define TS_HISTORY_THRESHOLD 8
 
 /*
  * If we don't have any history entries for this long, empty the filter to
- * make sure there are no big discontinuities.
+ * make sure there are no big discontinuities (e.g. after suspend or long idle).
+ *
+ * This is set to 2 seconds to accommodate normal batching flush intervals
+ * (which can be up to 1-2 seconds) without constantly resetting the filter,
+ * while still ensuring a reset occurs during actual long gaps or suspend.
  */
-#define TS_HISTORY_BORED_US 500000
+#define TS_HISTORY_BORED_US 2000000
 
 /* To measure by how much the filter is overshooting, if it happens. */
 #define FUTURE_TS_ANALYTICS_COUNT_MAX 100
@@ -259,7 +265,7 @@ cros_ec_sensor_ring_ts_filter_update(struct cros_ec_sensors_ts_filter_state
 	s64 m; /* stored as *M_PRECISION */
 	s64 *m_history_copy = state->temp_buf;
 	s64 *error = state->temp_buf;
-	int i;
+	int i, start;
 
 	/* we trust b the most, that'll be our independent variable */
 	x = b;
@@ -271,13 +277,36 @@ cros_ec_sensor_ring_ts_filter_update(struct cros_ec_sensors_ts_filter_state
 		return; /* we already have this irq in the history */
 	dy = (state->y_history[0] + state->y_offset) - y;
 	m = div64_s64(dy * M_PRECISION, dx);
+	/*
+	 * Clamp the calculated slope to a realistic range (+/- 200 ppm).
+	 * Temporary large jitter spikes can produce huge slopes (e.g. +/- 20000 ppm)
+	 * that would otherwise corrupt the median filter history.
+	 */
+	m = clamp(m, -MAX_M, MAX_M);
+
+	/*
+	 * Detect ODR (Output Data Rate) or Mode changes (e.g., transition from
+	 * active streaming to low-frequency batching). If the time delta (dx)
+	 * changes by more than 10x in either direction, reset the filter history
+	 * to prevent mixing noisy active samples with batching samples.
+	 */
+	if (state->history_len > 1) {
+		s64 prev_dx = -state->x_history[1];
+
+		if (prev_dx > 0 && (abs(dx) > 10 * prev_dx || prev_dx > 10 * abs(dx))) {
+			state->history_len = 0;
+			m = 0;
+		}
+	}
 
 	/* Empty filter if we haven't seen any action in a while. */
 	if (-dx > TS_HISTORY_BORED_US)
 		state->history_len = 0;
 
 	/* Move everything over, also update offset to all absolute coords .*/
-	for (i = state->history_len - 1; i >= 1; i--) {
+	start = min_t(int, state->history_len,
+		      CROS_EC_SENSORHUB_TS_HISTORY_SIZE - 1);
+	for (i = start; i >= 1; i--) {
 		state->x_history[i] = state->x_history[i - 1] + dx;
 		state->y_history[i] = state->y_history[i - 1] + dy;
 
@@ -398,23 +427,29 @@ cros_ec_sensor_ring_fix_overflow(s64 *ts,
 	state->last = *ts;
 }
 
-static void
-cros_ec_sensor_ring_check_for_past_timestamp(struct cros_ec_sensorhub
-					     *sensorhub,
-					     struct cros_ec_sensors_ring_sample
-					     *sample)
+static bool
+cros_ec_sensor_ring_is_past_timestamp(struct cros_ec_sensorhub *sensorhub,
+				      struct cros_ec_sensors_ring_sample *sample)
 {
 	const u8 sensor_id = sample->sensor_id;
 
-	/* If this event is earlier than one we saw before... */
+	/*
+	 * If this event is strictly earlier than one we saw before, it is truly
+	 * out-of-order and must be dropped to preserve monotonicity.
+	 *
+	 * We intentionally allow duplicate timestamps (==) to pass through here
+	 * because the EC/ISH may report a batch of samples with the same raw
+	 * timestamp. These duplicates are required by the downstream spreading
+	 * logic (cros_ec_sensor_ring_spread_add) to interpolate unique,
+	 * monotonic timestamps for each sample in the batch.
+	 */
 	if (sensorhub->batch_state[sensor_id].newest_sensor_event >
 	    sample->timestamp)
-		/* mark it for spreading. */
-		sample->timestamp =
-			sensorhub->batch_state[sensor_id].last_ts;
-	else
-		sensorhub->batch_state[sensor_id].newest_sensor_event =
-			sample->timestamp;
+		return true;
+
+	sensorhub->batch_state[sensor_id].newest_sensor_event =
+		sample->timestamp;
+	return false;
 }
 
 /**
@@ -547,8 +582,10 @@ cros_ec_sensor_ring_process_event(struct cros_ec_sensorhub *sensorhub,
 	for (axis = 0; axis < 3; axis++)
 		out->vector[axis] = in->data[axis];
 
-	if (sensorhub->tight_timestamps)
-		cros_ec_sensor_ring_check_for_past_timestamp(sensorhub, out);
+	if (sensorhub->tight_timestamps) {
+		if (cros_ec_sensor_ring_is_past_timestamp(sensorhub, out))
+			return false;
+	}
 	return true;
 }
 
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH v4] platform/chrome: cros_ec_sensorhub: Modify clock drift estimation

[Tzung-Bi Shih]

On Tue, Oct 13, 2026 at 10:57:53AM +0000, Satyam Gupta wrote:
>  /* Precision of fixed point for the m values from the filter */
>  #define M_PRECISION BIT(23)
> +#define MAX_DRIFT_PPM 200LL
> +#define MAX_M ((s64)(MAX_DRIFT_PPM * M_PRECISION / 1000000))
...
> @@ -271,13 +277,36 @@ cros_ec_sensor_ring_ts_filter_update(struct cros_ec_sensors_ts_filter_state
>  		return; /* we already have this irq in the history */
>  	dy = (state->y_history[0] + state->y_offset) - y;
>  	m = div64_s64(dy * M_PRECISION, dx);
> +	/*
> +	 * Clamp the calculated slope to a realistic range (+/- 200 ppm).
> +	 * Temporary large jitter spikes can produce huge slopes (e.g. +/- 20000 ppm)
> +	 * that would otherwise corrupt the median filter history.
> +	 */
> +	m = clamp(m, -MAX_M, MAX_M);

[This is one independent patch.]

There is a unit mismatch here that makes MAX_M off by a factor of 1000,
clamping the drift slope to +/-0.2 PPM instead of the intended +/-200
PPM.

See cros_ec_sensor_ring_ts_filter_update():
- dx is in us; dy is in ns.
- m = dy/dx is actually off by a factor of 1000.

> +
> +	/*
> +	 * Detect ODR (Output Data Rate) or Mode changes (e.g., transition from
> +	 * active streaming to low-frequency batching). If the time delta (dx)
> +	 * changes by more than 10x in either direction, reset the filter history
> +	 * to prevent mixing noisy active samples with batching samples.
> +	 */
> +	if (state->history_len > 1) {
> +		s64 prev_dx = -state->x_history[1];
> +
> +		if (prev_dx > 0 && (abs(dx) > 10 * prev_dx || prev_dx > 10 * abs(dx))) {
> +			state->history_len = 0;
> +			m = 0;
> +		}
> +	}

[This is one independent patch.]

Setting `m = 0;` is unnecessary.  Setting `state->history_len = 0;` is
sufficient to reset the filter.

The code elsewhere uses `-dx`.  Using `abs(dx)` is inconsistent and would
treat backward timestamps as valid deltas.

>  
>  	/* Empty filter if we haven't seen any action in a while. */
>  	if (-dx > TS_HISTORY_BORED_US)
>  		state->history_len = 0;
>  
>  	/* Move everything over, also update offset to all absolute coords .*/
> -	for (i = state->history_len - 1; i >= 1; i--) {
> +	start = min_t(int, state->history_len,
> +		      CROS_EC_SENSORHUB_TS_HISTORY_SIZE - 1);
> +	for (i = start; i >= 1; i--) {
>  		state->x_history[i] = state->x_history[i - 1] + dx;
>  		state->y_history[i] = state->y_history[i - 1] + dy;
>  

[This is one independent patch.]

> -static void
> -cros_ec_sensor_ring_check_for_past_timestamp(struct cros_ec_sensorhub
> -					     *sensorhub,
> -					     struct cros_ec_sensors_ring_sample
> -					     *sample)
> +static bool
> +cros_ec_sensor_ring_is_past_timestamp(struct cros_ec_sensorhub *sensorhub,
> +				      struct cros_ec_sensors_ring_sample *sample)
>  {
>  	const u8 sensor_id = sample->sensor_id;
>  
> -	/* If this event is earlier than one we saw before... */
> +	/*
> +	 * If this event is strictly earlier than one we saw before, it is truly
> +	 * out-of-order and must be dropped to preserve monotonicity.
> +	 *
> +	 * We intentionally allow duplicate timestamps (==) to pass through here
> +	 * because the EC/ISH may report a batch of samples with the same raw
> +	 * timestamp. These duplicates are required by the downstream spreading
> +	 * logic (cros_ec_sensor_ring_spread_add) to interpolate unique,
> +	 * monotonic timestamps for each sample in the batch.
> +	 */
>  	if (sensorhub->batch_state[sensor_id].newest_sensor_event >
>  	    sample->timestamp)
> -		/* mark it for spreading. */
> -		sample->timestamp =
> -			sensorhub->batch_state[sensor_id].last_ts;
> -	else
> -		sensorhub->batch_state[sensor_id].newest_sensor_event =
> -			sample->timestamp;
> +		return true;
> +
> +	sensorhub->batch_state[sensor_id].newest_sensor_event =
> +		sample->timestamp;
> +	return false;
>  }
> 
>  /**
> @@ -547,8 +582,10 @@  cros_ec_sensor_ring_process_event(struct cros_ec_sensorhub *sensorhub,
>  	for (axis = 0; axis < 3; axis++)
>  		out->vector[axis] = in->data[axis];
> 
> -	if (sensorhub->tight_timestamps)
> -		cros_ec_sensor_ring_check_for_past_timestamp(sensorhub, out);
> +	if (sensorhub->tight_timestamps) {
> +		if (cros_ec_sensor_ring_is_past_timestamp(sensorhub, out))
> +			return false;
> +	}

[This is one independent patch.]

Dropping a sample without setting `batch_state[sensor_id].last_len = 0`
leaves the previous batch length intact.  The next batch will compute
`sample_period` across the gap using the stale `last_len`.