From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Jean-Baptiste Maneyrol To: Jonathan Cameron , Martin Kelly CC: "linux-iio@vger.kernel.org" Subject: Re: How to handle missing timestamps? (was Re: [PATCH] iio: imu: inv_mpu6050: improve missing timestamp handling) Date: Mon, 26 Mar 2018 14:17:47 +0000 Message-ID: References: <20180324000240.19519-1-mkelly@xevo.com>,<20180324123519.0acba88e@archlinux> In-Reply-To: <20180324123519.0acba88e@archlinux> Content-Type: multipart/alternative; boundary="_000_CY4PR1201MB0184E4503B2B1EEA7F24C41DC4AD0CY4PR1201MB0184_" MIME-Version: 1.0 List-ID: --_000_CY4PR1201MB0184E4503B2B1EEA7F24C41DC4AD0CY4PR1201MB0184_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hello, we should have 1 interrupt every time a sensor data has been acquired. So i= n theory this is not something that should happen. But real world is always= a different story. Do you have a case where you saw that happen? The best way if this happens would be to create the timestamp based on the = sampling rate since we know it (last timestamp + sampling interval). That w= ould be very similar to the real value since the only difference is the clo= ck drift between the chip and the system. Best regards, Jean-Baptiste Maneyrol ________________________________ From: linux-iio-owner@vger.kernel.org on = behalf of Jonathan Cameron Sent: Saturday, March 24, 2018 1:35:19 PM To: Martin Kelly Cc: linux-iio@vger.kernel.org; Jean-Baptiste Maneyrol Subject: How to handle missing timestamps? (was Re: [PATCH] iio: imu: inv_m= pu6050: improve missing timestamp handling) On Fri, 23 Mar 2018 17:02:40 -0700 Martin Kelly wrote: > When interrupts are generated at a slower rate than the FIFO queue 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 using the most recent timestamp when a > timestamp is missing. Although this guess is not perfectly accurate, it > is still close to the correct timestamp and won't result in the > confusion caused by using 0. > > Signed-off-by: Martin Kelly Hmm. I would like to see where other peoples opinions on this lie. The decision to mark it as 0 was deliberately made. There are a number of applications where you have to 'know' the timestamps are incorrect - pretending simply doesn't work. Arguably it is fine for a system to detect that it is seeing repeated values and hence 'fix them up. This is a change in ABI however which is going to be unfortunate if we have code out there which is doing the right thing - interpolating timestamps only once we have another known point to build from. So opinions anyone? Jonathan > --- > drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu= /inv_mpu6050/inv_mpu_ring.c > index ff81c6aa009d..a982037d5dad 100644 > --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c > @@ -126,6 +126,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > int result; > u8 data[INV_MPU6050_OUTPUT_DATA_SIZE]; > u16 fifo_count; > + s64 last_timestamp; > s64 timestamp; > > mutex_lock(&st->lock); > @@ -159,6 +160,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > if (kfifo_len(&st->timestamps) > > fifo_count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR) > goto flush_fifo; > + last_timestamp =3D 0; > while (fifo_count >=3D bytes_per_datum) { > result =3D regmap_bulk_read(st->map, st->reg->fifo_r_w, > data, bytes_per_datum); > @@ -166,9 +168,11 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) > goto flush_fifo; > > result =3D kfifo_out(&st->timestamps, ×tamp, 1); > - /* when there is no timestamp, put timestamp as 0 */ > + /* when there is no timestamp, just use the last one we saw= */ > if (result =3D=3D 0) > - timestamp =3D 0; > + timestamp =3D last_timestamp; > + else > + last_timestamp =3D timestamp; > > result =3D iio_push_to_buffers_with_timestamp(indio_dev, d= ata, > timestamp); -- 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 --_000_CY4PR1201MB0184E4503B2B1EEA7F24C41DC4AD0CY4PR1201MB0184_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hello,


we should have 1 interrupt every = time a sensor data has been acquired. So in theory this is not something th= at should happen. But real world is always a different story.


Do you have a case where you saw = that happen?


The best way if this happens woul= d be to create the timestamp based on the sampling rate since we know it (l= ast timestamp + sampling interval). That would be very similar to the r= eal value since the only difference is the clock drift between the chip and the system.


Best regards,

Jean-Baptiste Maneyrol

From: linux-iio-owner@vger.= kernel.org <linux-iio-owner@vger.kernel.org> on behalf of Jonathan Ca= meron <jic23@kernel.org>
Sent: Saturday, March 24, 2018 1:35:19 PM
To: Martin Kelly
Cc: linux-iio@vger.kernel.org; Jean-Baptiste Maneyrol
Subject: How to handle missing timestamps? (was Re: [PATCH] iio: imu= : inv_mpu6050: improve missing timestamp handling)
 
On Fri, 23 Mar 2018 17:02:40 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> When interrupts are generated at a slower rate than the FIFO queue fil= ls
> 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 using the most recent timestamp when a
> timestamp is missing. Although this guess is not perfectly accurate, i= t
> is still close to the correct timestamp and won't result in the
> confusion caused by using 0.
>
> Signed-off-by: Martin Kelly <mkelly@xevo.com>
Hmm. I would like to see where other peoples opinions on this lie.
The decision to mark it as 0 was deliberately made.  There are a numbe= r
of applications where you have to 'know' the timestamps are incorrect
- pretending simply doesn't work.  Arguably it is fine for a system to detect that it is seeing repeated values and hence 'fix them up.
This is a change in ABI however which is going to be unfortunate
if we have code out there which is doing the right thing - interpolating timestamps only once we have another known point to build from.

So opinions anyone?

Jonathan
> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 8 +++&#= 43;++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/= imu/inv_mpu6050/inv_mpu_ring.c
> index ff81c6aa009d..a982037d5dad 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -126,6 +126,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, voi= d *p)
>        int result;
>        u8 data[INV_MPU6050_OUTPUT_D= ATA_SIZE];
>        u16 fifo_count;
> +     s64 last_timestamp;
>        s64 timestamp;
> 
>        mutex_lock(&st->lock)= ;
> @@ -159,6 +160,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, voi= d *p)
>        if (kfifo_len(&st->ti= mestamps) >
>            fifo= _count / bytes_per_datum + INV_MPU6050_TIME_STAMP_TOR)
>            = ;    goto flush_fifo;
> +     last_timestamp =3D 0;
>        while (fifo_count >=3D by= tes_per_datum) {
>            = ;    result =3D regmap_bulk_read(st->map, st->reg->= fifo_r_w,
>            = ;            &n= bsp;            = ;     data, bytes_per_datum);
> @@ -166,9 +168,11 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, vo= id *p)
>            = ;            goto fl= ush_fifo;
> 
>            = ;    result =3D kfifo_out(&st->timestamps, &times= tamp, 1);
> -           &nb= sp; /* when there is no timestamp, put timestamp as 0 */
> +           = ;  /* when there is no timestamp, just use the last one we saw */
>            = ;    if (result =3D=3D 0)
> -           &nb= sp;         timestamp =3D 0;
> +           = ;          timestamp =3D last_= timestamp;
> +           = ;  else
> +           = ;          last_timestamp =3D = timestamp;
> 
>            = ;    result =3D iio_push_to_buffers_with_timestamp(indio_dev= , data,
>            = ;            &n= bsp;            = ;            &n= bsp;          timestamp);

--
To unsubscribe from this list: send the line "unsubscribe linux-iio&qu= ot; in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--_000_CY4PR1201MB0184E4503B2B1EEA7F24C41DC4AD0CY4PR1201MB0184_--