Skip to content
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

Aging Register adjustment: suggested addition #299

Open
terrypin999 opened this issue Mar 14, 2024 · 34 comments · May be fixed by #300
Open

Aging Register adjustment: suggested addition #299

terrypin999 opened this issue Mar 14, 2024 · 34 comments · May be fixed by #300

Comments

@terrypin999
Copy link

Hobbyist here, not a programmer/developer/techie, so hope you’ll excuse this submission as an ‘Issue’.

I believe that, like me, other RTClib users get clock inaccuracy with the DS3231. Arduino Forum contributor @gfvalvo helped me to implement its adjustment after a simple addition to RTClib.h. So directly below the section

//
/*!
@brief RTC based on the DS3231 chip connected via I2C and the Wire library
*/
//
(which in the current release 2.1.3 ends with empty line 400), I’ve inserted the following five lines:

// Add Aging Register Access
static constexpr uint8_t agingRegister = 0x10;
int8_t readAging(){
return read_register(agingRegister);
}
void setAging(int8_t newAgingValue) {
write_register(agingRegister, newAgingValue);
}
};
My own sketch already includes the RTClib library and its minimal initialisation

// Initialize the rtc object
rtc.begin();
to which I added the following five lines in setup(), to compensate for my clock’s current slowness.

// Using edited RTClib.h from gfvalvo to adjust AgingRegister
int8_t aging = rtc.readAging();
Serial.println(aging);
int8_t newAging = -50;
rtc.setAging(newAging);
Trial and error will be needed (before hopefully reducing drift to a couple of seconds a week at most), but I already believe it to be working satisfactorily. The method seems significantly simpler than some I’ve seen, so I’d suggest its serious consideration for a future release please.
@edgar-bonet
Copy link
Contributor

@terrypin999: I added the methods you are requesting to my fork of RTClib, but I don't have the hardware to test them. Would you mind testing that yourself? It is in the branch “aging-offset” of my fork. The new methods are called getAgingOffset() and setAgingOffset():

constexpr int8_t calibratedAgingOffset = -50;

void setup() {
    rtc.begin();
    Serial.begin(9600);
    Serial.print("Initial aging offset: ");
    Serial.println(rtc.getAgingOffset());
    rtc.setAgingOffset(calibratedAgingOffset);
    Serial.print("New aging offset:     ");
    Serial.println(rtc.getAgingOffset());
}

If this works for you, I will submit a pull request here with the change.

@terrypin999
Copy link
Author

terrypin999 commented Mar 23, 2024 via email

@terrypin999
Copy link
Author

terrypin999 commented Mar 24, 2024 via email

@edgar-bonet
Copy link
Contributor

@terrypin999: I found a tutorial with good step-by-step instructions: Install an Arduino library from GitHub. It is provided both as a Web page and as a video. Note that this tutorial offers two options:

  • Download a specific version of the library (from a “tag”)
  • download the latest source code

Please, use the second option (latest source code), as there is no tag on this specific branch.

@terrypin999
Copy link
Author

terrypin999 commented Mar 24, 2024 via email

@edgar-bonet
Copy link
Contributor

@terrypin999: I guess this may be due to both libraries having the same name, as it appears in the name field of library.properties. I suggest you move your current RTClib directory out of …\libraries, and rename RTClib-aging-offset as RTClib.

@terrypin999
Copy link
Author

terrypin999 commented Mar 24, 2024 via email

@terrypin999
Copy link
Author

terrypin999 commented Mar 24, 2024 via email

@terrypin999
Copy link
Author

terrypin999 commented Mar 24, 2024 via email

@edgar-bonet
Copy link
Contributor

@terrypin999: I did not name the ZIP file: the name was automatically generated by GitHub. You may have to change it to RTClib.zip to make it work.

Why don’t you give your library a unique name

It's not my library. 😝

My fork is not a competing project: it is a tool meant for submitting pull requests in order to help make this version of the library (which is the official one) better. This is the “GitHub way” of collaborating on open source projects. See the introduction to the guide Contributing to a project.

what needs testing?

My implementation of the idea you suggested. I did not copy the “simple edit to RTClib.h” you posted. Instead, I re-implemented the same functionality in a way that is consistent with the style and coding conventions of this library.

@terrypin999
Copy link
Author

terrypin999 commented Mar 25, 2024 via email

@edgar-bonet
Copy link
Contributor

@terrypin999 wrote:

here's a link [to the error report]

Thanks for that report! One noteworthy thing I see there is:

Multiple libraries were found for "RTClib.h"
 Used: C:\Users\terry\Dropbox\Electronics\Arduino\SKETCHES\libraries\RTClib
 Not used: C:\Users\terry\Dropbox\Electronics\Arduino\SKETCHES\libraries\RTClib-MyEdit
 Not used: C:\Users\terry\Dropbox\Electronics\Arduino\SKETCHES\libraries\RTClib-aging-offset

So it found three alternatives, and chose the one in the folder named like the library: “RTClib”. If this is a copy of RTClib-aging-offset, it should work.

error: 'class RTC_DS3231' has no member named 'readAging'
error: 'class RTC_DS3231' has no member named 'setAging'

I named the methods getAgingOffset() and setAgingOffset(), for consistency with the current naming conventions and the datasheet.

If you can write a minimalised demo sketch that you think should compile with your version of RTClib then I'll try that.

This compiles:

#include <RTClib.h>

constexpr int8_t calibratedAgingOffset = -50;

RTC_DS3231 rtc;

void setup() {
    rtc.begin();
    Serial.begin(9600);
    Serial.print("Initial aging offset: ");
    Serial.println(rtc.getAgingOffset());
    rtc.setAgingOffset(calibratedAgingOffset);
    Serial.print("New aging offset:     ");
    Serial.println(rtc.getAgingOffset());
}

void loop(){}

Comparison of .cpp and .h: [...]

OK, as expected.

For the record, here is the diff between adafruit:master and edgar-bonet:aging-offset, as computed by GitHub itself.

@terrypin999
Copy link
Author

terrypin999 commented Mar 25, 2024 via email

@edgar-bonet
Copy link
Contributor

I indeed do not have a DS3231.

Can you, perhaps with help from another expert, suggest a sketch and schematic I can test that would give you confidence?

I just wrote the test sketch below. The required connections are:

  • Arduino's I2C pins to a DS3231
  • Arduino's USB port to a computer running either the Arduino's serial monitor or a terminal emulator
Source code (click to expand/collapse) 
#include <RTClib.h>

#ifdef MOCK_RTC

/* Mock DS3231 for testing this sketch without the actual hardware. */
class Mock_RTC_DS3231 : public RTC_Micros {
public:
    void begin() { RTC_Micros::begin(DateTime(F(__DATE__), F(__TIME__))); }
    int8_t getAgingOffset() { return aging_offset; }
    void setAgingOffset(int8_t offset) {
        aging_offset = offset;
        adjustDrift(-aging_offset * 200);  // exaggerate drift
    }
private:
    int8_t aging_offset;
};

#define RTC_DS3231 Mock_RTC_DS3231

#endif  // defined(MOCK_RTC)

RTC_DS3231 rtc;

const char help[] = "Available commands:\n\r"
        "  h: print this help\n\r"
        "  t: toggle printing RTC times\n\r"
        "  f: make the RTC fast\n\r"
        "  s: make the RTC slow\n\r"
        "  p: print the aging offset";

void setup() {
    rtc.begin();
    Serial.begin(9600);
    Serial.println("Ready.");
    Serial.println(help);
}

void loop() {
    /* Print RTC updates if asked to do so. */
    static bool do_print_times = false;
    if (do_print_times) {
        static DateTime last_printed_time;
        DateTime now = rtc.now();
        if (now != last_printed_time) {
            char buffer[] = "hh:mm:ss";
            Serial.println(now.toString(buffer));
            last_printed_time = now;
        }
    }

    /* Execute commands from the serial port. */
    switch (Serial.read()) {
        case 'h': Serial.println(help); break;
        case 't': do_print_times = !do_print_times; break;
        case 'f':
            Serial.println("setting aging offset to -128 (fast)");
            rtc.setAgingOffset(-128);
            break;
        case 's':
            Serial.println("setting aging offset to +127 (slow)");
            rtc.setAgingOffset(+127);
            break;
        case 'p':
            Serial.print("aging offset: ");
            Serial.println(rtc.getAgingOffset());
    }
}

The sketch is controlled by single-character commands sent through the serial port. Send h to get a summary of the available commands. You may check that:

  • printing the aging offset gets what you would expect
  • setting the RTC to run fast makes it run fast
  • setting it to run slow makes it run slow

I tested the sketch using a mock RTC which I left there for the record. It will not be used unless you #define MOCK_RTC.

@terrypin999
Copy link
Author

terrypin999 commented Mar 26, 2024 via email

@edgar-bonet
Copy link
Contributor

@terrypin999: No need to comment-out anything. The line you quoted is already hidden behind #ifdef MOCK_RTC.

@terrypin999
Copy link
Author

terrypin999 commented Mar 27, 2024 via email

@edgar-bonet
Copy link
Contributor

edgar-bonet commented Mar 27, 2024 •
edited

@terrypin999: Hello, and thanks for the test!

Before submitting a pull request, I would prefer to have clear evidence that the setting does affect the RTC’s drift rate. Rather than a stopwatch, I would rely on the Arduino serial monitor: it is already timestamping the received messages, with something like 30 ms of jitter. With this resolution, I would expect the RTC's drift to be visible after about 4 hours, and very clear after 8 hours. Do you think you could let it run for at least 8 hours in “fast” mode, and the same is “slow” mode? If so the procedure would be as follows:

  1. hit f to make the RTC fast
  2. hit t to start printing the RTC times
  3. let the serial monitor capture a dozen or so timestamps
  4. hit t again to avoid spamming the serial monitor
  5. let it sit there for 8+ hours
  6. repeat 2–4 to get a new series of timestamps
  7. hit s to make the RTC slow
  8. let it sit there for 8+ hours
  9. repeat 2–4 to get a new series of timestamps

If it works, the received serial messages (timestamped by the serial monitor) should contain evidence that the adjustment works.

Regards,

@terrypin999
Copy link
Author

terrypin999 commented Mar 27, 2024 via email

@edgar-bonet
Copy link
Contributor

What does ‘spamming the serial monitor’ mean? Just sending too fast?

Sending too much. If the Arduino kept printing the time every second for 8+ hours, you would end up with so much stuff in that window that it would be difficult to locate the place where you changed from “fast” mode to “slow” mode. And you would not want to post the whole capture here.

If you wish I’ll restart and follow your steps exactly after that.

If you are confident that the test you are doing is essentially equivalent to what I wrote, then there is no need to restart.

@terrypin999
Copy link
Author

terrypin999 commented Mar 27, 2024 via email

@terrypin999
Copy link
Author

terrypin999 commented Mar 27, 2024 via email

@edgar-bonet
Copy link
Contributor

@terrypin999: Two things

  1. You have to ignore the time that gets printed right after hitting the t key: this one is printed as you send the keystroke, not at a seconds boundary.

  2. I think you got the subtraction wrong. I get:

    18:41:07.113 -> 07:36:19
12:03:08.506 -> 00:58:21
────────────────────────
06:37:58.607 -> 06:37:58

This shows the RTC is slow by 0.6 s in 23878 s, which is about 25 ppm. This looks suspicious. Was any of those times caught right after hitting t? Could your computer's clock be running fast? Is it properly synchronized to an NTP server?

@terrypin999
Copy link
Author

terrypin999 commented Mar 27, 2024 via email

@edgar-bonet
Copy link
Contributor

I don’t see why you assume the reported time is EXACTLY 06:37:58.000

Not quite exactly, as there is about 30 ms of jitter. See my previous point number 1: if you ignore the time stamps printed after hitting t, all other timestamps are printed at full-second boundaries. This is because the Arduino is repeatedly (and very quickly) executing this piece of code:

DateTime now = rtc.now();
if (now != last_printed_time) {
    char buffer[] = "hh:mm:ss";
    Serial.println(now.toString(buffer));
    last_printed_time = now;
}

The RTC repeats the same time over and over again, and the sketch does not print it because it is always equal to last_printed_time. Then, once we cross a full-second boundary, the RTC returns a different time, and that one gets printed. The jitter is due in part to the time taken to go through the loop() (mostly the I2C communication time) and the handling of the serial data on the computer.

@terrypin999
Copy link
Author

terrypin999 commented Mar 27, 2024 via email

@terrypin999
Copy link
Author

terrypin999 commented Mar 28, 2024 via email

@edgar-bonet
Copy link
Contributor

[The RTC being slow by 0.6 s] looks suspicious because?

Because it is slow instead of fast.

I took the first time reported after using the t command.

OK. Then a drift computed using that time would be invalid.

If [my computer's clock is running fast] then by a tiny degree if I understand Win10 Pro 'Date & Time' settings right

I am not fluent with Windows. However, a synchronization period of 11+ hours seems to long for an NTP client. Mine (systemd-timesyncd) uses a poll interval of 32 seconds, which grows progressively as as the synchronization improves, up to a maximum of 34 min 8 s. This can keep the local clock to within a handful milliseconds from the reference server.

I suspect Windows may be periodically stepping the clock here, instead of fine-tuning its frequency in a step-less fashion, like an NTP server would do. If this is the case, maybe it would be better to temporarily disable the synchronization, and let the computer's clock run free, just to avoid unexpected clock discontinuities.

@terrypin999
Copy link
Author

terrypin999 commented Mar 28, 2024 via email

@terrypin999
Copy link
Author

terrypin999 commented Mar 28, 2024 via email

@edgar-bonet
Copy link
Contributor

User input aside, does it even need continuous running? I could just reconnect the USB and take a pair of readings at any time, yes?

Yes. As long as the Arduino and the RTC are continuously powered, the USB link can be removed and re-established at will.

@terrypin999
Copy link
Author

terrypin999 commented Mar 29, 2024 via email

@edgar-bonet
Copy link
Contributor

@terrypin999: I checked your data, converted everything to seconds to make the calculations easier, and here is what I see:

During the period around 22:10, the RTC was behind the PC by 39,889.344 seconds. During the next recorded period (around 12:48), the RTC was behind the PC by 39,891.528 seconds. Thus, the RTC lost 2.184 seconds over the course of 52,682.184 seconds. This is a drift rate of −41.4 ppm.

To me, this looks reasonable for a “slow” mode.

I will prepare a pull request.

@edgar-bonet edgar-bonet linked a pull request Mar 29, 2024 that will close this issue
Open
@terrypin999
Copy link
Author

terrypin999 commented Mar 29, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

DS3231: Add support for the Aging Offset Register edgar-bonet/RTClib
2 participants
@terrypin999 @edgar-bonet