gpsd-dev
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric S. Raymond
Subject: Re: [gpsd-dev] Mysteriously vanishing bugs don't make me happy
Date: Mon, 4 Nov 2013 15:46:19 -0500
User-agent: Mutt/1.5.21 (2010-09-15)

Gary E. Miller <address@hidden>:
> FYI, after many days of running chronyd my clock stability is now down
> to +/- 400 nSec.  Yes, nSec, not uSec, but it takes a while to get there.

That reminds me.  I was doing a bit of cleanup on the PPS code - teaching it to
use the new struct timedrift_t and pushing nanosecond precision as close
to the code for shipping time hints as possible - and I ran across a couple
of things that looked odd.

/* actual_ts is when we think the PPS pulse wass */
/* clock_ts is the local clocke time we saw the pulse */
/* offset is actual_ts - clock_ts */
static void chrony_send(struct gps_device_t *session,
                        struct timespec *actual_ts,
                        struct timespec *clock_ts UNUSED,  double offset)
{
    struct sock_sample sample;

    /* FIXME!! offset is double of the error from local time */
    /* chrony expects tv-sec since Jan 1970 */
    sample.pulse = 0;
    sample.leap = session->context->leap_notify;
    sample.magic = SOCK_MAGIC;
    TSTOTV(&sample.tv, actual_ts);
    sample.offset = offset; 

    (void)send(session->chronyfd, &sample, sizeof (sample), 0);
}

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, 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.

My point is, I wanted to check with you about whether that TSOTV really
belongs there; if so, we're only shipping microsecond precision to
chrony.  If not, perhaps it should be replaced by a structure assignment?

The other thing is this code in ntpshm.c:

    /* this is more an offset jitter/dispersion than precision,
     * but still useful for debug */
    offset = fabs((double)(clock_tv.tv_sec - actual_tv.tv_sec)
                  + ((double)(clock_tv.tv_usec - actual_tv.tv_usec) / 
1000000.0));

This is the opposite way around from the way offsets are computed
elsewhere, e.g near ppsthread.c:438.  Is this actually right?

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.  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.
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>

Attachment: signature.asc
Description: Digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]