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
