Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Tue, 12 Dec 2017 20:21:14 + Jonathan Cameronwrote: > On Sun, 10 Dec 2017 21:47:37 +0100 > Stefan Brüns wrote: > > > On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote: > > > On Fri, 8 Dec 2017 18:41:50 +0100 > > > > > > Stefan Brüns wrote: > > > > The iio timestamp clock is user selectable and may be non-monotonic. > > > > Also, > > > > only part of the acquisition time is measured, thus the delay was longer > > > > than intended. > > > > > > > > Signed-off-by: Stefan Brüns > > > > --- > > > > > > > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > > > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > > > > index 2621a34ee5c6..65bd9e69faf2 100644 > > > > --- a/drivers/iio/adc/ina2xx-adc.c > > > > +++ b/drivers/iio/adc/ina2xx-adc.c > > > > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev > > > > *indio_dev)> > > > > /* data buffer needs space for channel data and timestap */ > > > > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > > > > int bit, ret, i = 0; > > > > > > > > - s64 time_a, time_b; > > > > + s64 time; > > > > > > > > unsigned int alert; > > > > > > > > - time_a = iio_get_time_ns(indio_dev); > > > > + time = iio_get_time_ns(indio_dev); > > > > > > > > /* > > > > > > > > * Because the timer thread and the chip conversion clock > > > > > > > > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev > > > > *indio_dev)> > > > > data[i++] = val; > > > > > > > > } > > > > > > > > - time_b = iio_get_time_ns(indio_dev); > > > > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > > > > > > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > > > > - > > > > - return (unsigned long)(time_b - time_a) / 1000; > > > > + return 0; > > > > > > > > }; > > > > > > > > static int ina2xx_capture_thread(void *data) > > > > > > > > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > > > > > > > > struct iio_dev *indio_dev = data; > > > > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > > > > int sampling_us = SAMPLING_PERIOD(chip); > > > > > > > > - int buffer_us, delay_us; > > > > + int ret; > > > > + struct timespec64 next, now, delta; > > > > + s64 delay_us; > > > > > > > > /* > > > > > > > > * Poll a bit faster than the chip internal Fs, in case > > > > > > > > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > > > > > > > > if (!chip->allow_async_readout) > > > > > > > > sampling_us -= 200; > > > > > > > > + ktime_get_ts64(); > > > > + > > > > > > > > do { > > > > > > > > - buffer_us = ina2xx_work_buffer(indio_dev); > > > > - if (buffer_us < 0) > > > > - return buffer_us; > > > > + ret = ina2xx_work_buffer(indio_dev); > > > > + if (ret < 0) > > > > + return ret; > > > > > > > > - if (sampling_us > buffer_us) { > > > > - delay_us = sampling_us - buffer_us; > > > > - usleep_range(delay_us, (delay_us * 3) >> 1); > > > > - } > > > > + ktime_get_ts64(); > > > > + > > > > + do { > > > > + timespec64_add_ns(, 1000 * sampling_us); > > > > + delta = timespec64_sub(next, now); > > > > + delay_us = timespec64_to_ns() / 1000; > > > > + } while (delay_us <= 0); > > > > > > Umm. I'm lost, what is the purpose of the above dance? > > > A comment perhaps. > > > > next is the timestamp for the next read to happen, now is the current time. > > Obviously we have to sleep for the remainder. > > > > Each sampling interval the "next" timestamp is pushed back by sampling_us. > > Normally this happens exactly once per read, i.e. we schedule the reads to > > happen exactly each sampling interval. > > > > The sampling inteval is *only* added multiple times if it is faster than > > the > > bus can deliver the data (at 100 kBits/s, each register read takes about > > 400 > > us, so sampling faster than every ~1 ms is not possible. > > So this is deliberately skipping a sample if this happens? It was this > element that I wasn't understanding previously. > Add a comment in the code to explain this and I'm happy. It's horrible, > but not much we can do if things are simply going too fast. I still want to see a comment in the code making it clear what is happening in that loop. So
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Tue, 12 Dec 2017 20:21:14 + Jonathan Cameron wrote: > On Sun, 10 Dec 2017 21:47:37 +0100 > Stefan Brüns wrote: > > > On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote: > > > On Fri, 8 Dec 2017 18:41:50 +0100 > > > > > > Stefan Brüns wrote: > > > > The iio timestamp clock is user selectable and may be non-monotonic. > > > > Also, > > > > only part of the acquisition time is measured, thus the delay was longer > > > > than intended. > > > > > > > > Signed-off-by: Stefan Brüns > > > > --- > > > > > > > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > > > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > > > > index 2621a34ee5c6..65bd9e69faf2 100644 > > > > --- a/drivers/iio/adc/ina2xx-adc.c > > > > +++ b/drivers/iio/adc/ina2xx-adc.c > > > > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev > > > > *indio_dev)> > > > > /* data buffer needs space for channel data and timestap */ > > > > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > > > > int bit, ret, i = 0; > > > > > > > > - s64 time_a, time_b; > > > > + s64 time; > > > > > > > > unsigned int alert; > > > > > > > > - time_a = iio_get_time_ns(indio_dev); > > > > + time = iio_get_time_ns(indio_dev); > > > > > > > > /* > > > > > > > > * Because the timer thread and the chip conversion clock > > > > > > > > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev > > > > *indio_dev)> > > > > data[i++] = val; > > > > > > > > } > > > > > > > > - time_b = iio_get_time_ns(indio_dev); > > > > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > > > > > > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > > > > - > > > > - return (unsigned long)(time_b - time_a) / 1000; > > > > + return 0; > > > > > > > > }; > > > > > > > > static int ina2xx_capture_thread(void *data) > > > > > > > > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > > > > > > > > struct iio_dev *indio_dev = data; > > > > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > > > > int sampling_us = SAMPLING_PERIOD(chip); > > > > > > > > - int buffer_us, delay_us; > > > > + int ret; > > > > + struct timespec64 next, now, delta; > > > > + s64 delay_us; > > > > > > > > /* > > > > > > > > * Poll a bit faster than the chip internal Fs, in case > > > > > > > > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > > > > > > > > if (!chip->allow_async_readout) > > > > > > > > sampling_us -= 200; > > > > > > > > + ktime_get_ts64(); > > > > + > > > > > > > > do { > > > > > > > > - buffer_us = ina2xx_work_buffer(indio_dev); > > > > - if (buffer_us < 0) > > > > - return buffer_us; > > > > + ret = ina2xx_work_buffer(indio_dev); > > > > + if (ret < 0) > > > > + return ret; > > > > > > > > - if (sampling_us > buffer_us) { > > > > - delay_us = sampling_us - buffer_us; > > > > - usleep_range(delay_us, (delay_us * 3) >> 1); > > > > - } > > > > + ktime_get_ts64(); > > > > + > > > > + do { > > > > + timespec64_add_ns(, 1000 * sampling_us); > > > > + delta = timespec64_sub(next, now); > > > > + delay_us = timespec64_to_ns() / 1000; > > > > + } while (delay_us <= 0); > > > > > > Umm. I'm lost, what is the purpose of the above dance? > > > A comment perhaps. > > > > next is the timestamp for the next read to happen, now is the current time. > > Obviously we have to sleep for the remainder. > > > > Each sampling interval the "next" timestamp is pushed back by sampling_us. > > Normally this happens exactly once per read, i.e. we schedule the reads to > > happen exactly each sampling interval. > > > > The sampling inteval is *only* added multiple times if it is faster than > > the > > bus can deliver the data (at 100 kBits/s, each register read takes about > > 400 > > us, so sampling faster than every ~1 ms is not possible. > > So this is deliberately skipping a sample if this happens? It was this > element that I wasn't understanding previously. > Add a comment in the code to explain this and I'm happy. It's horrible, > but not much we can do if things are simply going too fast. I still want to see a comment in the code making it clear what is happening in that loop. So for now I'm going to stop here in applying this series. Jonathan > > Thanks for the info. > > Jonathan >
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Sun, 10 Dec 2017 21:47:37 +0100 Stefan Brünswrote: > On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote: > > On Fri, 8 Dec 2017 18:41:50 +0100 > > > > Stefan Brüns wrote: > > > The iio timestamp clock is user selectable and may be non-monotonic. Also, > > > only part of the acquisition time is measured, thus the delay was longer > > > than intended. > > > > > > Signed-off-by: Stefan Brüns > > > --- > > > > > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > > > index 2621a34ee5c6..65bd9e69faf2 100644 > > > --- a/drivers/iio/adc/ina2xx-adc.c > > > +++ b/drivers/iio/adc/ina2xx-adc.c > > > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev > > > *indio_dev)> > > > /* data buffer needs space for channel data and timestap */ > > > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > > > int bit, ret, i = 0; > > > > > > - s64 time_a, time_b; > > > + s64 time; > > > > > > unsigned int alert; > > > > > > - time_a = iio_get_time_ns(indio_dev); > > > + time = iio_get_time_ns(indio_dev); > > > > > > /* > > > > > >* Because the timer thread and the chip conversion clock > > > > > > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev > > > *indio_dev)> > > > data[i++] = val; > > > > > > } > > > > > > - time_b = iio_get_time_ns(indio_dev); > > > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > > > > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > > > - > > > - return (unsigned long)(time_b - time_a) / 1000; > > > + return 0; > > > > > > }; > > > > > > static int ina2xx_capture_thread(void *data) > > > > > > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > > > > > > struct iio_dev *indio_dev = data; > > > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > > > int sampling_us = SAMPLING_PERIOD(chip); > > > > > > - int buffer_us, delay_us; > > > + int ret; > > > + struct timespec64 next, now, delta; > > > + s64 delay_us; > > > > > > /* > > > > > >* Poll a bit faster than the chip internal Fs, in case > > > > > > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > > > > > > if (!chip->allow_async_readout) > > > > > > sampling_us -= 200; > > > > > > + ktime_get_ts64(); > > > + > > > > > > do { > > > > > > - buffer_us = ina2xx_work_buffer(indio_dev); > > > - if (buffer_us < 0) > > > - return buffer_us; > > > + ret = ina2xx_work_buffer(indio_dev); > > > + if (ret < 0) > > > + return ret; > > > > > > - if (sampling_us > buffer_us) { > > > - delay_us = sampling_us - buffer_us; > > > - usleep_range(delay_us, (delay_us * 3) >> 1); > > > - } > > > + ktime_get_ts64(); > > > + > > > + do { > > > + timespec64_add_ns(, 1000 * sampling_us); > > > + delta = timespec64_sub(next, now); > > > + delay_us = timespec64_to_ns() / 1000; > > > + } while (delay_us <= 0); > > > > Umm. I'm lost, what is the purpose of the above dance? > > A comment perhaps. > > next is the timestamp for the next read to happen, now is the current time. > Obviously we have to sleep for the remainder. > > Each sampling interval the "next" timestamp is pushed back by sampling_us. > Normally this happens exactly once per read, i.e. we schedule the reads to > happen exactly each sampling interval. > > The sampling inteval is *only* added multiple times if it is faster than the > bus can deliver the data (at 100 kBits/s, each register read takes about 400 > us, so sampling faster than every ~1 ms is not possible. So this is deliberately skipping a sample if this happens? It was this element that I wasn't understanding previously. Add a comment in the code to explain this and I'm happy. It's horrible, but not much we can do if things are simply going too fast. Thanks for the info. Jonathan > > The old code measured the time spent for reading the registers and slept for > the remainder of the interval. This way the sampling drifts, as there is some > time not accounted for - usleep_range, function call overhead, kthread > interrupted. > > Using a timestamp avoids the drift. It also allows simple readjustment of the > "next" sampling time when polling the status register. > > Kind regards, > > Stefan >
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Sun, 10 Dec 2017 21:47:37 +0100 Stefan Brüns wrote: > On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote: > > On Fri, 8 Dec 2017 18:41:50 +0100 > > > > Stefan Brüns wrote: > > > The iio timestamp clock is user selectable and may be non-monotonic. Also, > > > only part of the acquisition time is measured, thus the delay was longer > > > than intended. > > > > > > Signed-off-by: Stefan Brüns > > > --- > > > > > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > > > index 2621a34ee5c6..65bd9e69faf2 100644 > > > --- a/drivers/iio/adc/ina2xx-adc.c > > > +++ b/drivers/iio/adc/ina2xx-adc.c > > > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev > > > *indio_dev)> > > > /* data buffer needs space for channel data and timestap */ > > > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > > > int bit, ret, i = 0; > > > > > > - s64 time_a, time_b; > > > + s64 time; > > > > > > unsigned int alert; > > > > > > - time_a = iio_get_time_ns(indio_dev); > > > + time = iio_get_time_ns(indio_dev); > > > > > > /* > > > > > >* Because the timer thread and the chip conversion clock > > > > > > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev > > > *indio_dev)> > > > data[i++] = val; > > > > > > } > > > > > > - time_b = iio_get_time_ns(indio_dev); > > > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > > > > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > > > - > > > - return (unsigned long)(time_b - time_a) / 1000; > > > + return 0; > > > > > > }; > > > > > > static int ina2xx_capture_thread(void *data) > > > > > > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > > > > > > struct iio_dev *indio_dev = data; > > > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > > > int sampling_us = SAMPLING_PERIOD(chip); > > > > > > - int buffer_us, delay_us; > > > + int ret; > > > + struct timespec64 next, now, delta; > > > + s64 delay_us; > > > > > > /* > > > > > >* Poll a bit faster than the chip internal Fs, in case > > > > > > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > > > > > > if (!chip->allow_async_readout) > > > > > > sampling_us -= 200; > > > > > > + ktime_get_ts64(); > > > + > > > > > > do { > > > > > > - buffer_us = ina2xx_work_buffer(indio_dev); > > > - if (buffer_us < 0) > > > - return buffer_us; > > > + ret = ina2xx_work_buffer(indio_dev); > > > + if (ret < 0) > > > + return ret; > > > > > > - if (sampling_us > buffer_us) { > > > - delay_us = sampling_us - buffer_us; > > > - usleep_range(delay_us, (delay_us * 3) >> 1); > > > - } > > > + ktime_get_ts64(); > > > + > > > + do { > > > + timespec64_add_ns(, 1000 * sampling_us); > > > + delta = timespec64_sub(next, now); > > > + delay_us = timespec64_to_ns() / 1000; > > > + } while (delay_us <= 0); > > > > Umm. I'm lost, what is the purpose of the above dance? > > A comment perhaps. > > next is the timestamp for the next read to happen, now is the current time. > Obviously we have to sleep for the remainder. > > Each sampling interval the "next" timestamp is pushed back by sampling_us. > Normally this happens exactly once per read, i.e. we schedule the reads to > happen exactly each sampling interval. > > The sampling inteval is *only* added multiple times if it is faster than the > bus can deliver the data (at 100 kBits/s, each register read takes about 400 > us, so sampling faster than every ~1 ms is not possible. So this is deliberately skipping a sample if this happens? It was this element that I wasn't understanding previously. Add a comment in the code to explain this and I'm happy. It's horrible, but not much we can do if things are simply going too fast. Thanks for the info. Jonathan > > The old code measured the time spent for reading the registers and slept for > the remainder of the interval. This way the sampling drifts, as there is some > time not accounted for - usleep_range, function call overhead, kthread > interrupted. > > Using a timestamp avoids the drift. It also allows simple readjustment of the > "next" sampling time when polling the status register. > > Kind regards, > > Stefan >
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote: > On Fri, 8 Dec 2017 18:41:50 +0100 > > Stefan Brünswrote: > > The iio timestamp clock is user selectable and may be non-monotonic. Also, > > only part of the acquisition time is measured, thus the delay was longer > > than intended. > > > > Signed-off-by: Stefan Brüns > > --- > > > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > > index 2621a34ee5c6..65bd9e69faf2 100644 > > --- a/drivers/iio/adc/ina2xx-adc.c > > +++ b/drivers/iio/adc/ina2xx-adc.c > > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev > > *indio_dev)> > > /* data buffer needs space for channel data and timestap */ > > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > > int bit, ret, i = 0; > > > > - s64 time_a, time_b; > > + s64 time; > > > > unsigned int alert; > > > > - time_a = iio_get_time_ns(indio_dev); > > + time = iio_get_time_ns(indio_dev); > > > > /* > > > > * Because the timer thread and the chip conversion clock > > > > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev > > *indio_dev)> > > data[i++] = val; > > > > } > > > > - time_b = iio_get_time_ns(indio_dev); > > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > > - > > - return (unsigned long)(time_b - time_a) / 1000; > > + return 0; > > > > }; > > > > static int ina2xx_capture_thread(void *data) > > > > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > > > > struct iio_dev *indio_dev = data; > > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > > int sampling_us = SAMPLING_PERIOD(chip); > > > > - int buffer_us, delay_us; > > + int ret; > > + struct timespec64 next, now, delta; > > + s64 delay_us; > > > > /* > > > > * Poll a bit faster than the chip internal Fs, in case > > > > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > > > > if (!chip->allow_async_readout) > > > > sampling_us -= 200; > > > > + ktime_get_ts64(); > > + > > > > do { > > > > - buffer_us = ina2xx_work_buffer(indio_dev); > > - if (buffer_us < 0) > > - return buffer_us; > > + ret = ina2xx_work_buffer(indio_dev); > > + if (ret < 0) > > + return ret; > > > > - if (sampling_us > buffer_us) { > > - delay_us = sampling_us - buffer_us; > > - usleep_range(delay_us, (delay_us * 3) >> 1); > > - } > > + ktime_get_ts64(); > > + > > + do { > > + timespec64_add_ns(, 1000 * sampling_us); > > + delta = timespec64_sub(next, now); > > + delay_us = timespec64_to_ns() / 1000; > > + } while (delay_us <= 0); > > Umm. I'm lost, what is the purpose of the above dance? > A comment perhaps. next is the timestamp for the next read to happen, now is the current time. Obviously we have to sleep for the remainder. Each sampling interval the "next" timestamp is pushed back by sampling_us. Normally this happens exactly once per read, i.e. we schedule the reads to happen exactly each sampling interval. The sampling inteval is *only* added multiple times if it is faster than the bus can deliver the data (at 100 kBits/s, each register read takes about 400 us, so sampling faster than every ~1 ms is not possible. The old code measured the time spent for reading the registers and slept for the remainder of the interval. This way the sampling drifts, as there is some time not accounted for - usleep_range, function call overhead, kthread interrupted. Using a timestamp avoids the drift. It also allows simple readjustment of the "next" sampling time when polling the status register. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Sunday, December 10, 2017 6:31:57 PM CET Jonathan Cameron wrote: > On Fri, 8 Dec 2017 18:41:50 +0100 > > Stefan Brüns wrote: > > The iio timestamp clock is user selectable and may be non-monotonic. Also, > > only part of the acquisition time is measured, thus the delay was longer > > than intended. > > > > Signed-off-by: Stefan Brüns > > --- > > > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > > 1 file changed, 21 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > > index 2621a34ee5c6..65bd9e69faf2 100644 > > --- a/drivers/iio/adc/ina2xx-adc.c > > +++ b/drivers/iio/adc/ina2xx-adc.c > > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev > > *indio_dev)> > > /* data buffer needs space for channel data and timestap */ > > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > > int bit, ret, i = 0; > > > > - s64 time_a, time_b; > > + s64 time; > > > > unsigned int alert; > > > > - time_a = iio_get_time_ns(indio_dev); > > + time = iio_get_time_ns(indio_dev); > > > > /* > > > > * Because the timer thread and the chip conversion clock > > > > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev > > *indio_dev)> > > data[i++] = val; > > > > } > > > > - time_b = iio_get_time_ns(indio_dev); > > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > > - > > - return (unsigned long)(time_b - time_a) / 1000; > > + return 0; > > > > }; > > > > static int ina2xx_capture_thread(void *data) > > > > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > > > > struct iio_dev *indio_dev = data; > > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > > int sampling_us = SAMPLING_PERIOD(chip); > > > > - int buffer_us, delay_us; > > + int ret; > > + struct timespec64 next, now, delta; > > + s64 delay_us; > > > > /* > > > > * Poll a bit faster than the chip internal Fs, in case > > > > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > > > > if (!chip->allow_async_readout) > > > > sampling_us -= 200; > > > > + ktime_get_ts64(); > > + > > > > do { > > > > - buffer_us = ina2xx_work_buffer(indio_dev); > > - if (buffer_us < 0) > > - return buffer_us; > > + ret = ina2xx_work_buffer(indio_dev); > > + if (ret < 0) > > + return ret; > > > > - if (sampling_us > buffer_us) { > > - delay_us = sampling_us - buffer_us; > > - usleep_range(delay_us, (delay_us * 3) >> 1); > > - } > > + ktime_get_ts64(); > > + > > + do { > > + timespec64_add_ns(, 1000 * sampling_us); > > + delta = timespec64_sub(next, now); > > + delay_us = timespec64_to_ns() / 1000; > > + } while (delay_us <= 0); > > Umm. I'm lost, what is the purpose of the above dance? > A comment perhaps. next is the timestamp for the next read to happen, now is the current time. Obviously we have to sleep for the remainder. Each sampling interval the "next" timestamp is pushed back by sampling_us. Normally this happens exactly once per read, i.e. we schedule the reads to happen exactly each sampling interval. The sampling inteval is *only* added multiple times if it is faster than the bus can deliver the data (at 100 kBits/s, each register read takes about 400 us, so sampling faster than every ~1 ms is not possible. The old code measured the time spent for reading the registers and slept for the remainder of the interval. This way the sampling drifts, as there is some time not accounted for - usleep_range, function call overhead, kthread interrupted. Using a timestamp avoids the drift. It also allows simple readjustment of the "next" sampling time when polling the status register. Kind regards, Stefan -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Fri, 8 Dec 2017 18:41:50 +0100 Stefan Brünswrote: > The iio timestamp clock is user selectable and may be non-monotonic. Also, > only part of the acquisition time is measured, thus the delay was longer > than intended. > > Signed-off-by: Stefan Brüns > --- > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > index 2621a34ee5c6..65bd9e69faf2 100644 > --- a/drivers/iio/adc/ina2xx-adc.c > +++ b/drivers/iio/adc/ina2xx-adc.c > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) > /* data buffer needs space for channel data and timestap */ > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > int bit, ret, i = 0; > - s64 time_a, time_b; > + s64 time; > unsigned int alert; > > - time_a = iio_get_time_ns(indio_dev); > + time = iio_get_time_ns(indio_dev); > > /* >* Because the timer thread and the chip conversion clock > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) > data[i++] = val; > } > > - time_b = iio_get_time_ns(indio_dev); > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > - > - return (unsigned long)(time_b - time_a) / 1000; > + return 0; > }; > > static int ina2xx_capture_thread(void *data) > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > struct iio_dev *indio_dev = data; > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > int sampling_us = SAMPLING_PERIOD(chip); > - int buffer_us, delay_us; > + int ret; > + struct timespec64 next, now, delta; > + s64 delay_us; > > /* >* Poll a bit faster than the chip internal Fs, in case > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > if (!chip->allow_async_readout) > sampling_us -= 200; > > + ktime_get_ts64(); > + > do { > - buffer_us = ina2xx_work_buffer(indio_dev); > - if (buffer_us < 0) > - return buffer_us; > + ret = ina2xx_work_buffer(indio_dev); > + if (ret < 0) > + return ret; > > - if (sampling_us > buffer_us) { > - delay_us = sampling_us - buffer_us; > - usleep_range(delay_us, (delay_us * 3) >> 1); > - } > + ktime_get_ts64(); > + > + do { > + timespec64_add_ns(, 1000 * sampling_us); > + delta = timespec64_sub(next, now); > + delay_us = timespec64_to_ns() / 1000; > + } while (delay_us <= 0); Umm. I'm lost, what is the purpose of the above dance? A comment perhaps. > + > + usleep_range(delay_us, (delay_us * 3) >> 1); > > } while (!kthread_should_stop()); >
Re: [PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
On Fri, 8 Dec 2017 18:41:50 +0100 Stefan Brüns wrote: > The iio timestamp clock is user selectable and may be non-monotonic. Also, > only part of the acquisition time is measured, thus the delay was longer > than intended. > > Signed-off-by: Stefan Brüns > --- > > drivers/iio/adc/ina2xx-adc.c | 35 +-- > 1 file changed, 21 insertions(+), 14 deletions(-) > > diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c > index 2621a34ee5c6..65bd9e69faf2 100644 > --- a/drivers/iio/adc/ina2xx-adc.c > +++ b/drivers/iio/adc/ina2xx-adc.c > @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) > /* data buffer needs space for channel data and timestap */ > unsigned short data[4 + sizeof(s64)/sizeof(short)]; > int bit, ret, i = 0; > - s64 time_a, time_b; > + s64 time; > unsigned int alert; > > - time_a = iio_get_time_ns(indio_dev); > + time = iio_get_time_ns(indio_dev); > > /* >* Because the timer thread and the chip conversion clock > @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) > data[i++] = val; > } > > - time_b = iio_get_time_ns(indio_dev); > + iio_push_to_buffers_with_timestamp(indio_dev, data, time); > > - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); > - > - return (unsigned long)(time_b - time_a) / 1000; > + return 0; > }; > > static int ina2xx_capture_thread(void *data) > @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) > struct iio_dev *indio_dev = data; > struct ina2xx_chip_info *chip = iio_priv(indio_dev); > int sampling_us = SAMPLING_PERIOD(chip); > - int buffer_us, delay_us; > + int ret; > + struct timespec64 next, now, delta; > + s64 delay_us; > > /* >* Poll a bit faster than the chip internal Fs, in case > @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) > if (!chip->allow_async_readout) > sampling_us -= 200; > > + ktime_get_ts64(); > + > do { > - buffer_us = ina2xx_work_buffer(indio_dev); > - if (buffer_us < 0) > - return buffer_us; > + ret = ina2xx_work_buffer(indio_dev); > + if (ret < 0) > + return ret; > > - if (sampling_us > buffer_us) { > - delay_us = sampling_us - buffer_us; > - usleep_range(delay_us, (delay_us * 3) >> 1); > - } > + ktime_get_ts64(); > + > + do { > + timespec64_add_ns(, 1000 * sampling_us); > + delta = timespec64_sub(next, now); > + delay_us = timespec64_to_ns() / 1000; > + } while (delay_us <= 0); Umm. I'm lost, what is the purpose of the above dance? A comment perhaps. > + > + usleep_range(delay_us, (delay_us * 3) >> 1); > > } while (!kthread_should_stop()); >
[PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also, only part of the acquisition time is measured, thus the delay was longer than intended. Signed-off-by: Stefan Brüns--- drivers/iio/adc/ina2xx-adc.c | 35 +-- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 2621a34ee5c6..65bd9e69faf2 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) /* data buffer needs space for channel data and timestap */ unsigned short data[4 + sizeof(s64)/sizeof(short)]; int bit, ret, i = 0; - s64 time_a, time_b; + s64 time; unsigned int alert; - time_a = iio_get_time_ns(indio_dev); + time = iio_get_time_ns(indio_dev); /* * Because the timer thread and the chip conversion clock @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) data[i++] = val; } - time_b = iio_get_time_ns(indio_dev); + iio_push_to_buffers_with_timestamp(indio_dev, data, time); - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); - - return (unsigned long)(time_b - time_a) / 1000; + return 0; }; static int ina2xx_capture_thread(void *data) @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) struct iio_dev *indio_dev = data; struct ina2xx_chip_info *chip = iio_priv(indio_dev); int sampling_us = SAMPLING_PERIOD(chip); - int buffer_us, delay_us; + int ret; + struct timespec64 next, now, delta; + s64 delay_us; /* * Poll a bit faster than the chip internal Fs, in case @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) if (!chip->allow_async_readout) sampling_us -= 200; + ktime_get_ts64(); + do { - buffer_us = ina2xx_work_buffer(indio_dev); - if (buffer_us < 0) - return buffer_us; + ret = ina2xx_work_buffer(indio_dev); + if (ret < 0) + return ret; - if (sampling_us > buffer_us) { - delay_us = sampling_us - buffer_us; - usleep_range(delay_us, (delay_us * 3) >> 1); - } + ktime_get_ts64(); + + do { + timespec64_add_ns(, 1000 * sampling_us); + delta = timespec64_sub(next, now); + delay_us = timespec64_to_ns() / 1000; + } while (delay_us <= 0); + + usleep_range(delay_us, (delay_us * 3) >> 1); } while (!kthread_should_stop()); -- 2.15.1
[PATCH v1 5/7] iio: adc: ina2xx: Use a monotonic clock for delay calculation
The iio timestamp clock is user selectable and may be non-monotonic. Also, only part of the acquisition time is measured, thus the delay was longer than intended. Signed-off-by: Stefan Brüns --- drivers/iio/adc/ina2xx-adc.c | 35 +-- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c index 2621a34ee5c6..65bd9e69faf2 100644 --- a/drivers/iio/adc/ina2xx-adc.c +++ b/drivers/iio/adc/ina2xx-adc.c @@ -703,10 +703,10 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) /* data buffer needs space for channel data and timestap */ unsigned short data[4 + sizeof(s64)/sizeof(short)]; int bit, ret, i = 0; - s64 time_a, time_b; + s64 time; unsigned int alert; - time_a = iio_get_time_ns(indio_dev); + time = iio_get_time_ns(indio_dev); /* * Because the timer thread and the chip conversion clock @@ -752,11 +752,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) data[i++] = val; } - time_b = iio_get_time_ns(indio_dev); + iio_push_to_buffers_with_timestamp(indio_dev, data, time); - iio_push_to_buffers_with_timestamp(indio_dev, data, time_a); - - return (unsigned long)(time_b - time_a) / 1000; + return 0; }; static int ina2xx_capture_thread(void *data) @@ -764,7 +762,9 @@ static int ina2xx_capture_thread(void *data) struct iio_dev *indio_dev = data; struct ina2xx_chip_info *chip = iio_priv(indio_dev); int sampling_us = SAMPLING_PERIOD(chip); - int buffer_us, delay_us; + int ret; + struct timespec64 next, now, delta; + s64 delay_us; /* * Poll a bit faster than the chip internal Fs, in case @@ -773,15 +773,22 @@ static int ina2xx_capture_thread(void *data) if (!chip->allow_async_readout) sampling_us -= 200; + ktime_get_ts64(); + do { - buffer_us = ina2xx_work_buffer(indio_dev); - if (buffer_us < 0) - return buffer_us; + ret = ina2xx_work_buffer(indio_dev); + if (ret < 0) + return ret; - if (sampling_us > buffer_us) { - delay_us = sampling_us - buffer_us; - usleep_range(delay_us, (delay_us * 3) >> 1); - } + ktime_get_ts64(); + + do { + timespec64_add_ns(, 1000 * sampling_us); + delta = timespec64_sub(next, now); + delay_us = timespec64_to_ns() / 1000; + } while (delay_us <= 0); + + usleep_range(delay_us, (delay_us * 3) >> 1); } while (!kthread_should_stop()); -- 2.15.1