DCF77 driver seems to be broken

[Achim Gratz]

Eric S. Raymond writes:
> Achim Gratz <Stromeko at 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 <Stromeko at 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