Date: Fri, 25 Oct 2019 10:06:44 -0600 From: Ian Lepore <ian@freebsd.org> To: Andriy Gapon <avg@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r354076 - head/sys/dev/ow Message-ID: <8baac891351521c9e7148a0c0db9978645158dce.camel@freebsd.org> In-Reply-To: <01c62f31-1b10-6856-eb33-9238dcb0d9c6@FreeBSD.org> References: <201910251538.x9PFc9ii028313@repo.freebsd.org> <ca3e991449dda0ed70bbc578aa6069cefce40fc4.camel@freebsd.org> <2fde5b7b-6186-0969-08a5-83524b6aa274@FreeBSD.org> <0877c65ce11b0509dcc9e3ee491a37cc2a488f93.camel@freebsd.org> <01c62f31-1b10-6856-eb33-9238dcb0d9c6@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8baac891351521c9e7148a0c0db9978645158dce.camel>