| Message ID | 20190623122909.hhzskp6k5vm4wify@arbad (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: srf04: fix wrong limitation in distance measuring | expand |
On Sun, 23 Jun 2019 14:29:10 +0200 Andreas Klinger <ak@it-klinger.de> wrote: > The measured time value in the driver is limited to the maximum distance > which can be read by the sensor. This limitation was wrong and is fixed > by this patch. > > It also takes into account that we are supporting a variety of sensors > today and that the recently added sensors have a higher maximum > distance range. > > Suggested-by: Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> > Signed-off-by: Andreas Klinger <ak@it-klinger.de> Ideally I'm looking for Zbyněk to confirm that this addresses the original question. Thanks, Jonathan > --- > drivers/iio/proximity/srf04.c | 29 +++++++++++++++-------------- > 1 file changed, 15 insertions(+), 14 deletions(-) > > diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c > index 8b50d56b0a03..01eb8cc63076 100644 > --- a/drivers/iio/proximity/srf04.c > +++ b/drivers/iio/proximity/srf04.c > @@ -110,7 +110,7 @@ static int srf04_read(struct srf04_data *data) > udelay(data->cfg->trigger_pulse_us); > gpiod_set_value(data->gpiod_trig, 0); > > - /* it cannot take more than 20 ms */ > + /* it should not take more than 20 ms until echo is rising */ > ret = wait_for_completion_killable_timeout(&data->rising, HZ/50); > if (ret < 0) { > mutex_unlock(&data->lock); > @@ -120,7 +120,8 @@ static int srf04_read(struct srf04_data *data) > return -ETIMEDOUT; > } > > - ret = wait_for_completion_killable_timeout(&data->falling, HZ/50); > + /* it cannot take more than 50 ms until echo is falling */ > + ret = wait_for_completion_killable_timeout(&data->falling, HZ/20); > if (ret < 0) { > mutex_unlock(&data->lock); > return ret; > @@ -135,19 +136,19 @@ static int srf04_read(struct srf04_data *data) > > dt_ns = ktime_to_ns(ktime_dt); > /* > - * measuring more than 3 meters is beyond the capabilities of > - * the sensor > + * measuring more than 6,45 meters is beyond the capabilities of > + * the supported sensors > * ==> filter out invalid results for not measuring echos of > * another us sensor > * > * formula: > - * distance 3 m > - * time = ---------- = --------- = 9404389 ns > - * speed 319 m/s > + * distance 6,45 * 2 m > + * time = ---------- = ------------ = 40438871 ns > + * speed 319 m/s > * > * using a minimum speed at -20 °C of 319 m/s > */ > - if (dt_ns > 9404389) > + if (dt_ns > 40438871) > return -EIO; > > time_ns = dt_ns; > @@ -159,20 +160,20 @@ static int srf04_read(struct srf04_data *data) > * with Temp in °C > * and speed in m/s > * > - * use 343 m/s as ultrasonic speed at 20 °C here in absence of the > + * use 343,5 m/s as ultrasonic speed at 20 °C here in absence of the > * temperature > * > * therefore: > - * time 343 > - * distance = ------ * ----- > - * 10^6 2 > + * time 343,5 time * 106 > + * distance = ------ * ------- = ------------ > + * 10^6 2 617176 > * with time in ns > * and distance in mm (one way) > * > - * because we limit to 3 meters the multiplication with 343 just > + * because we limit to 6,45 meters the multiplication with 106 just > * fits into 32 bit > */ > - distance_mm = time_ns * 343 / 2000000; > + distance_mm = time_ns * 106 / 617176; > > return distance_mm; > }
On Wed, 26 Jun 2019 20:59:08 +0200 Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> wrote: > Hello, Andreas, > > Your patch seems to solve my question. I haven't had time to test it because of holidays. I'll test it as soon as possible. No rush, it's a clear fix anyway so we can queue this up in the rc phase of the next kernel cycle! Thanks, Jonathan > > Zbyněk > --- > email: zbynek.kocur@fel.cvut.cz > phone: +420 224 354 054 > web: http://www.fel.cvut.cz > Department of Telecommunications Engineering > Faculty of Electrical Engineering > > > On 26 Jun 2019, at 20:21, Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sun, 23 Jun 2019 14:29:10 +0200 > > Andreas Klinger <ak@it-klinger.de> wrote: > > > >> The measured time value in the driver is limited to the maximum distance > >> which can be read by the sensor. This limitation was wrong and is fixed > >> by this patch. > >> > >> It also takes into account that we are supporting a variety of sensors > >> today and that the recently added sensors have a higher maximum > >> distance range. > >> > >> Suggested-by: Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> > >> Signed-off-by: Andreas Klinger <ak@it-klinger.de> > > Ideally I'm looking for Zbyněk to confirm that this addresses the > > original question. > > > > Thanks, > > > > Jonathan > > > >> --- > >> drivers/iio/proximity/srf04.c | 29 +++++++++++++++-------------- > >> 1 file changed, 15 insertions(+), 14 deletions(-) > >> > >> diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c > >> index 8b50d56b0a03..01eb8cc63076 100644 > >> --- a/drivers/iio/proximity/srf04.c > >> +++ b/drivers/iio/proximity/srf04.c > >> @@ -110,7 +110,7 @@ static int srf04_read(struct srf04_data *data) > >> udelay(data->cfg->trigger_pulse_us); > >> gpiod_set_value(data->gpiod_trig, 0); > >> > >> - /* it cannot take more than 20 ms */ > >> + /* it should not take more than 20 ms until echo is rising */ > >> ret = wait_for_completion_killable_timeout(&data->rising, HZ/50); > >> if (ret < 0) { > >> mutex_unlock(&data->lock); > >> @@ -120,7 +120,8 @@ static int srf04_read(struct srf04_data *data) > >> return -ETIMEDOUT; > >> } > >> > >> - ret = wait_for_completion_killable_timeout(&data->falling, HZ/50); > >> + /* it cannot take more than 50 ms until echo is falling */ > >> + ret = wait_for_completion_killable_timeout(&data->falling, HZ/20); > >> if (ret < 0) { > >> mutex_unlock(&data->lock); > >> return ret; > >> @@ -135,19 +136,19 @@ static int srf04_read(struct srf04_data *data) > >> > >> dt_ns = ktime_to_ns(ktime_dt); > >> /* > >> - * measuring more than 3 meters is beyond the capabilities of > >> - * the sensor > >> + * measuring more than 6,45 meters is beyond the capabilities of > >> + * the supported sensors > >> * ==> filter out invalid results for not measuring echos of > >> * another us sensor > >> * > >> * formula: > >> - * distance 3 m > >> - * time = ---------- = --------- = 9404389 ns > >> - * speed 319 m/s > >> + * distance 6,45 * 2 m > >> + * time = ---------- = ------------ = 40438871 ns > >> + * speed 319 m/s > >> * > >> * using a minimum speed at -20 °C of 319 m/s > >> */ > >> - if (dt_ns > 9404389) > >> + if (dt_ns > 40438871) > >> return -EIO; > >> > >> time_ns = dt_ns; > >> @@ -159,20 +160,20 @@ static int srf04_read(struct srf04_data *data) > >> * with Temp in °C > >> * and speed in m/s > >> * > >> - * use 343 m/s as ultrasonic speed at 20 °C here in absence of the > >> + * use 343,5 m/s as ultrasonic speed at 20 °C here in absence of the > >> * temperature > >> * > >> * therefore: > >> - * time 343 > >> - * distance = ------ * ----- > >> - * 10^6 2 > >> + * time 343,5 time * 106 > >> + * distance = ------ * ------- = ------------ > >> + * 10^6 2 617176 > >> * with time in ns > >> * and distance in mm (one way) > >> * > >> - * because we limit to 3 meters the multiplication with 343 just > >> + * because we limit to 6,45 meters the multiplication with 106 just > >> * fits into 32 bit > >> */ > >> - distance_mm = time_ns * 343 / 2000000; > >> + distance_mm = time_ns * 106 / 617176; > >> > >> return distance_mm; > >> } > > >
Thank you, i will write a feedback after testing. Zbynek --- email: zbynek.kocur@fel.cvut.cz phone: +420 224 354 054 web: http://www.fel.cvut.cz Department of Telecommunications Engineering Faculty of Electrical Engineering > On 26 Jun 2019, at 21:49, Jonathan Cameron <jic23@kernel.org> wrote: > > No rush, it's a clear fix anyway so we can queue this up > in the rc phase of the next kernel cycle!
Hi Zbynek, do you have any feedback for us? Thank you, Andreas Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> schrieb am Mi, 26. Jun 21:50: > Thank you, i will write a feedback after testing. > > > Zbynek > --- > email: zbynek.kocur@fel.cvut.cz > phone: +420 224 354 054 > web: http://www.fel.cvut.cz > Department of Telecommunications Engineering > Faculty of Electrical Engineering > > > On 26 Jun 2019, at 21:49, Jonathan Cameron <jic23@kernel.org> wrote: > > > > No rush, it's a clear fix anyway so we can queue this up > > in the rc phase of the next kernel cycle! >
Hi Andreas, I'm so sorry for the delay. I was working on another project now, and I wasn't entirely devoted to this. I looked at it at the weekend and the modified code is working. I have only tested OpenWRT in build - r10204-38b22b1, kernel 4.14.125. I do not expect complications on other platforms. I tried it on several sensors. I was primarily interested in JSN-SR04T (https://www.makerguides.com/jsn-sr04t-arduino-tutorial/). I have also verified other designs (https://randomnerdtutorials.com/complete-guide-for-ultrasonic-sensor-hc-sr04/). It generally works. Relatively accurate for the area up to 2.5 m. However, if the distance is longer, it starts to fizz, depending on the sensor design. It is a property of used HW and its construction. I do not know how to do this in the development of drivers, whether it makes sense to put a filter directly into the driver or at least pull the maximum distance parameter as a variable? Anyway, the change made works. Zbynek --- email: zbynek.kocur@fel.cvut.cz phone: +420 224 354 054 web: http://www.fel.cvut.cz Department of Telecommunications Engineering Faculty of Electrical Engineering > On 7 Sep 2019, at 11:46, Andreas Klinger <ak@it-klinger.de> wrote: > > Hi Zbynek, > > do you have any feedback for us? > > Thank you, > > Andreas > > > Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> schrieb am Mi, 26. Jun 21:50: >> Thank you, i will write a feedback after testing. >> >> >> Zbynek >> --- >> email: zbynek.kocur@fel.cvut.cz >> phone: +420 224 354 054 >> web: http://www.fel.cvut.cz >> Department of Telecommunications Engineering >> Faculty of Electrical Engineering >> >>> On 26 Jun 2019, at 21:49, Jonathan Cameron <jic23@kernel.org> wrote: >>> >>> No rush, it's a clear fix anyway so we can queue this up >>> in the rc phase of the next kernel cycle! >> > > -- > Andreas Klinger > Grabenreith 27 > 84508 Burgkirchen > +49 8623 919966 > ak@it-klinger.de > www.it-klinger.de > www.grabenreith.de
Hi Zbyněk, thanks a lot for testing and sharing your results with us. > I do not know how to do this in the development of drivers, whether it makes > sense to put a filter directly into the driver or at least pull the maximum > distance parameter as a variable? In this case one should read several times, remove spikes and calculate the average. This sounds to me as task for the userspace not for the driver. Regards, Andreas Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> schrieb am Mo, 09. Sep 08:53: > Hi Andreas, > > I'm so sorry for the delay. I was working on another project now, and I wasn't entirely devoted to this. I looked at it at the weekend and the modified code is working. I have only tested OpenWRT in build - r10204-38b22b1, kernel 4.14.125. I do not expect complications on other platforms. > > I tried it on several sensors. I was primarily interested in JSN-SR04T (https://www.makerguides.com/jsn-sr04t-arduino-tutorial/). I have also verified other designs (https://randomnerdtutorials.com/complete-guide-for-ultrasonic-sensor-hc-sr04/). > > It generally works. Relatively accurate for the area up to 2.5 m. However, if the distance is longer, it starts to fizz, depending on the sensor design. It is a property of used HW and its construction. > > I do not know how to do this in the development of drivers, whether it makes sense to put a filter directly into the driver or at least pull the maximum distance parameter as a variable? > > Anyway, the change made works. > > Zbynek > --- > email: zbynek.kocur@fel.cvut.cz > phone: +420 224 354 054 > web: http://www.fel.cvut.cz > Department of Telecommunications Engineering > Faculty of Electrical Engineering > > > On 7 Sep 2019, at 11:46, Andreas Klinger <ak@it-klinger.de> wrote: > > > > Hi Zbynek, > > > > do you have any feedback for us? > > > > Thank you, > > > > Andreas > > > > > > Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> schrieb am Mi, 26. Jun 21:50: > >> Thank you, i will write a feedback after testing. > >> > >> > >> Zbynek > >> --- > >> email: zbynek.kocur@fel.cvut.cz > >> phone: +420 224 354 054 > >> web: http://www.fel.cvut.cz > >> Department of Telecommunications Engineering > >> Faculty of Electrical Engineering > >> > >>> On 26 Jun 2019, at 21:49, Jonathan Cameron <jic23@kernel.org> wrote: > >>> > >>> No rush, it's a clear fix anyway so we can queue this up > >>> in the rc phase of the next kernel cycle! > >> > > > > -- > > Andreas Klinger > > Grabenreith 27 > > 84508 Burgkirchen > > +49 8623 919966 > > ak@it-klinger.de > > www.it-klinger.de > > www.grabenreith.de >
diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c index 8b50d56b0a03..01eb8cc63076 100644 --- a/drivers/iio/proximity/srf04.c +++ b/drivers/iio/proximity/srf04.c @@ -110,7 +110,7 @@ static int srf04_read(struct srf04_data *data) udelay(data->cfg->trigger_pulse_us); gpiod_set_value(data->gpiod_trig, 0); - /* it cannot take more than 20 ms */ + /* it should not take more than 20 ms until echo is rising */ ret = wait_for_completion_killable_timeout(&data->rising, HZ/50); if (ret < 0) { mutex_unlock(&data->lock); @@ -120,7 +120,8 @@ static int srf04_read(struct srf04_data *data) return -ETIMEDOUT; } - ret = wait_for_completion_killable_timeout(&data->falling, HZ/50); + /* it cannot take more than 50 ms until echo is falling */ + ret = wait_for_completion_killable_timeout(&data->falling, HZ/20); if (ret < 0) { mutex_unlock(&data->lock); return ret; @@ -135,19 +136,19 @@ static int srf04_read(struct srf04_data *data) dt_ns = ktime_to_ns(ktime_dt); /* - * measuring more than 3 meters is beyond the capabilities of - * the sensor + * measuring more than 6,45 meters is beyond the capabilities of + * the supported sensors * ==> filter out invalid results for not measuring echos of * another us sensor * * formula: - * distance 3 m - * time = ---------- = --------- = 9404389 ns - * speed 319 m/s + * distance 6,45 * 2 m + * time = ---------- = ------------ = 40438871 ns + * speed 319 m/s * * using a minimum speed at -20 °C of 319 m/s */ - if (dt_ns > 9404389) + if (dt_ns > 40438871) return -EIO; time_ns = dt_ns; @@ -159,20 +160,20 @@ static int srf04_read(struct srf04_data *data) * with Temp in °C * and speed in m/s * - * use 343 m/s as ultrasonic speed at 20 °C here in absence of the + * use 343,5 m/s as ultrasonic speed at 20 °C here in absence of the * temperature * * therefore: - * time 343 - * distance = ------ * ----- - * 10^6 2 + * time 343,5 time * 106 + * distance = ------ * ------- = ------------ + * 10^6 2 617176 * with time in ns * and distance in mm (one way) * - * because we limit to 3 meters the multiplication with 343 just + * because we limit to 6,45 meters the multiplication with 106 just * fits into 32 bit */ - distance_mm = time_ns * 343 / 2000000; + distance_mm = time_ns * 106 / 617176; return distance_mm; }
The measured time value in the driver is limited to the maximum distance which can be read by the sensor. This limitation was wrong and is fixed by this patch. It also takes into account that we are supporting a variety of sensors today and that the recently added sensors have a higher maximum distance range. Suggested-by: Zbyněk Kocur <zbynek.kocur@fel.cvut.cz> Signed-off-by: Andreas Klinger <ak@it-klinger.de> --- drivers/iio/proximity/srf04.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)