svn commit: r354076 - head/sys/dev/ow

[Ian Lepore]

On Fri, 2019-10-25 at 18:51 +0300, Andriy Gapon wrote:
> On 25/10/2019 18:46, Ian Lepore wrote:
> > On Fri, 2019-10-25 at 15:38 +0000, Andriy Gapon wrote:
> > > Author: avg
> > > Date: Fri Oct 25 15:38:09 2019
> > > New Revision: 354076
> > > URL: https://svnweb.freebsd.org/changeset/base/354076
> > > 
> > > Log:
> > >   owc_gpiobus_read_data: compare times in sbintime_t units
> > >   
> > >   Previously the code used sbttous() before microseconds
> > > comparison
> > > in one
> > >   place, sbttons() and nanoseconds in another, division by
> > > SBT_1US
> > > and
> > >   microseconds in yet another.
> > >   
> > >   Now the code consistently uses multiplication by SBT_1US to
> > > convert
> > >   microseconds to sbintime_t before comparing them with periods
> > > between
> > >   calls to sbinuptime().  This is fast, this is precise enough
> > > (below
> > >   0.03%) and the periods defined by the protocol cannot overflow.
> > >   
> > >   Reviewed by:	imp (D22108)
> > >   MFC after:	2 weeks
> > > 
> > > Modified:
> > >   head/sys/dev/ow/owc_gpiobus.c
> > > 
> > > Modified: head/sys/dev/ow/owc_gpiobus.c
> > > =================================================================
> > > ====
> > > =========
> > > --- head/sys/dev/ow/owc_gpiobus.c	Fri Oct 25 15:02:50 2019	(
> > > r354
> > > 075)
> > > +++ head/sys/dev/ow/owc_gpiobus.c	Fri Oct 25 15:38:09 2019	(
> > > r354
> > > 076)
> > > @@ -296,10 +296,10 @@ owc_gpiobus_read_data(device_t dev, struct
> > > ow_timing *
> > >  	do {
> > >  		now = sbinuptime();
> > >  		GETPIN(sc, &sample);
> > > -	} while (sbttous(now - then) < t->t_rdv + 2 && sample == 0);
> > > +	} while (now - then < (t->t_rdv + 2) * SBT_1US && sample == 0);
> > >  	critical_exit();
> > >  
> > > -	if (sbttons(now - then) < t->t_rdv * 1000)
> > > +	if (now - then < t->t_rdv * SBT_1US)
> > >  		*bit = 1;
> > >  	else
> > >  		*bit = 0;
> > > @@ -307,7 +307,7 @@ owc_gpiobus_read_data(device_t dev, struct
> > > ow_timing *
> > >  	/* Wait out the rest of t_slot */
> > >  	do {
> > >  		now = sbinuptime();
> > > -	} while ((now - then) / SBT_1US < t->t_slot);
> > > +	} while (now - then < t->t_slot * SBT_1US);
> > >  
> > >  	RELBUS(sc);
> > >  
> > 
> > Unit conversions with sbt times should be done using the macros
> > that
> > carefully avoid roundoff errors.  I don't understand why you've
> > changed
> > the code that correctly did use those macros to inline math.
> 
> I think that the commit message explains it:
> This is fast, this is precise enough (below 0.03%) and the periods
> defined by
> the protocol cannot overflow.
> 
> Do you disagree?
> Could you please explain in which of the three lines changed the new
> code is
> worse and why?
> 

I absolutely disagree (or I wouldn't have replied).  Unit conversions
using sbt times should use the predefined macros, NOT incline
multiplication and division.  I don't know how to say it more clearly
than that.  The conversion macros are idiomatic (at least, they would
be if people stopped writing inline conversion expressions).

-- Ian