Date: Fri, 25 Oct 2019 19:03:47 +0300 From: Andriy Gapon <avg@FreeBSD.org> To: Ian Lepore <ian@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: <01c62f31-1b10-6856-eb33-9238dcb0d9c6@FreeBSD.org> In-Reply-To: <0877c65ce11b0509dcc9e3ee491a37cc2a488f93.camel@freebsd.org> References: <201910251538.x9PFc9ii028313@repo.freebsd.org> <ca3e991449dda0ed70bbc578aa6069cefce40fc4.camel@freebsd.org> <2fde5b7b-6186-0969-08a5-83524b6aa274@FreeBSD.org> <0877c65ce11b0509dcc9e3ee491a37cc2a488f93.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 */ }; -- Andriy Gapon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?01c62f31-1b10-6856-eb33-9238dcb0d9c6>