[PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Martin Kelly]

When interrupts are generated at a slower rate than the FIFO fills up,
we will have fewer timestamps than samples. Currently, we fill in 0 for
any unmatched timestamps. However, this is very confusing for userspace,
which does not expect discontinuities in timestamps and must somehow
work around the issue.

Improve the situation by interpolating timestamps when we can't map them
1:1 to data. We do this by assuming the timestamps we have are the most
recent and interpolating backwards to fill the older data. This makes
sense because inv_mpu6050_read_fifo gets called right after an interrupt
is generated, presumably when a datum was generated, so the most recent
timestamp matches up with the most recent datum. In addition, this
assumption is borne out by observation, which shows monotonically
increasing timestamps when interpolating this way but discontinuities
when interpolating in the other direction.

Although this method is not perfectly accurate, it is probably the best
we can do if we don't get one interrupt per datum.

Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  2 ++
 drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  2 ++
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 50 +++++++++++++++++++++++++++---
 3 files changed, 50 insertions(+), 4 deletions(-)

v1:
- Set a missing timestamp to the last timestamp we saw.
v2:
- Interpolate missing timestamps.
v3:
- Slight optimization to move timestamp_interp += into the result == 0 case.

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 7d64be353403..4a95ff8df3b9 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -83,6 +83,7 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = {
 	.fsr = INV_MPU6050_FSR_2000DPS,
 	.lpf = INV_MPU6050_FILTER_20HZ,
 	.fifo_rate = INV_MPU6050_INIT_FIFO_RATE,
+	.fifo_period = NSEC_PER_SEC / INV_MPU6050_INIT_FIFO_RATE,
 	.gyro_fifo_enable = false,
 	.accl_fifo_enable = false,
 	.accl_fs = INV_MPU6050_FS_02G,
@@ -630,6 +631,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
 	if (result)
 		goto fifo_rate_fail_power_off;
 	st->chip_config.fifo_rate = fifo_rate;
+	st->chip_config.fifo_period = NSEC_PER_SEC / fifo_rate;
 
 	result = inv_mpu6050_set_lpf(st, fifo_rate);
 	if (result)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
index 065794162d65..3bc7d62822ca 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
@@ -86,6 +86,7 @@ enum inv_devices {
  *  @accl_fifo_enable:	enable accel data output
  *  @gyro_fifo_enable:	enable gyro data output
  *  @fifo_rate:		FIFO update rate.
+ *  @fifo_period:	FIFO update period, in nanoseconds.
  */
 struct inv_mpu6050_chip_config {
 	unsigned int fsr:2;
@@ -94,6 +95,7 @@ struct inv_mpu6050_chip_config {
 	unsigned int accl_fifo_enable:1;
 	unsigned int gyro_fifo_enable:1;
 	u16 fifo_rate;
+	u32 fifo_period;
 };
 
 /**
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
index ff81c6aa009d..40610a316eb0 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
@@ -126,7 +126,11 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	int result;
 	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
 	u16 fifo_count;
+	u16 fifo_items;
+	s32 fifo_diff;
 	s64 timestamp;
+	s64 timestamp_interp;
+	s64 offset;
 
 	mutex_lock(&st->lock);
 	if (!(st->chip_config.accl_fifo_enable |
@@ -156,9 +160,25 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 	if (fifo_count >  INV_MPU6050_FIFO_THRESHOLD)
 		goto flush_fifo;
 	/* Timestamp mismatch. */
+	fifo_items = fifo_count / bytes_per_datum;
 	if (kfifo_len(&st->timestamps) >
-	    fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
+	    fifo_items + INV_MPU6050_TIME_STAMP_TOR)
 		goto flush_fifo;
+
+	/*
+	 * If we have more data than timestamps, the timestamps we have
+	 * correspond to the newest items in the FIFO, since some data was
+	 * generated without a corresponding interrupt and thus timestamp. Since
+	 * we remove FIFO items oldest to newest, we need to interpolate the
+	 * older timestamps based on the number of missing timestamps.
+	 */
+	fifo_diff = fifo_items - kfifo_len(&st->timestamps);
+	if (fifo_diff > 0)
+		offset = st->chip_config.fifo_period * fifo_diff;
+	else
+		offset = 0;
+
+	timestamp_interp = 0;
 	while (fifo_count >= bytes_per_datum) {
 		result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
 					  data, bytes_per_datum);
@@ -166,9 +186,31 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 			goto flush_fifo;
 
 		result = kfifo_out(&st->timestamps, &timestamp, 1);
-		/* when there is no timestamp, put timestamp as 0 */
-		if (result == 0)
-			timestamp = 0;
+		/* When there is no timestamp, interpolate based on period */
+		if (result == 0) {
+			/*
+			 * We have no more timestamps left, so interpolate the
+			 * rest of them.
+			 */
+			if (likely(timestamp_interp != 0))
+				timestamp_interp += st->chip_config.fifo_period;
+			else
+				/*
+				 * In this unlikely error case, we should output
+				 * a 0 timestamp instead of 0 + FIFO period.
+				 */
+				dev_err(regmap_get_device(st->map),
+					"Timestamp FIFO is empty!\n");
+			timestamp = timestamp_interp;
+		} else {
+			/*
+			 * If we are interpolating, this is an older datum with
+			 * a newer timestamp, so offset it backwards to account
+			 * for the time gap.  Otherwise, offset will be 0.
+			 */
+			timestamp -= offset;
+			timestamp_interp = timestamp;
+		}
 
 		result = iio_push_to_buffers_with_timestamp(indio_dev, data,
 							    timestamp);
-- 
2.11.0

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Jonathan Cameron]

On Wed, 28 Mar 2018 10:40:29 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> When interrupts are generated at a slower rate than the FIFO fills up,
> we will have fewer timestamps than samples. Currently, we fill in 0 for
> any unmatched timestamps. However, this is very confusing for userspace,
> which does not expect discontinuities in timestamps and must somehow
> work around the issue.
> 
> Improve the situation by interpolating timestamps when we can't map them
> 1:1 to data. We do this by assuming the timestamps we have are the most
> recent and interpolating backwards to fill the older data. This makes
> sense because inv_mpu6050_read_fifo gets called right after an interrupt
> is generated, presumably when a datum was generated, so the most recent
> timestamp matches up with the most recent datum. In addition, this
> assumption is borne out by observation, which shows monotonically
> increasing timestamps when interpolating this way but discontinuities
> when interpolating in the other direction.
> 
> Although this method is not perfectly accurate, it is probably the best
> we can do if we don't get one interrupt per datum.
This patch worries me somewhat - we are basically guessing what the cause
of the missed interrupts was.  If for example the fifo read itself
takes a long time, the interrupt time we have is actually valid for
the first sample and we should in interpolating forwards in time.

It sounds from discussions like something else is broken on your
board.  I like the idea of interpolating interrupts, but would like to
conduct the same experiments you did when we are running at high speeds
and seeing misses - not on the test case you currently have.

The problem then will be estimating how long the interrupt took to
be handed vs the read out rate of the fifo.  Chances are our 'correct'
timestamp is somewhat after the first reading but well before the last
one.

Anyhow, let us know once you have the thing running properly at
high speed then we can work out how best to address this.

Jonathan

> 
> Signed-off-by: Martin Kelly <mkelly@xevo.com>
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c |  2 ++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h  |  2 ++
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 50 +++++++++++++++++++++++++++---
>  3 files changed, 50 insertions(+), 4 deletions(-)
> 
> v1:
> - Set a missing timestamp to the last timestamp we saw.
> v2:
> - Interpolate missing timestamps.
> v3:
> - Slight optimization to move timestamp_interp += into the result == 0 case.
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 7d64be353403..4a95ff8df3b9 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -83,6 +83,7 @@ static const struct inv_mpu6050_chip_config chip_config_6050 = {
>  	.fsr = INV_MPU6050_FSR_2000DPS,
>  	.lpf = INV_MPU6050_FILTER_20HZ,
>  	.fifo_rate = INV_MPU6050_INIT_FIFO_RATE,
> +	.fifo_period = NSEC_PER_SEC / INV_MPU6050_INIT_FIFO_RATE,
>  	.gyro_fifo_enable = false,
>  	.accl_fifo_enable = false,
>  	.accl_fs = INV_MPU6050_FS_02G,
> @@ -630,6 +631,7 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
>  	if (result)
>  		goto fifo_rate_fail_power_off;
>  	st->chip_config.fifo_rate = fifo_rate;
> +	st->chip_config.fifo_period = NSEC_PER_SEC / fifo_rate;
>  
>  	result = inv_mpu6050_set_lpf(st, fifo_rate);
>  	if (result)
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> index 065794162d65..3bc7d62822ca 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h
> @@ -86,6 +86,7 @@ enum inv_devices {
>   *  @accl_fifo_enable:	enable accel data output
>   *  @gyro_fifo_enable:	enable gyro data output
>   *  @fifo_rate:		FIFO update rate.
> + *  @fifo_period:	FIFO update period, in nanoseconds.
>   */
>  struct inv_mpu6050_chip_config {
>  	unsigned int fsr:2;
> @@ -94,6 +95,7 @@ struct inv_mpu6050_chip_config {
>  	unsigned int accl_fifo_enable:1;
>  	unsigned int gyro_fifo_enable:1;
>  	u16 fifo_rate;
> +	u32 fifo_period;
>  };
>  
>  /**
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index ff81c6aa009d..40610a316eb0 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -126,7 +126,11 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  	int result;
>  	u8 data[INV_MPU6050_OUTPUT_DATA_SIZE];
>  	u16 fifo_count;
> +	u16 fifo_items;
> +	s32 fifo_diff;
>  	s64 timestamp;
> +	s64 timestamp_interp;
> +	s64 offset;
>  
>  	mutex_lock(&st->lock);
>  	if (!(st->chip_config.accl_fifo_enable |
> @@ -156,9 +160,25 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  	if (fifo_count >  INV_MPU6050_FIFO_THRESHOLD)
>  		goto flush_fifo;
>  	/* Timestamp mismatch. */
> +	fifo_items = fifo_count / bytes_per_datum;
>  	if (kfifo_len(&st->timestamps) >
> -	    fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
> +	    fifo_items + INV_MPU6050_TIME_STAMP_TOR)
>  		goto flush_fifo;
> +
> +	/*
> +	 * If we have more data than timestamps, the timestamps we have
> +	 * correspond to the newest items in the FIFO, since some data was
> +	 * generated without a corresponding interrupt and thus timestamp. Since
> +	 * we remove FIFO items oldest to newest, we need to interpolate the
> +	 * older timestamps based on the number of missing timestamps.
> +	 */
> +	fifo_diff = fifo_items - kfifo_len(&st->timestamps);
> +	if (fifo_diff > 0)
> +		offset = st->chip_config.fifo_period * fifo_diff;
> +	else
> +		offset = 0;
> +
> +	timestamp_interp = 0;
>  	while (fifo_count >= bytes_per_datum) {
>  		result = regmap_bulk_read(st->map, st->reg->fifo_r_w,
>  					  data, bytes_per_datum);
> @@ -166,9 +186,31 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
>  			goto flush_fifo;
>  
>  		result = kfifo_out(&st->timestamps, &timestamp, 1);
> -		/* when there is no timestamp, put timestamp as 0 */
> -		if (result == 0)
> -			timestamp = 0;
> +		/* When there is no timestamp, interpolate based on period */
> +		if (result == 0) {
> +			/*
> +			 * We have no more timestamps left, so interpolate the
> +			 * rest of them.
> +			 */
> +			if (likely(timestamp_interp != 0))
> +				timestamp_interp += st->chip_config.fifo_period;
> +			else
> +				/*
> +				 * In this unlikely error case, we should output
> +				 * a 0 timestamp instead of 0 + FIFO period.
> +				 */
> +				dev_err(regmap_get_device(st->map),
> +					"Timestamp FIFO is empty!\n");
> +			timestamp = timestamp_interp;
> +		} else {
> +			/*
> +			 * If we are interpolating, this is an older datum with
> +			 * a newer timestamp, so offset it backwards to account
> +			 * for the time gap.  Otherwise, offset will be 0.
> +			 */
> +			timestamp -= offset;
> +			timestamp_interp = timestamp;
> +		}
>  
>  		result = iio_push_to_buffers_with_timestamp(indio_dev, data,
>  							    timestamp);

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Martin Kelly]

On 03/30/2018 03:36 AM, Jonathan Cameron wrote:
> On Wed, 28 Mar 2018 10:40:29 -0700
> Martin Kelly <mkelly@xevo.com> wrote:
> 
>> When interrupts are generated at a slower rate than the FIFO fills up,
>> we will have fewer timestamps than samples. Currently, we fill in 0 for
>> any unmatched timestamps. However, this is very confusing for userspace,
>> which does not expect discontinuities in timestamps and must somehow
>> work around the issue.
>>
>> Improve the situation by interpolating timestamps when we can't map them
>> 1:1 to data. We do this by assuming the timestamps we have are the most
>> recent and interpolating backwards to fill the older data. This makes
>> sense because inv_mpu6050_read_fifo gets called right after an interrupt
>> is generated, presumably when a datum was generated, so the most recent
>> timestamp matches up with the most recent datum. In addition, this
>> assumption is borne out by observation, which shows monotonically
>> increasing timestamps when interpolating this way but discontinuities
>> when interpolating in the other direction.
>>
>> Although this method is not perfectly accurate, it is probably the best
>> we can do if we don't get one interrupt per datum.
> This patch worries me somewhat - we are basically guessing what the cause
> of the missed interrupts was.  If for example the fifo read itself
> takes a long time, the interrupt time we have is actually valid for
> the first sample and we should in interpolating forwards in time.
> 

I agree with you that if the delay is somewhere after the IRQ fires but 
prior to the FIFO read (or the FIFO read itself), then we'd need forward 
interpolation. That said, if there is more than 1 sample in FIFO when 
the IRQ fires, we need backwards interpolation (since those samples were 
certainly created prior to the IRQ firing and the timestamp being 
generated). I believe the current patch is will accurately handle the 
backwards interpolation case but be wrong for the forwards interpolation 
case (though it will at least guarantee samples to be monotonically 
increasing, as they should be).

I thought about this and I think we can fix both issues. I propose we do 
the following:

- When the IRQ fires, immediately timestamp as we already do. Also, 
immediately check the FIFO length and store it (call this length n).

- Right after reading from the FIFO length (calling regmap_bulk_read() 
at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length m.

- The first n samples (those present when the IRQ fired) should be 
backwards-interpolated from the timestamp taken when the IRQ fired.

- If the two FIFO lengths taken are different (m - n > 0), then some 
samples were generated between the IRQ firing and the FIFO being read. 
This could be caused by a slow read, a delay firing off the bottom half 
of the IRQ, or some other delay in the inv_mpu6050_read_fifo() function. 
In any case, these m - n samples need to be forward-interpolated between 
the two timestamps we took, since they were generated sometime between 
the two timestamps.

I believe this approach will fix both issues. Although my board is not 
seeing the second issue (where m - n > 0), I can simulate it by adding 
artificial delays.

In addition to fixing my issue, this should make the code more resilient 
to unknown bus and IRQ issues in the future.

Jonathan, how does this approach sound to you?

> It sounds from discussions like something else is broken on your
> board.  I like the idea of interpolating interrupts, but would like to
> conduct the same experiments you did when we are running at high speeds
> and seeing misses - not on the test case you currently have.
> 
> The problem then will be estimating how long the interrupt took to
> be handed vs the read out rate of the fifo.  Chances are our 'correct'
> timestamp is somewhat after the first reading but well before the last
> one.
> 
> Anyhow, let us know once you have the thing running properly at
> high speed then we can work out how best to address this.
> 
> Jonathan
> 

Once I get the thing working, I will definitely look again at the 
interpolation and make sure it's still valid. If I see any anomalies, 
I'll let you know.

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Jonathan Cameron]

On Mon, 2 Apr 2018 10:07:57 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> On 03/30/2018 03:36 AM, Jonathan Cameron wrote:
> > On Wed, 28 Mar 2018 10:40:29 -0700
> > Martin Kelly <mkelly@xevo.com> wrote:
> >   
> >> When interrupts are generated at a slower rate than the FIFO fills up,
> >> we will have fewer timestamps than samples. Currently, we fill in 0 for
> >> any unmatched timestamps. However, this is very confusing for userspace,
> >> which does not expect discontinuities in timestamps and must somehow
> >> work around the issue.
> >>
> >> Improve the situation by interpolating timestamps when we can't map them
> >> 1:1 to data. We do this by assuming the timestamps we have are the most
> >> recent and interpolating backwards to fill the older data. This makes
> >> sense because inv_mpu6050_read_fifo gets called right after an interrupt
> >> is generated, presumably when a datum was generated, so the most recent
> >> timestamp matches up with the most recent datum. In addition, this
> >> assumption is borne out by observation, which shows monotonically
> >> increasing timestamps when interpolating this way but discontinuities
> >> when interpolating in the other direction.
> >>
> >> Although this method is not perfectly accurate, it is probably the best
> >> we can do if we don't get one interrupt per datum.  
> > This patch worries me somewhat - we are basically guessing what the cause
> > of the missed interrupts was.  If for example the fifo read itself
> > takes a long time, the interrupt time we have is actually valid for
> > the first sample and we should in interpolating forwards in time.
> >   
> 
> I agree with you that if the delay is somewhere after the IRQ fires but 
> prior to the FIFO read (or the FIFO read itself), then we'd need forward 
> interpolation. That said, if there is more than 1 sample in FIFO when 
> the IRQ fires, we need backwards interpolation (since those samples were 
> certainly created prior to the IRQ firing and the timestamp being 
> generated). I believe the current patch is will accurately handle the 
> backwards interpolation case but be wrong for the forwards interpolation 
> case (though it will at least guarantee samples to be monotonically 
> increasing, as they should be).
> 
> I thought about this and I think we can fix both issues. I propose we do 
> the following:
> 
> - When the IRQ fires, immediately timestamp as we already do. Also, 
> immediately check the FIFO length and store it (call this length n).
> 
> - Right after reading from the FIFO length (calling regmap_bulk_read() 
> at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length m.
> 
> - The first n samples (those present when the IRQ fired) should be 
> backwards-interpolated from the timestamp taken when the IRQ fired.
> 
> - If the two FIFO lengths taken are different (m - n > 0), then some 
> samples were generated between the IRQ firing and the FIFO being read. 
> This could be caused by a slow read, a delay firing off the bottom half 
> of the IRQ, or some other delay in the inv_mpu6050_read_fifo() function. 
> In any case, these m - n samples need to be forward-interpolated between 
> the two timestamps we took, since they were generated sometime between 
> the two timestamps.
> 
> I believe this approach will fix both issues. Although my board is not 
> seeing the second issue (where m - n > 0), I can simulate it by adding 
> artificial delays.
> 
> In addition to fixing my issue, this should make the code more resilient 
> to unknown bus and IRQ issues in the future.
> 
> Jonathan, how does this approach sound to you?
Great.

> 
> > It sounds from discussions like something else is broken on your
> > board.  I like the idea of interpolating interrupts, but would like to
> > conduct the same experiments you did when we are running at high speeds
> > and seeing misses - not on the test case you currently have.
> > 
> > The problem then will be estimating how long the interrupt took to
> > be handed vs the read out rate of the fifo.  Chances are our 'correct'
> > timestamp is somewhat after the first reading but well before the last
> > one.
> > 
> > Anyhow, let us know once you have the thing running properly at
> > high speed then we can work out how best to address this.
> > 
> > Jonathan
> >   
> 
> Once I get the thing working, I will definitely look again at the 
> interpolation and make sure it's still valid. If I see any anomalies, 
> I'll let you know.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Jean-Baptiste Maneyrol]

[-- Attachment #1: Type: text/plain, Size: 5606 bytes --]

Hello,


there is just a problem if I'm understanding well.


Reading FIFO count register under hard irq handler (when taking the timestamp) is not possible since i2c access is using a mutex. That's why we are using an irq thread for reading FIFO content.


I've been able to reproduce the issue when doing a long stress-test at high speed (200Hz). Perhaps this is related to the FIFO error handling which looks really over-complex for me. I am currently investigating this issue.


But I would be happy to test any new solution if proposed.


Best regards,

JB

________________________________
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Sent: Friday, April 6, 2018 5:15:09 PM
To: Martin Kelly
Cc: Jonathan Cameron; linux-iio@vger.kernel.org; Jean-Baptiste Maneyrol
Subject: Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

On Mon, 2 Apr 2018 10:07:57 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> On 03/30/2018 03:36 AM, Jonathan Cameron wrote:
> > On Wed, 28 Mar 2018 10:40:29 -0700
> > Martin Kelly <mkelly@xevo.com> wrote:
> >
> >> When interrupts are generated at a slower rate than the FIFO fills up,
> >> we will have fewer timestamps than samples. Currently, we fill in 0 for
> >> any unmatched timestamps. However, this is very confusing for userspace,
> >> which does not expect discontinuities in timestamps and must somehow
> >> work around the issue.
> >>
> >> Improve the situation by interpolating timestamps when we can't map them
> >> 1:1 to data. We do this by assuming the timestamps we have are the most
> >> recent and interpolating backwards to fill the older data. This makes
> >> sense because inv_mpu6050_read_fifo gets called right after an interrupt
> >> is generated, presumably when a datum was generated, so the most recent
> >> timestamp matches up with the most recent datum. In addition, this
> >> assumption is borne out by observation, which shows monotonically
> >> increasing timestamps when interpolating this way but discontinuities
> >> when interpolating in the other direction.
> >>
> >> Although this method is not perfectly accurate, it is probably the best
> >> we can do if we don't get one interrupt per datum.
> > This patch worries me somewhat - we are basically guessing what the cause
> > of the missed interrupts was.  If for example the fifo read itself
> > takes a long time, the interrupt time we have is actually valid for
> > the first sample and we should in interpolating forwards in time.
> >
>
> I agree with you that if the delay is somewhere after the IRQ fires but
> prior to the FIFO read (or the FIFO read itself), then we'd need forward
> interpolation. That said, if there is more than 1 sample in FIFO when
> the IRQ fires, we need backwards interpolation (since those samples were
> certainly created prior to the IRQ firing and the timestamp being
> generated). I believe the current patch is will accurately handle the
> backwards interpolation case but be wrong for the forwards interpolation
> case (though it will at least guarantee samples to be monotonically
> increasing, as they should be).
>
> I thought about this and I think we can fix both issues. I propose we do
> the following:
>
> - When the IRQ fires, immediately timestamp as we already do. Also,
> immediately check the FIFO length and store it (call this length n).
>
> - Right after reading from the FIFO length (calling regmap_bulk_read()
> at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length m.
>
> - The first n samples (those present when the IRQ fired) should be
> backwards-interpolated from the timestamp taken when the IRQ fired.
>
> - If the two FIFO lengths taken are different (m - n > 0), then some
> samples were generated between the IRQ firing and the FIFO being read.
> This could be caused by a slow read, a delay firing off the bottom half
> of the IRQ, or some other delay in the inv_mpu6050_read_fifo() function.
> In any case, these m - n samples need to be forward-interpolated between
> the two timestamps we took, since they were generated sometime between
> the two timestamps.
>
> I believe this approach will fix both issues. Although my board is not
> seeing the second issue (where m - n > 0), I can simulate it by adding
> artificial delays.
>
> In addition to fixing my issue, this should make the code more resilient
> to unknown bus and IRQ issues in the future.
>
> Jonathan, how does this approach sound to you?
Great.

>
> > It sounds from discussions like something else is broken on your
> > board.  I like the idea of interpolating interrupts, but would like to
> > conduct the same experiments you did when we are running at high speeds
> > and seeing misses - not on the test case you currently have.
> >
> > The problem then will be estimating how long the interrupt took to
> > be handed vs the read out rate of the fifo.  Chances are our 'correct'
> > timestamp is somewhat after the first reading but well before the last
> > one.
> >
> > Anyhow, let us know once you have the thing running properly at
> > high speed then we can work out how best to address this.
> >
> > Jonathan
> >
>
> Once I get the thing working, I will definitely look again at the
> interpolation and make sure it's still valid. If I see any anomalies,
> I'll let you know.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: Type: text/html, Size: 7881 bytes --]

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Jean-Baptiste Maneyrol]

Hello,

there is just a problem if I'm understanding well.

Reading FIFO count register under hard irq handler (when taking the timesta=
mp) is not possible since i2c/spi access is using a mutex. That's why we ar=
e using an irq thread for reading chip registers.

I've been able to reproduce the issue when doing a long stress-test at high=
 speed (200Hz). Perhaps this is related to the FIFO error handling which lo=
oks really over-complex for me. I am currently investigating this  issue.

But I would be happy to test any new solution if proposed.

Best regards,
JB
 =20
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Sent: Friday, April 6, 2018 5:15:09 PM
To: Martin Kelly
Cc: Jonathan Cameron; linux-iio@vger.kernel.org; Jean-Baptiste Maneyrol
Subject: Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps
=A0=20

On Mon, 2 Apr 2018 10:07:57 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> On 03/30/2018 03:36 AM, Jonathan Cameron wrote:
> > On Wed, 28 Mar 2018 10:40:29 -0700
> > Martin Kelly <mkelly@xevo.com> wrote:
> >=A0=A0=20
> >> When interrupts are generated at a slower rate than the FIFO fills up,
> >> we will have fewer timestamps than samples. Currently, we fill in 0 fo=
r
> >> any unmatched timestamps. However, this is very confusing for userspac=
e,
> >> which does not expect discontinuities in timestamps and must somehow
> >> work around the issue.
> >>
> >> Improve the situation by interpolating timestamps when we can't map th=
em
> >> 1:1 to data. We do this by assuming the timestamps we have are the mos=
t
> >> recent and interpolating backwards to fill the older data. This makes
> >> sense because inv_mpu6050_read_fifo gets called right after an interru=
pt
> >> is generated, presumably when a datum was generated, so the most recen=
t
> >> timestamp matches up with the most recent datum. In addition, this
> >> assumption is borne out by observation, which shows monotonically
> >> increasing timestamps when interpolating this way but discontinuities
> >> when interpolating in the other direction.
> >>
> >> Although this method is not perfectly accurate, it is probably the bes=
t
> >> we can do if we don't get one interrupt per datum.=A0=20
> > This patch worries me somewhat - we are basically guessing what the cau=
se
> > of the missed interrupts was.=A0 If for example the fifo read itself
> > takes a long time, the interrupt time we have is actually valid for
> > the first sample and we should in interpolating forwards in time.
> >=A0=A0=20
>=20
> I agree with you that if the delay is somewhere after the IRQ fires but=20
> prior to the FIFO read (or the FIFO read itself), then we'd need forward=
=20
> interpolation. That said, if there is more than 1 sample in FIFO when=20
> the IRQ fires, we need backwards interpolation (since those samples were=
=20
> certainly created prior to the IRQ firing and the timestamp being=20
> generated). I believe the current patch is will accurately handle the=20
> backwards interpolation case but be wrong for the forwards interpolation=
=20
> case (though it will at least guarantee samples to be monotonically=20
> increasing, as they should be).
>=20
> I thought about this and I think we can fix both issues. I propose we do=
=20
> the following:
>=20
> - When the IRQ fires, immediately timestamp as we already do. Also,=20
> immediately check the FIFO length and store it (call this length n).
>=20
> - Right after reading from the FIFO length (calling regmap_bulk_read()=20
> at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length m.
>=20
> - The first n samples (those present when the IRQ fired) should be=20
> backwards-interpolated from the timestamp taken when the IRQ fired.
>=20
> - If the two FIFO lengths taken are different (m - n > 0), then some=20
> samples were generated between the IRQ firing and the FIFO being read.=20
> This could be caused by a slow read, a delay firing off the bottom half=20
> of the IRQ, or some other delay in the inv_mpu6050_read_fifo() function.=
=20
> In any case, these m - n samples need to be forward-interpolated between=
=20
> the two timestamps we took, since they were generated sometime between=20
> the two timestamps.
>=20
> I believe this approach will fix both issues. Although my board is not=20
> seeing the second issue (where m - n > 0), I can simulate it by adding=20
> artificial delays.
>=20
> In addition to fixing my issue, this should make the code more resilient=
=20
> to unknown bus and IRQ issues in the future.
>=20
> Jonathan, how does this approach sound to you?
Great.

>=20
> > It sounds from discussions like something else is broken on your
> > board.=A0 I like the idea of interpolating interrupts, but would like t=
o
> > conduct the same experiments you did when we are running at high speeds
> > and seeing misses - not on the test case you currently have.
> >=20
> > The problem then will be estimating how long the interrupt took to
> > be handed vs the read out rate of the fifo.=A0 Chances are our 'correct=
'
> > timestamp is somewhat after the first reading but well before the last
> > one.
> >=20
> > Anyhow, let us know once you have the thing running properly at
> > high speed then we can work out how best to address this.
> >=20
> > Jonathan
> >=A0=A0=20
>=20
> Once I get the thing working, I will definitely look again at the=20
> interpolation and make sure it's still valid. If I see any anomalies,=20
> I'll let you know.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at=A0 http://vger.kernel.org/majordomo-info.html


VGER.KERNEL.ORG - Majordomo info
vger.kernel.org
Very short Majordomo intro. Send request in email to address <majordomo@vge=
r.kernel.org> . To subscribe a list (``linux-kernel'' is given as an exampl=
e), use following as the only content of your letter:

     =

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Jonathan Cameron]

On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
>Hello,
>
>
>there is just a problem if I'm understanding well.
>
>
>Reading FIFO count register under hard irq handler (when taking the
>timestamp) is not possible since i2c access is using a mutex. That's
>why we are using an irq thread for reading FIFO content.

Good point. Need more sleep or caffeine!


>
>
>I've been able to reproduce the issue when doing a long stress-test at
>high speed (200Hz). Perhaps this is related to the FIFO error handling
>which looks really over-complex for me. I am currently investigating
>this issue.
>
>
>But I would be happy to test any new solution if proposed.
>
>
>Best regards,
>
>JB
>
>________________________________
>From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>Sent: Friday, April 6, 2018 5:15:09 PM
>To: Martin Kelly
>Cc: Jonathan Cameron; linux-iio@vger.kernel.org; Jean-Baptiste Maneyrol
>Subject: Re: [PATCH v3] imu: inv_mpu6050: interpolate missing
>timestamps
>
>On Mon, 2 Apr 2018 10:07:57 -0700
>Martin Kelly <mkelly@xevo.com> wrote:
>
>> On 03/30/2018 03:36 AM, Jonathan Cameron wrote:
>> > On Wed, 28 Mar 2018 10:40:29 -0700
>> > Martin Kelly <mkelly@xevo.com> wrote:
>> >
>> >> When interrupts are generated at a slower rate than the FIFO fills
>up,
>> >> we will have fewer timestamps than samples. Currently, we fill in
>0 for
>> >> any unmatched timestamps. However, this is very confusing for
>userspace,
>> >> which does not expect discontinuities in timestamps and must
>somehow
>> >> work around the issue.
>> >>
>> >> Improve the situation by interpolating timestamps when we can't
>map them
>> >> 1:1 to data. We do this by assuming the timestamps we have are the
>most
>> >> recent and interpolating backwards to fill the older data. This
>makes
>> >> sense because inv_mpu6050_read_fifo gets called right after an
>interrupt
>> >> is generated, presumably when a datum was generated, so the most
>recent
>> >> timestamp matches up with the most recent datum. In addition, this
>> >> assumption is borne out by observation, which shows monotonically
>> >> increasing timestamps when interpolating this way but
>discontinuities
>> >> when interpolating in the other direction.
>> >>
>> >> Although this method is not perfectly accurate, it is probably the
>best
>> >> we can do if we don't get one interrupt per datum.
>> > This patch worries me somewhat - we are basically guessing what the
>cause
>> > of the missed interrupts was.  If for example the fifo read itself
>> > takes a long time, the interrupt time we have is actually valid for
>> > the first sample and we should in interpolating forwards in time.
>> >
>>
>> I agree with you that if the delay is somewhere after the IRQ fires
>but
>> prior to the FIFO read (or the FIFO read itself), then we'd need
>forward
>> interpolation. That said, if there is more than 1 sample in FIFO when
>> the IRQ fires, we need backwards interpolation (since those samples
>were
>> certainly created prior to the IRQ firing and the timestamp being
>> generated). I believe the current patch is will accurately handle the
>> backwards interpolation case but be wrong for the forwards
>interpolation
>> case (though it will at least guarantee samples to be monotonically
>> increasing, as they should be).
>>
>> I thought about this and I think we can fix both issues. I propose we
>do
>> the following:
>>
>> - When the IRQ fires, immediately timestamp as we already do. Also,
>> immediately check the FIFO length and store it (call this length n).
>>
>> - Right after reading from the FIFO length (calling
>regmap_bulk_read()
>> at inv_mpu_ring.c:146), take a timestamp again. Call this FIFO length
>m.
>>
>> - The first n samples (those present when the IRQ fired) should be
>> backwards-interpolated from the timestamp taken when the IRQ fired.
>>
>> - If the two FIFO lengths taken are different (m - n > 0), then some
>> samples were generated between the IRQ firing and the FIFO being
>read.
>> This could be caused by a slow read, a delay firing off the bottom
>half
>> of the IRQ, or some other delay in the inv_mpu6050_read_fifo()
>function.
>> In any case, these m - n samples need to be forward-interpolated
>between
>> the two timestamps we took, since they were generated sometime
>between
>> the two timestamps.
>>
>> I believe this approach will fix both issues. Although my board is
>not
>> seeing the second issue (where m - n > 0), I can simulate it by
>adding
>> artificial delays.
>>
>> In addition to fixing my issue, this should make the code more
>resilient
>> to unknown bus and IRQ issues in the future.
>>
>> Jonathan, how does this approach sound to you?
>Great.
>
>>
>> > It sounds from discussions like something else is broken on your
>> > board.  I like the idea of interpolating interrupts, but would like
>to
>> > conduct the same experiments you did when we are running at high
>speeds
>> > and seeing misses - not on the test case you currently have.
>> >
>> > The problem then will be estimating how long the interrupt took to
>> > be handed vs the read out rate of the fifo.  Chances are our
>'correct'
>> > timestamp is somewhat after the first reading but well before the
>last
>> > one.
>> >
>> > Anyhow, let us know once you have the thing running properly at
>> > high speed then we can work out how best to address this.
>> >
>> > Jonathan
>> >
>>
>> Once I get the thing working, I will definitely look again at the
>> interpolation and make sure it's still valid. If I see any anomalies,
>> I'll let you know.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Martin Kelly]

On 04/06/2018 08:41 AM, Jonathan Cameron wrote:
> 
> 
> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
>> Hello,
>>
>>
>> there is just a problem if I'm understanding well.
>>
>>
>> Reading FIFO count register under hard irq handler (when taking the
>> timestamp) is not possible since i2c access is using a mutex. That's
>> why we are using an irq thread for reading FIFO content.
> 
> Good point. Need more sleep or caffeine!
> 
> 

I was about to reply with the same, as I started coding it up :). Too 
bad, it was such a great plan!

I have a little update: When switching to level triggered interrupts, 
the problem goes away for me, as do the bus errors I get at high 
frequencies. I'm working on a patch to support other interrupt types 
than rising edge, which is almost done.

I also intend to look again at the bus errors for edge driven 
interrupts. Since they happen only at high frequency and go away with 
level driven interrupts (which must be acked and therefore prevent 
reentrancy), I suspect there's a concurrency bug.

That said, I think the question remains: Since we can't get the FIFO 
count from the hard IRQ handler, and since it could be some time before 
the bottom half thread is scheduled, I don't see any way to accurately 
handle forward interpolation.

Though we can't do forward interpolation, I think at least having 
backward interpolation is better than filling in blank timestamps, 
especially as we haven't seen an actual case of forward interpolation 
being needed, while we have real use cases that require backward 
interpolation (and can be easily verified in a logic analyzer).

However, that's only my opinion. Jonathan, Jean-Baptiste, and others, 
what do you think?

>>
>>
>> I've been able to reproduce the issue when doing a long stress-test at
>> high speed (200Hz). Perhaps this is related to the FIFO error handling
>> which looks really over-complex for me. I am currently investigating
>> this issue.
>>
>>
>> But I would be happy to test any new solution if proposed.
>>
>>
>> Best regards,
>>
>> JB
>>

Thanks Jean-Baptiste; let me know how that turns out.

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Martin Kelly]

On 04/06/2018 09:33 AM, Martin Kelly wrote:
> On 04/06/2018 08:41 AM, Jonathan Cameron wrote:
>>
>>
>> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol 
>> <JManeyrol@invensense.com> wrote:
>>> Hello,
>>>
>>>
>>> there is just a problem if I'm understanding well.
>>>
>>>
>>> Reading FIFO count register under hard irq handler (when taking the
>>> timestamp) is not possible since i2c access is using a mutex. That's
>>> why we are using an irq thread for reading FIFO content.
>>
>> Good point. Need more sleep or caffeine!
>>
>>
> 
> I was about to reply with the same, as I started coding it up :). Too 
> bad, it was such a great plan!
> 
> I have a little update: When switching to level triggered interrupts, 
> the problem goes away for me, as do the bus errors I get at high 
> frequencies. I'm working on a patch to support other interrupt types 
> than rising edge, which is almost done.
> 
> I also intend to look again at the bus errors for edge driven 
> interrupts. Since they happen only at high frequency and go away with 
> level driven interrupts (which must be acked and therefore prevent 
> reentrancy), I suspect there's a concurrency bug.
> 
> That said, I think the question remains: Since we can't get the FIFO 
> count from the hard IRQ handler, and since it could be some time before 
> the bottom half thread is scheduled, I don't see any way to accurately 
> handle forward interpolation.
> 
> Though we can't do forward interpolation, I think at least having 
> backward interpolation is better than filling in blank timestamps, 
> especially as we haven't seen an actual case of forward interpolation 
> being needed, while we have real use cases that require backward 
> interpolation (and can be easily verified in a logic analyzer).
> 
> However, that's only my opinion. Jonathan, Jean-Baptiste, and others, 
> what do you think?
> 

Hi,

What can I do to help get closure on this? Is there any data I could 
gather that would help make a decision?

In cases of a troubled system, I think the interpolation is very useful, 
and it's awkward to do in userspace, as it involves keeping a history of 
past records, which can be inconvenient and not always accurate (e.g. if 
userspace doesn't read fast enough and we miss records). However, I 
certainly understand the concern about interpolation. Should we consider 
making the interpolation configurable via sysfs or device-tree?

I'd be happy to hear other ideas too about better handling the case of 
missed interrupts.

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Jean-Baptiste Maneyrol]

On 25/04/2018 20:06, Martin Kelly wrote:
> On 04/06/2018 09:33 AM, Martin Kelly wrote:
>> On 04/06/2018 08:41 AM, Jonathan Cameron wrote:
>>>
>>>
>>> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol 
>>> <JManeyrol@invensense.com> wrote:
>>>> Hello,
>>>>
>>>>
>>>> there is just a problem if I'm understanding well.
>>>>
>>>>
>>>> Reading FIFO count register under hard irq handler (when taking the
>>>> timestamp) is not possible since i2c access is using a mutex. That's
>>>> why we are using an irq thread for reading FIFO content.
>>>
>>> Good point. Need more sleep or caffeine!
>>>
>>>
>>
>> I was about to reply with the same, as I started coding it up :). Too 
>> bad, it was such a great plan!
>>
>> I have a little update: When switching to level triggered interrupts, 
>> the problem goes away for me, as do the bus errors I get at high 
>> frequencies. I'm working on a patch to support other interrupt types 
>> than rising edge, which is almost done.
>>
>> I also intend to look again at the bus errors for edge driven 
>> interrupts. Since they happen only at high frequency and go away with 
>> level driven interrupts (which must be acked and therefore prevent 
>> reentrancy), I suspect there's a concurrency bug.
>>
>> That said, I think the question remains: Since we can't get the FIFO 
>> count from the hard IRQ handler, and since it could be some time 
>> before the bottom half thread is scheduled, I don't see any way to 
>> accurately handle forward interpolation.
>>
>> Though we can't do forward interpolation, I think at least having 
>> backward interpolation is better than filling in blank timestamps, 
>> especially as we haven't seen an actual case of forward interpolation 
>> being needed, while we have real use cases that require backward 
>> interpolation (and can be easily verified in a logic analyzer).
>>
>> However, that's only my opinion. Jonathan, Jean-Baptiste, and others, 
>> what do you think?
>>
> 
> Hi,
> 
> What can I do to help get closure on this? Is there any data I could 
> gather that would help make a decision?
> 
> In cases of a troubled system, I think the interpolation is very useful, 
> and it's awkward to do in userspace, as it involves keeping a history of 
> past records, which can be inconvenient and not always accurate (e.g. if 
> userspace doesn't read fast enough and we miss records). However, I 
> certainly understand the concern about interpolation. Should we consider 
> making the interpolation configurable via sysfs or device-tree?
> 
> I'd be happy to hear other ideas too about better handling the case of 
> missed interrupts.

Hello,

I have implemented a new timestamp mechanism that do something similar 
but in a more precise way.

The main ideas are:
* check if interrupt timestamp is valid (computes interrupt timestamps 
interval and check against set period value with a margin)
* use validated interrupt timestamps to measure chip internal period 
from the system (to have more accurate computed timestamp value)
* use the interrupt timestamp for data if it is valid
* if interrupt timestamp is invalid (meaning interrupt was delayed or 
missing), computes timestamp using the measured chip period

I will send the corresponding patch series as soon as my last clean-up 
series has been accepted by Jonathan.

Regards,
JB

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Martin Kelly]

On 04/26/2018 12:35 AM, Jean-Baptiste Maneyrol wrote:
> 
> 
> On 25/04/2018 20:06, Martin Kelly wrote:
>> On 04/06/2018 09:33 AM, Martin Kelly wrote:
>>> On 04/06/2018 08:41 AM, Jonathan Cameron wrote:
>>>>
>>>>
>>>> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol 
>>>> <JManeyrol@invensense.com> wrote:
>>>>> Hello,
>>>>>
>>>>>
>>>>> there is just a problem if I'm understanding well.
>>>>>
>>>>>
>>>>> Reading FIFO count register under hard irq handler (when taking the
>>>>> timestamp) is not possible since i2c access is using a mutex. That's
>>>>> why we are using an irq thread for reading FIFO content.
>>>>
>>>> Good point. Need more sleep or caffeine!
>>>>
>>>>
>>>
>>> I was about to reply with the same, as I started coding it up :). Too 
>>> bad, it was such a great plan!
>>>
>>> I have a little update: When switching to level triggered interrupts, 
>>> the problem goes away for me, as do the bus errors I get at high 
>>> frequencies. I'm working on a patch to support other interrupt types 
>>> than rising edge, which is almost done.
>>>
>>> I also intend to look again at the bus errors for edge driven 
>>> interrupts. Since they happen only at high frequency and go away with 
>>> level driven interrupts (which must be acked and therefore prevent 
>>> reentrancy), I suspect there's a concurrency bug.
>>>
>>> That said, I think the question remains: Since we can't get the FIFO 
>>> count from the hard IRQ handler, and since it could be some time 
>>> before the bottom half thread is scheduled, I don't see any way to 
>>> accurately handle forward interpolation.
>>>
>>> Though we can't do forward interpolation, I think at least having 
>>> backward interpolation is better than filling in blank timestamps, 
>>> especially as we haven't seen an actual case of forward interpolation 
>>> being needed, while we have real use cases that require backward 
>>> interpolation (and can be easily verified in a logic analyzer).
>>>
>>> However, that's only my opinion. Jonathan, Jean-Baptiste, and others, 
>>> what do you think?
>>>
>>
>> Hi,
>>
>> What can I do to help get closure on this? Is there any data I could 
>> gather that would help make a decision?
>>
>> In cases of a troubled system, I think the interpolation is very 
>> useful, and it's awkward to do in userspace, as it involves keeping a 
>> history of past records, which can be inconvenient and not always 
>> accurate (e.g. if userspace doesn't read fast enough and we miss 
>> records). However, I certainly understand the concern about 
>> interpolation. Should we consider making the interpolation 
>> configurable via sysfs or device-tree?
>>
>> I'd be happy to hear other ideas too about better handling the case of 
>> missed interrupts.
> 
> Hello,
> 
> I have implemented a new timestamp mechanism that do something similar 
> but in a more precise way.
> 
> The main ideas are:
> * check if interrupt timestamp is valid (computes interrupt timestamps 
> interval and check against set period value with a margin)
> * use validated interrupt timestamps to measure chip internal period 
> from the system (to have more accurate computed timestamp value)
> * use the interrupt timestamp for data if it is valid
> * if interrupt timestamp is invalid (meaning interrupt was delayed or 
> missing), computes timestamp using the measured chip period
> 
> I will send the corresponding patch series as soon as my last clean-up 
> series has been accepted by Jonathan.
> 
> Regards,
> JB

Excellent, I look forward to the patches. Jonathan, are you OK with this 
design approach?

Re: [PATCH v3] imu: inv_mpu6050: interpolate missing timestamps

[Jonathan Cameron]

On Thu, 26 Apr 2018 09:46:18 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> On 04/26/2018 12:35 AM, Jean-Baptiste Maneyrol wrote:
> > 
> > 
> > On 25/04/2018 20:06, Martin Kelly wrote:  
> >> On 04/06/2018 09:33 AM, Martin Kelly wrote:  
> >>> On 04/06/2018 08:41 AM, Jonathan Cameron wrote:  
> >>>>
> >>>>
> >>>> On 6 April 2018 16:21:05 BST, Jean-Baptiste Maneyrol 
> >>>> <JManeyrol@invensense.com> wrote:  
> >>>>> Hello,
> >>>>>
> >>>>>
> >>>>> there is just a problem if I'm understanding well.
> >>>>>
> >>>>>
> >>>>> Reading FIFO count register under hard irq handler (when taking the
> >>>>> timestamp) is not possible since i2c access is using a mutex. That's
> >>>>> why we are using an irq thread for reading FIFO content.  
> >>>>
> >>>> Good point. Need more sleep or caffeine!
> >>>>
> >>>>  
> >>>
> >>> I was about to reply with the same, as I started coding it up :). Too 
> >>> bad, it was such a great plan!
> >>>
> >>> I have a little update: When switching to level triggered interrupts, 
> >>> the problem goes away for me, as do the bus errors I get at high 
> >>> frequencies. I'm working on a patch to support other interrupt types 
> >>> than rising edge, which is almost done.
> >>>
> >>> I also intend to look again at the bus errors for edge driven 
> >>> interrupts. Since they happen only at high frequency and go away with 
> >>> level driven interrupts (which must be acked and therefore prevent 
> >>> reentrancy), I suspect there's a concurrency bug.
> >>>
> >>> That said, I think the question remains: Since we can't get the FIFO 
> >>> count from the hard IRQ handler, and since it could be some time 
> >>> before the bottom half thread is scheduled, I don't see any way to 
> >>> accurately handle forward interpolation.
> >>>
> >>> Though we can't do forward interpolation, I think at least having 
> >>> backward interpolation is better than filling in blank timestamps, 
> >>> especially as we haven't seen an actual case of forward interpolation 
> >>> being needed, while we have real use cases that require backward 
> >>> interpolation (and can be easily verified in a logic analyzer).
> >>>
> >>> However, that's only my opinion. Jonathan, Jean-Baptiste, and others, 
> >>> what do you think?
> >>>  
> >>
> >> Hi,
> >>
> >> What can I do to help get closure on this? Is there any data I could 
> >> gather that would help make a decision?
> >>
> >> In cases of a troubled system, I think the interpolation is very 
> >> useful, and it's awkward to do in userspace, as it involves keeping a 
> >> history of past records, which can be inconvenient and not always 
> >> accurate (e.g. if userspace doesn't read fast enough and we miss 
> >> records). However, I certainly understand the concern about 
> >> interpolation. Should we consider making the interpolation 
> >> configurable via sysfs or device-tree?
> >>
> >> I'd be happy to hear other ideas too about better handling the case of 
> >> missed interrupts.  
> > 
> > Hello,
> > 
> > I have implemented a new timestamp mechanism that do something similar 
> > but in a more precise way.
> > 
> > The main ideas are:
> > * check if interrupt timestamp is valid (computes interrupt timestamps 
> > interval and check against set period value with a margin)
> > * use validated interrupt timestamps to measure chip internal period 
> > from the system (to have more accurate computed timestamp value)
> > * use the interrupt timestamp for data if it is valid
> > * if interrupt timestamp is invalid (meaning interrupt was delayed or 
> > missing), computes timestamp using the measured chip period
> > 
> > I will send the corresponding patch series as soon as my last clean-up 
> > series has been accepted by Jonathan.
> > 
> > Regards,
> > JB  
> 
> Excellent, I look forward to the patches. Jonathan, are you OK with this 
> design approach?
It does sound a rather complex solution, but should be accurate.

We'll see when the patches are out, but it will probably do the job given
I think we have concluded we want to hide this from userspace as much
as possible.

Thanks,

Jonathan