[ntp:hackers] More questions on the ATOM driver.
reg at dwf.com
Sun Mar 5 23:24:48 UTC 2006
I am confused.
First, I can NOT find a copy of the e-mail you sent out last fall describing
your changes to the ATOM driver. My recollection, however, was that you said
that you were doing additional preening of the PPS time-hacks, and with this
preening, the in-core version (using adjtime) was doing just as well as the
kernel version (using 'enable kernel').
This lead to my note of about six weeks ago questioning the need for the kernel
code at all, which I suggested would make the kernel maintainers happier.
(At least in the Linux world, anything that can be moved from the kernel
to user space is considered a plus).
In any case, I have since taken a look at the ATOM driver, and I am confused.
(1) Is the copy of the ATOM driver in ntp-dev, your latest and greatest?
Or is it at least the one you were describing in the previous note?
If not, then the rest of this not is probably irrelevant, but I continue.
Second, assuming that it is your latest, a couple of editorial points.
(2) At about line 70, at the end of the text at the top of the code you say:
If flag2 is dim (default), the on-time epoch is the assert edge of
the PPS signal;
If lit, the on-time epoch is the clear edge.
If flag2 is lit, the assert edge is used.
That comment makes no sense, and needs to be fixed to agree with the code.
(3) Down in atom_timer(), in the 2nd block of comments, at about line 370
If the PPS clock advanced once between polls, we make sure the fraction
time difference since the last sample is within the range gate of 5 ms
(Ok, but then you say)
If the PPS clock advanced twice since the last poll, the poll bracketed
more than one second and the first second was lost to a slip.
Since the interval since the last sample found is now two seconds,
just widen the range gate.
I see no code to implement this 2nd part, so either the code needs to be added
or this part of the comment should be deleted.
(4) You use atom_timer() in the 7th position of refclock_atom. This location
has always been marked as 'NOFLAGS' and 'Unused'.
It was for reasons like this that I asked several years ago if there wasn't
some documentation of the code (ntp) and your answer was no, and it seemed that
you liked it that way. The code (in addition to the algorithms used) really
NEEDS some documentation.
The atom_timer() gets the time-hack using the PPSAPI and then checks that the
dt around the previous PPS time-hack (+1sec) is less than 5ms.
The pps_sample routine is called from another driver is handed a time-hack,
but doesn't even do this simple test.
At this point both hand the time-hack with SAMPLE() to that great circular
buffer in the sky.
Sadly, THIS ONE TEST IS THE ONLY TEST I see in all of the ATOM code, from
your previous comments, I had expected to see lots of clever PREENING.
And shouldn't there be similar tests on both paths?
(5) This is just coding style and not an error. The code used to convert
and offset the time-hack in these two routines is VERY different. The
first (in atom_timer()) looks like someone copied the macro expansions from
somewhere into the code, the 2nd, 50 lines later in pps_sample, uses macros.
>From my point of view, it would be 'nice' if they both were coded the same
way. It would be easier to verify that they both were correct.
Finally, in atom_poll(), where the time-hack is going to be reported (or not)
to the upper levels on NTP, there are a couple of tests that have to do with
the pointers used in SAMPLE, a check that sys_prefer->offset is < 0.4 and
then refclock_receive is called (or not). I'm guessing that there is some
guarantee that the atom_timer() is called before the atom_poll(), but don't
where that is documented.
If the preening is in fact being done with the calls to SAMPLE, and then
being checked by testing the SAMPLE pointers, fine. But if this is the case
(I have not checked this code to see what it does) then it would seem that
adding less than a dozen lines of code to each of the current drivers
would accomplish the same thing as using the ATOM driver and prefer, and not
require the use of multiple drivers, and the prefer thing in the config file.
Of course, there would have to be some call after SAMPLE to get the preening
done, before the code to return the time-hack with refclock_receive().
Am I missing something?
To me, here doesn't seem to be enough going on in ATOM to justify its
reg at dwf.com
More information about the hackers