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

[Andriy Gapon]

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