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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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/spi access is using a mutex. That's why we are 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 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@xxxxxxxxxx>
Sent: Friday, April 6, 2018 5:15:09 PM
To: Martin Kelly
Cc: Jonathan Cameron; linux-iio@xxxxxxxxxxxxxxx; 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@xxxxxxxx> wrote:

> On 03/30/2018 03:36 AM, Jonathan Cameron wrote:
> > On Wed, 28 Mar 2018 10:40:29 -0700
> > Martin Kelly <mkelly@xxxxxxxx> 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@xxxxxxxxxxxxxxx
> More majordomo info at  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@xxxxxxxxxxxxxxx> . To subscribe a list (``linux-kernel'' is given as an example), use following as the only content of your letter:

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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux