Skip to content

Clock Correction in msmod #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Clock Correction in msmod #2

wants to merge 15 commits into from

Conversation

@dricki
Copy link

@dricki dricki commented Jul 25, 2025 •
edited
Loading

We added clock correction as an msmod option. Data providers could use this to correct instrument times with a known drift, as is the case for ocean-bottom seismometers where the instrument-reference offset is measured before and after deployment.

The code in msmod.c is commented and generally follow the description in https://github.com/FDSN/OBS-standards/blob/main/other/msmod_drift_addition.md with the following caveats:

  1. Computation of time is done in microseconds and we log the time with higher precision than in the design document
  2. Logging is done via ms_log() function and since it is not initialized in msmod, the log output goes to the default stdout
  3. Because of 2) Error 5) (Log Exists) is not relevant
  4. Error 3 is currently implemented without a suggestion how to correct the the configuration file
  5. Error 4 is not implemented because the original version of msmod strips time correction before we enter processmod() function
    /* Revert time to uncorrected value if correction was applied during unpacking */
    if ( msr->fsdh->time_correct != 0 && ! (msr->fsdh->act_flags & 0x02) )
    {
    msr->starttime = msr_starttime_uc (msr);
    }
    So if the user inputs time-corrected input file and tries to do the same time correction, the input and output files will be identical and no error and error messages are printed out. If this behavior is not what we want I suggest Chad modifies msr_starttime_uc() function.

@chad-earthscope
Copy link

Thanks @dricki
Also tagging @WayneCrawford

Three issues before I can consider merging.

  1. No documentation in the man page for this new feature. The extended help (-H option) has a bit of detail regarding the option but it doesn't look sufficient for user to understand the features of the clock correction field. Extended help addition is (which also looks to have minor formatting issues):
  # The clock correction (--cc option) file format is: #
 type: {keyword} {parameters}
 # Instrument Time     Reference Time
 {instrument_time_0}   {reference_time_0}
 {instrument_time_1}   {reference_time_1}
 ....
  1. Compiler warnings with the new code. General rule is no compiler warnings with latest, or reasonably current, clang and gcc compilers. The warnings:
cc -g -I../libmseed -c msmod.c
msmod.c:298:17: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
  298 |           if ( retcode = processmods (msr) )
      |                ~~~~~~~~^~~~~~~~~~~~~~~~~~~
msmod.c:298:17: note: place parentheses around the assignment to silence this warning
  298 |           if ( retcode = processmods (msr) )
      |                        ^
      |                (                          )
msmod.c:298:17: note: use '==' to turn this assignment into an equality comparison
  298 |           if ( retcode = processmods (msr) )
      |                        ^
      |                        ==
msmod.c:1984:11: warning: using floating point absolute value function 'fabs' when argument is of integer type [-Wabsolute-value]
 1984 |       if (fabs(cc_config->ref_time[i] - correction - cc_config->inst_time[i]) > SMALL)
      |           ^
msmod.c:1984:11: note: use function 'llabs' instead
 1984 |       if (fabs(cc_config->ref_time[i] - correction - cc_config->inst_time[i]) > SMALL)
      |           ^~~~
      |           llabs
  1. The new code is a large addition relative to the main code. It makes code review difficult and will make future maintenance harder. New code should be in separate file.

@dricki
Copy link
Author

dricki commented Sep 4, 2025 via email

@WayneCrawford
Copy link

@dricki , I can make a stab at the help text, but would prefer that you add it in once I'm done, as I'm not very familiar/comfortable with pull requests.

@dricki
Copy link
Author

dricki commented Sep 5, 2025 via email

@WayneCrawford
Copy link

Here is my recommendation. I'm putting it here so that Chad can verify it's not too long before you go to the bother of inserting it:

      fprintf (stderr,
           "\n"
	       "  # Clock correction (--cc option) #\n"
	       " Sets timecorrection and modifies starttime in every record according\n"
	       " to a clock drift specified in the clock correction input file.\n"
	       "\n"
	       "The clock correction input file format is:"
	       " type: {keyword} [{parameters}]\n"
	       " # Instrument Time     Reference Time\n"
	       " {instrument_time_0}   {reference_time_0}\n"
	       " {instrument_time_1}   {reference_time_1}\n"
	       " ....\n"
	       "\n"
	       "The times in each column must monotonically increase and the\n"
	       "{instrument_time}s must cover the time range of the miniSEED file(s).\n"
	       "Time format is yyyy-mm-ddTHH:MM:SS(.FFFFF)Z.\n"
	       "Comment lines start with '#' and have no effect on processing.\n"
	       "Possible 'type' lines are:\n"
	       "type: piecewise_linear\n:
	       "    shifts instrument_time to reference_time for each provided value,\n"
	       "    linearly interpolates in between\n"
	       "type: cubic_spline\n"
	       "    shifts instrument_time to reference_time for each provided value,\n"
	       "    cubic spline interpolation in between\n"
	       "type: polynomial a0 a1 a2 a3...\n"
	       "    sets corrected_time = instrument_time_0 + a0 + a1*delta + a2*delta**2 + ...,\n"
	       "    where delta = instrument_time - instrument_time_0.\n"
           "    {instrument_time_n} and {reference_time_n} are used to validate results\n"
	       "\n"
	       ""Also outputs a text file named {CCFILENAME}.log, with the columns:\n"
	       "    RecNo: the record number\n"
	       "    Instrument time: original instrument time (ISO8601)\n"
	       "    Corrected to reference: corrected time (ISO8601)\n"
	       "    Corrected-Instrument: corrected time minus instrument time (s)\n"
	       "    Instrument-sync_inst[0]: instrument time - instrument_time_0 (s)\n"
	       "\n");

@chad-earthscope
Copy link

chad-earthscope commented Sep 5, 2025 •
edited
Loading

[Edit with more clarity]

The command line help message -h should include the option in the listing (as it is now) and in the extended message with -H a brief description, similar to what is there now but formatted better.

The doc/msmod.1 page should include:

  • addition of the --cc option to the argument list
  • the long description of the correction file format in it's own section.

@WayneCrawford
Copy link

With that in mind, I would recommend shortening the -H description to:

      fprintf (stderr,
           "\n"
	       " # Clock correction (--cc option) #\n"
	       "     Sets timecorrection and modifies starttime in every record according\n"
	       "     to the specified clock drift.\n"
	       "\n"
	       "    Clock correction input file format:"
	       "\n"
	       "    {type_line}\n"
	       "    {instrument_time_0}   {reference_time_0}\n"
	       "    {instrument_time_1}   {reference_time_1}\n"
	       "    ....\n"
	       "\n"
	       "    Possible {type_line}s:\n"
	       "    type: piecewise_linear\n:
	       "    type: cubic_spline\n"
	       "    type: polynomial a0 a1 a2 a3...\n");
	       "\n"
	       "    Time format: yyyy-mm-ddTHH:MM:SS(.FFFFF)Z.\n"

@dricki
Copy link
Author

dricki commented Sep 17, 2025 •
edited
Loading

@chad-earthscope and @WayneCrawford -
I think I have completed the requests from Chad:

  1. Compiler warnings
  2. Man page and help update
  3. Moving the new code to clockcorr.c file

.md file is not updated, versions and dates are not updated
Also makefiles.wat and makefile.win files need update to include the new source file

Please review the pull request

@WayneCrawford
Copy link

Hi Ilya

I will download and test the code. Could you copy the changes you put in msmod.1 to msmod.md? I tried but it requires me to fork.

Cheers
Wayne

@dricki
Copy link
Author

dricki commented Sep 22, 2025 via email

@WayneCrawford
Copy link

OK great,

I've started looking at the code. First, on my system, I get the following compile warning:

cc -g -I../libmseed -c clockcorr.c
clockcorr.c:697:11: warning: absolute value function 'abs' given an argument of type 'hptime_t' (aka 'long long') but has parameter of type 'int' which may cause truncation of value [-Wabsolute-value]
  697 |       if (abs(cc_config->ref_time[i] - correction - cc_config->inst_time[i]) > SMALL)
      |           ^
clockcorr.c:697:11: note: use function 'llabs' instead
  697 |       if (abs(cc_config->ref_time[i] - correction - cc_config->inst_time[i]) > SMALL)
      |           ^~~
      |           llabs
1 warning generated.

Then, when I run msmod -H I get the following "CC" section:

# Clock correction (--cc option) #
     Sets timecorrection and modifies starttime in every record according
     to the specified clock drift.

    Clock correction input file format:
    {type_line}
    {instrument_time_0}   {reference_time_0}
    {instrument_time_1}   {reference_time_1}
    ....

    Possible {type_line}s:
    type: piecewise_linear
:    type: cubic_spline
    type: polynomial a0 a1 a2 a3...

    Time format: yyyy-mm-ddTHH:MM:SS(.FFFFF)Z.

which I would change to (new stuff in "**", if Chad approves):

# Clock correction (--cc option) #
     Sets **record header** timecorrection and **starttime fields** according
     to the **original starttime and** the specified clock drift.

    Clock correction input file format:
    **type:** {type_value}
    {instrument_time_0}   {reference_time_0}
    {instrument_time_1}   {reference_time_1}
    ....

    Possible {type_value}s are:
        piecewise_linear
        cubic_spline
        polynomial a0 a1 a2 a3...

    **{*_time_*}** format: yyyy-mm-ddTHH:MM:SS(.FFFFF)Z

Finally, when I run

> msmod --cc clock_correct_cubic.txt test_30sph.mseed

I get

ERROR Unknown option: --cc

I get the same error for

> msmod --cc clock_correct_cubic.txt test_30sph.mseed -o test_30sph_cubic.mseed

Either I'm not using the command line correctly, or there is some bug that's not accepting "--cc". I will look to see if I find the reason but you will probably be a lot faster than me!

Cheers
Wayne

@WayneCrawford
Copy link

WayneCrawford commented Sep 24, 2025 •
edited
Loading

Complilation warning is gone.
--cc option is recognized (maybe I made an error during testing?)

cubic_spline file read gives error because it expects cubic-spline instead of cubic_spline. cubic_spline is how it's written in the docs and is consistent with the piecewise_linear type, please change to cubic_spline. Also, error message is not very informative:

ERROR: Badly formatted input file: line 1
ERROR reading CC configuration: 'clock_correct_cubic.txt'

maybe could say "unknown type on type line: xxx"

Otherwise, all the different test files (linear1, linear2, polynomial and cubic (with "cubic-spline")) give output, I will now check if the output matches the specs.

@WayneCrawford
Copy link

WayneCrawford commented Sep 24, 2025 •
edited
Loading

I checked the code with 9 different input files:

  • linear correction with two sync times
  • linear correction with three sync times
  • cubic spline correction with three sync times
  • polynomial correction with three sync times
  • 2 "bad" files in which the first sync is after the data start: one by 1 second, one by 1 month
  • 2 "bad" files in which the last sync is before the data end: one by 1 second, one by 1 month
  • a "bad" file in which the sync times are not increasing with each increasing row.
  • a "bad" file in which the reference synchronization time does not match the given polynomial

Major bug: miniSEED record header start times are not modified

In each new record header, the time correction value is entered and the "time correction applied" flag is set, but the start time is the same as before. Here's an example, pruned from msi -p using grep -e "start time" -e "time correction":

    mseed input file, last headers...
             start time: 2022,331,01:12:00.000000
        time correction: 0
             start time: 2022,340,05:14:00.000000
        time correction: 0
             start time: 2022,349,09:16:00.000000
        time correction: 0
             start time: 2022,358,13:18:00.000000
        time correction: 0
    mseed output file, last headers...
             start time: 2022,331,01:12:00.000000
        time correction: -12713
             start time: 2022,340,05:14:00.000000
        time correction: -13313
             start time: 2022,349,09:16:00.000000
        time correction: -13913
             start time: 2022,358,13:18:00.000000
        time correction: -14512

Drift correction values

  • The linear correction worked fine (within a microsecond of my results)
  • The cubic spline correction worked fine (but the "type" field should be "cubic_spline", not "cubic-spline"
  • The polynomial works ok, although it seems to confuse the "instrument" and "reference" times:
    • the sync_instrument[0] in the log file seems to actually be the sync_reference[0]
    • the CORRECTED-REFERENCE in the polynomial time check error message is actually CORRECTED-INSTRUMENT

Output log file

  • There is a strange large space in "Corrected to reference" on the first line
  • Seconds are shown to microsecond precision (miniSEED2 only has 100-microsecond precision, but this
    is probably fine as miniSEED3 has higher precision)

Output miniSEED file

  • Time corrections are truncations of the "Corrected - Instrument" rather than rounding to the closest value (for example "-14384" for "-1.438470"
  • Time corrections are not applied to the Record start time

Error catching and signalling

Time checking on polynomial

Works, but the CORRECTED-REFERENCE value is actually the CORRECTED minus the INSTRUMENT

Catching a too-late first sync time

Works for the 1 month offset, not for the 1 second offset.
The 1-second offset subsequently fails because of interpolation bounds, but the message should be the same as for the 1-month offset

Catching a too-early last sync time

Works for the 1-month offset, not for the 1-second offset.
No subsequent fail, probably because the last second starts more than a second before the end of data, but it should be caught anyway.

Catching an out-of-order sync time

Works.

Attached files

The zipped miniSEED file I ran the tests on:
test_30sph.mseed.zip

The "bad" test files I used:
clock_correct_bad_end_very.txt
clock_correct_bad_end.txt
clock_correct_bad_order.txt
clock_correct_bad_polynomial.txt
clock_correct_bad_start_very.txt
clock_correct_bad_start.txt

The code I used to run the tests:
run_tests.sh

The outputs of the different tests:
test_results.md

I can also provide the "standard" test clock_correct files and their expected outputs (I had to slightly modify them from what I provided initially, in order to match the msmod output)

@chad-earthscope
Copy link

Please ping me when this passes muster @WayneCrawford and I'll merge into the main repo.

The plan is to merge this into a 1.x branch and then I'll port the code to a new major version that includes miniSEED v2 and v3 support.

@WayneCrawford
Copy link

I recompiled the code (no errors or warnings) and ran the code on my test suite. For "good" input files, the outputs are all good and most of what's left is formatting, except for 1) the creation of miniSEED and log files after errors and 2) the identification of bad time bounds in piecewise_linear and cubic_spline input files:

Overview of test results

  • For good input files, the outputs are now all good (piecewise_linear, cubic_spline, and polynomial)
  • For the "error" input files, the error messages are generally good, but:
    • they still generate (sometimes empty) miniseed and log files generated, which could lead to confusion.
      Any errors should not generate output files (unless it's a log file with only the error messages)
    • Some of the error messages are not preceded by "ERROR": I think they should be, for clarity.
  • the log file still has a strange long space in the middle of the third header ("Corrected to reference") and that header starts before the column it represents
  • In the Markdown documentation, there seem to be two extraneous lines in the "Clock correction input file format" description:
    Screenshot 2025-10-02 at 12 10 19

I'm also still not sure what criteria you use to decide whether the date bounds cover the data time limits. Allowing a little slack seems to work ok for a slightly out of range end time, because the last record starts before the end of data (but by how much is variable). But you will always get an interpolation error for a bad start time, so you might as well nip it in the bud for consistency. I understand that this could create a problem for polynomials, which have a completely different relationship to the "instrument" and "reference" times. I suggest that you implement strict checking for piecewise_linear and cubic_spline, but not for polynomial

Individual test evaluations

clock_correct_bad_end_very.txt

Gives an error, but I think the first line should be preceded by ERROR, too:

Testing clock_correct_bad_end_very.txt...
Data ends after last instrument time (by 537238.50 seconds).
ERROR, processing Clock Correction failed
ERROR modifying:

Also still creates a mseed file and a log file, should not

clock_correct_bad_end.txt

No error, what is the tolerance (1 record?)

clock_correct_bad_order.txt

Good ERROR message, no mseed file created, empty log file created

clock_correct_bad_polynomial.txt

Good ERROR message, no mseed file created, empty log file created

clock_correct_bad_start_very.txt

Good ERROR message, empty mseed file and 1-line log file created

log file:

XX_STA__LXX, 000001, D, 4096, 6601 samples, 0.008333333333 Hz, 2022,001,00:00:00.000000

clock_correct_bad_start.txt

Error is created, but first line does not start with ERROR:

Time in MSEED header is out of interpolation bounds.
ERROR, processing Clock Correction failed
ERROR modifying:
  Error processing test_30sph.mseed: Generic error

empty mseed file and 1-line log file created

log file:

XX_STA__LXX, 000001, D, 4096, 6601 samples, 0.008333333333 Hz, 2022,001,00:00:00.000000

clock_correct_cubic.txt

My original input file still had cubic-spline, gave correct error, no mseed file created, but empty log file created

With corrected input file, Log and mseed files look good

clock_correct_linear1.txt

Log and mseed files look good

clock_correct_linear2.txt

Log and mseed files look good
(last significant digit is still not correctly rounded, for example -1.451288 seconds becomes -14512 time correction, but do we care?):

     40  2022-12-24T13:18:00.000000   2022-12-24T13:17:58.548712      -1.451288           30892680.000000
    mseed input file, last headers...
             start time: 2022,358,13:18:00.000000
        time correction: 0
    mseed output file, last headers...
             start time: 2022,358,13:17:58.548700
        time correction: -14512

clock_correct_polynomial.txt

Log and mseed files look good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dricki @chad-earthscope @WayneCrawford