Re: [gpsd-dev] Mysteriously vanishing bugs don't make me happy

[Eric S. Raymond]

Gary E. Miller <address@hidden>:
> I can find nothing on timedrift_t.  Not in current git, not in my
> system headers.  Please let us avoid reinventing the wheel.

It's in gps.h:

struct timedrift_t {
    struct timespec     real;
    struct timespec     clock;
};
#define TIMEDIFF(drift) ((((drift)->real.tv_sec - (drift)->clock.tv_sec)*1e9)\
                         + ((drift)->real.tv_nsec - (drift)->clock.tv_nsec))
 

I looked and didn't find anything like this in the standards relating
to POSIX time.  By having this data type we simplify the interface to
the PPS-loop hooks considerably.

> > and pushing nanosecond
> > precision as close to the code for shipping time hints as possible -
> 
> And I have been doing the reverse. nSec are the future, stamp out uSec
> where we can.  Push nsec everywhere, nt at the last instant.  Can we not
> go off in two competing directions?

Perhaps I wasn't clear.  We're going in the *same* direction.

> > and I ran across a couple of things that looked odd.
> 
> Yes, but can you please tread carefully so we do not lose another week?
> It takes me longer to find your bugs than to do it myself.

We lost nearly a week because the carnivorous-config bug was screwing
up my builds enough to prevent me from running effective
regressions. Looking back, I think that's also why I screwed up my
earlier first attempt at preventing the KPPS initialization race - I
thought code was being compiled that was actually conditioned out!

We don't have that problem now.  And the PPS visibility in gpsmon
means that when I have a GR-601W hooked up, it's a 10-second check to
verify that hasn't broken and that my PPS offset looks reasonable.  So
I can test comprehensively and frequently, and am thus back to
cranking out good code at high speed.

Remember my list of 8 bugs from late last week? All dead as of this
afternoon, unless your SiRF-III problem resurrects.

> > What seems off about this is that (I think) you've mentioned chronyd
> > taking nanosecond-scale corrections before, yet the TSOTV steps
> > precision down to microseconds. Is this correct?
> 
> Yes, it is correct.  The correction is in nSec, the time to be corrected
> is in uSec.  It makes no sense, but that is their documented interface and
> it works quite well.

OK, got it.
 
> > Yes, I realize this doesn't look quite like you might remember.  It
> > used to take a struct timeval actual argument; I changed that, and
> > added the stepdown, to (a) emulate the previous behavior exactly, but
> > (b) consistently pass our around our internal time representations in
> > timespecs, up-converting from timevals at the places where they're
> > generated and down-converting to timevals only when that is explicitly
> > needed.  That is less confusing.
> 
> Yes, and I already explained to you that was what I was going to do
> before the recent unpleasantness.

Sorry, you weren't explicit enough about that for me to know that's what
you were going to do next. That could be my error of understanding.

> > I'm thinking I might wrote a macro or inline function to replace your
> > timediff_kpps() macro and this calculation and a couple of other
> > places where it's done ad-hoc.
> 
> I'm confused why people want to replace single instances of code with
> a macro.  If you can find more tha one place it is used fine, but just
> replacing a single instance is pointless.

Sometimes it's an encapsulation device. But that's not the case here;
we're talking about replacing about a dozen instances.

> > Having N different implementations
> > floating around with subtly different scaling is just begging for
> > trouble. One - with scaling made explicit at each call point - would
> > be better.
> 
> The problem is that we get time serveral different ways and we have to
> give time in several different ways.  So things are done n-different
> ways because there are n unique equations to solve.  You just can not
> make things orthongal.  Worse yet we need the last decimal point of
> accuracy even going to the point of rounding, so the order of operations
> is very important in non-obvious ways.

I know just enough numerical analysis to know how you do this sort of
thing right. You write your generic macro with operations that don't
lose precision (except in the cade of integer overflow, which isn't a
risk here).  The TIMEDIFF macro (not yet used anywhere) is an example.

#define TIMEDIFF(drift) ((((drift)->real.tv_sec - (drift)->clock.tv_sec)*1e9)\
                         + ((drift)->real.tv_nsec - (drift)->clock.tv_nsec))

An analogous candidate two-argument macro would be:

#define TIMEDIFF_NSEC(drift)    ((((a)->real.tv_sec - (b)->clock.tv_sec)*1e9)\
                                 + ((a)->real.tv_nsec - (b)->clock.tv_nsec))

Because this is all integer arithmetic with no division, you're guaranteed
no loss until you downconvert to microseconds; then you apply any division 
or rounding you're going to do just once.

> Best to get the code stable and let me get on with the previously
> discussed goal of moving everything to a timespec as soon as possible
> and keeping iot there as long as possible.

Go. I've got enough to do going through thr Time Service HOWTO instructions.
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>

signature.asc
Description: Digital signature