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

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

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