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

[Ian Lepore]

On Fri, 2019-10-25 at 19:03 +0300, Andriy Gapon wrote:
> On 25/10/2019 18:56, Ian Lepore wrote:
> > 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).
> 
> I can agree that I should have used ustosbt() instead of
> multiplication by
> SBT_1US, but I don't agree with your original message that I changed
> the code
> that correctly used the macros.
> 
> But again, I know the times being converted, they are fixed by the
> protocol and
> I do not see why I have to use this:
> 
> static __inline sbintime_t
> ustosbt(int64_t _us)
> {
>         sbintime_t sb = 0;
> 
> #ifdef KASSERT
>         KASSERT(_us >= 0, ("Negative values illegal for ustosbt:
> %jd", _us));
> #endif
>         if (_us >= SBT_1S) {
>                 sb = (_us / 1000000) * SBT_1S;
>                 _us = _us % 1000000;
>         }
>         /* 9223372036855 = ceil(2^63 / 1000000) */
>         sb += ((_us * 9223372036855ull) + 0x7fffffff) >> 31;
>         return (sb);
> }
> 
> instead of this
> x * (((sbintime_t)1 << 32) / 1000000)
> 
> The times:
> static struct ow_timing timing_regular = {
>         .t_slot = 60,           /* 60 to 120 */
>         .t_low0 = 60,           /* really 60 to 120 */
>         .t_low1 = 1,            /* really 1 to 15 */
>         .t_release = 45,        /* <= 45us */
>         .t_rec = 1,             /* at least 1us */
>         .t_rdv = 15,            /* 15us */
>         .t_rstl = 480,          /* 480us or more */
>         .t_rsth = 480,          /* 480us or more */
>         .t_pdl = 60,            /* 60us to 240us */
>         .t_pdh = 60,            /* 15us to 60us */
>         .t_lowr = 1,            /* 1us */
> };
> 
> 

The fact that you could say all that means that there is nothing I can
say that is going to change your mind.  I will say if you ever do
anything like this to code I wrote, I will revert it immediately.

-- Ian