Eric S. Raymond writes: > Achim Gratz <strom...@nexgo.de>: >> It wasn't a timeout, but a reversed application of an offset and a wrong >> conversion of the resulting delta value as an absolute timestamp… sigh. >> Here's the full patch that restores the DCF77 driver to function. > > I'll apply this if you like, but I'd prefer a merge request on GitLab so > you get properly credited in the commit history. You deserve it for this.
--8<---------------cut here---------------start------------->8--- From 09849ce568a7d40a86f81088ef7d453d140ddd21 Mon Sep 17 00:00:00 2001 From: Achim Gratz <strom...@stromeko.de> Date: Sun, 19 Feb 2017 18:40:00 +0100 Subject: [PATCH] Fix parse and DCF77 refclock include/timespecops: Remove misleading definitions of the result coding of the ternary comparison macros (the comments in that file are still wrong). libparse/parse.c (parse_timedout): Use the correct condition code (follow the examples elsewhere in the code by using comparison against zero). libparse/clk_rawdcf.c (calc_usecdiff): Apply the offset backwards as the original code before the lfp fixup did. The delta timestamp must not be treated as an absolute time, so use the appropriate conversion function. --- include/timespecops.h | 3 --- libparse/clk_rawdcf.c | 12 ++++++------ libparse/parse.c | 2 +- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/include/timespecops.h b/include/timespecops.h index 399287d..03848cb 100644 --- a/include/timespecops.h +++ b/include/timespecops.h @@ -215,9 +215,6 @@ abs_tspec( * compare previously-normalised a and b * return 1 / 0 / -1 if a < / == / > b */ -#define TIMESPEC_LESS_THAN 1 -#define TIMESPEC_EQUAL 0 -#define TIMESPEC_GREATER_THAN -1 static inline int cmp_tspec( diff --git a/libparse/clk_rawdcf.c b/libparse/clk_rawdcf.c index 4151f41..e3a03d8 100644 --- a/libparse/clk_rawdcf.c +++ b/libparse/clk_rawdcf.c @@ -509,11 +509,11 @@ calc_usecdiff( l_fp delt; delt = *ref; - bumplfpsint(delt, offset); + bumplfpsint(delt, -offset); delt -= *base; - delta = lfp_stamp_to_tspec(delt, NULL); + delta = lfp_uintv_to_tspec(delt); - delta_usec = 1000000 * (int32_t)delta.tv_sec + delta.tv_nsec/1000; + delta_usec = (NANOSECONDS/1000)*(int32_t)delta.tv_sec + delta.tv_nsec/1000; return delta_usec; } @@ -553,7 +553,7 @@ snt_rawdcf( parseio->parse_index - 1, delta_usec)); if (((parseio->parse_dtime.parse_status & CVT_MASK) == CVT_OK) && - (delta_usec < 500000 && delta_usec >= 0)) /* only if minute marker is available */ + (delta_usec < (NANOSECONDS/2000) && delta_usec >= 0)) /* only if minute marker is available */ { parseio->parse_dtime.parse_stime = *ptime; @@ -578,7 +578,7 @@ inp_rawdcf( timestamp_t *tstamp ) { - static struct timespec timeout = { 1, 500000000 }; /* 1.5 secongs denote second #60 */ + static struct timespec timeout = { 1, (NANOSECONDS/2) }; /* 1.5 seconds denote second #60 */ parseprintf(DD_PARSE, ("inp_rawdcf(0x%lx, 0x%x, ...)\n", (long)parseio, ch)); @@ -618,7 +618,7 @@ inp_rawdcf( delta_usec = -1; } - if (delta_usec < 500000 && delta_usec >= 0) + if (delta_usec < (NANOSECONDS/2000) && delta_usec >= 0) { parseprintf(DD_RAWDCF, ("inp_rawdcf: timeout time difference %ld usec - minute marker set\n", delta_usec)); /* collect minute markers only if spaced by 60 seconds */ diff --git a/libparse/parse.c b/libparse/parse.c index 702b718..612eeeb 100644 --- a/libparse/parse.c +++ b/libparse/parse.c @@ -38,7 +38,7 @@ parse_timedout( delt = *tstamp; delt -= parseio->parse_lastchar; delta = lfp_uintv_to_tspec(delt); - if (cmp_tspec(delta, *del) == TIMESPEC_GREATER_THAN) + if (cmp_tspec(delta, *del) > 0) { parseprintf(DD_PARSE, ("parse: timedout: TRUE\n")); return true; -- 2.1.4 --8<---------------cut here---------------end--------------->8--- > And *thanks*! This would have been impossible to notice without a live > DCF77 and extremely difficult for me to debug even if I knew it was there. > I'll add DCF77 to the list on the website of drivers tested and known good. It ran for a day without a hitch, but I wouldn't exactly call that tested. But yes, you have at least one user with a Conrad DCF77 receiver and I will update that box more often from now on. > You have also lifted a burden from my mind by confirming that the > generic-driver framework itself is not broken. The changes I had to make to > it during the big cleanup were devilishly tricky and have been at or > near the top of by worry list about where I was most likely to have > busted something. Just out of curiosity, why have you defined the lfp access macros in such an overly redundant manner? I realize that the compiler will optimize most of that away, but it seems odd to do that in the first place unless you're expecting to support a platform that has a non-conforming compiler that doesn't implement conversions between types of different rank correctly. Your reliance of standard types in other places seems to preclude that. So how about: #define lfpfrac(n) ((uint32_t)(n)) #define lfpsint(n) ((int32_t)((n) >> 32)) #define lfpuint(n) ((uint32_t)((n) >> 32)) #define bumplfpsint(n, i) ((n) += ((int64_t)(i) << 32)) #define bumplfpuint(n, i) ((n) += ((uint64_t)(i) << 32)) I'm sitting on the fence with the last two (there actually would not need to be two forms either as the result is the same either way). There could be processors where the compiler infers a multiply-add in the explicit multiplication and not recognize the shift-add form. But there's no real need for the "BUMP" constant otherwise and a compiler on a processor that supports halfword access should have no trouble just loading the upper word directly and not do a shift at all. Another argument against using a multiply is that a number of processors have 32x32 multiplication in hardware, but not 64x32 or 64x64. Even the HIGH and LOW masking could be replaced by nested casts: #define setlfpfrac(n, v) ((n) = (uint64_t)((((uint64_t)(lfpuint(n)))<< 32) | lfpfrac(v))) #define setlfpsint(n, v) ((n) = (int64_t)((((int64_t)(v)) << 32) | lfpfrac(n))) #define setlfpuint(n, v) ((n) = (uint64_t)((((uint64_t)(v)) << 32) | lfpfrac(n))) The comment about the signed and unsigned version producing the same result applies again. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ DIY Stuff: http://Synth.Stromeko.net/DIY.html _______________________________________________ devel mailing list devel@ntpsec.org http://lists.ntpsec.org/mailman/listinfo/devel