Start and halt oscillator without setting current timestamp

[NorbertsSoftWorld]

Many thanks for the DS1302 functions. You publish a halt() function. The data sheet says the oscillator halt flag is undefined after power lost. So it would be a good idea also to publish a start() function to set the oscillator into running mode.

[rafacouto]

That function is implemented with the setDateTime() function. But you are right since ds1302 could be used with delta time without the need of setting the current timestamp. A start() function will be implemented in short. Thank you for the comment.

[rafacouto]

Any volunteer to test this patch? @NorbertsSoftWorld ?

[photomultiplier]

Hello @rafacouto, I tested the feature/issue-4 branch (#7) and the halt() and start() functions work as expected. Only thing is that I found a race condition if the clock is halted at :59 seconds, just before the minute changes, in which case the following happens:

• halt() is called
• CE is set to high
• the DS1302 copies the current date and time into a separate register so that the clock can continue to run (as per the datasheer, page 6 of 13, under CLOCK/CALENDAR: «When reading or writing the time and date registers, secondary (user) buffers are used to prevent errors when the internal registers update. When reading the time and date registers, the user buffers are synchronized to the internal registers the rising edge of CE.»)
• while the seconds' register (which is set to 59, as this is the user register) is being read the second passes, making the minutes' register advance and resetting the actual seconds' register to 00
• the old seconds value (59) gets written to the DS1302 together with the halt flag
• result: the clock is 59 seconds ahead!

I realize this is quite the edge case, but I've actually been able to reproduce this issue manually with a sketch that halts the clock at the press of a button.

A possible solution may be to read both the seconds and the minutes' register in burst read mode (so they're synchronised), then halt the clock, then check if the minutes' register is still the same. If it's different (not necessarily one greater, which would create problems at the hour change), re-halt the clock with 00 in the seconds' register.

If you like this solution I can open a PR implementing it. Cheers!

[photomultiplier]

I've just implemented this change, and from my testing it seems to solve the issue. I've opened a PR (#9), let me know what you think. Cheers!

[rafacouto]

You realized! Good job about your implementation but I think it's also needed to avoid the race condition on the start() function.

I just commit a new patch to this PR that set/clear the halt flag by managing the both registers involved in a possible RC. Could you please test this new code again in order to review if race conditions are gone?

Thank you very much for your time and collaboration :100:

[photomultiplier]

Hello again!

First of all you're welcome, and thank you for the time you're also putting into this!

You're right that a RC might also happen in the start function - I assumed the clock wouldn't be running, but it might very well be if, for example, the function is called twice in a row.

I took a look at your code and it looks really clean - splitting the functions was a nice idea.

That said, there's still a race condition! Reading and writing both the minutes and seconds registers solves the RC in case of a seconds overflow, but it introduces a new one when both the minutes and seconds are at 59 and the seconds roll over; in which case the clock runs 59 minutes and 59 seconds ahead.

I don't see a way to resolve it without comparing the minutes before and after the seconds's register is written, unless we read (and then write) the whole date like getDateTime and setDateTime do (or else halting the clock at 23:59:59 2050-12-31 would still create problems). Reading and writing the whole date would require 16 bytes (1 command byte + 7 registers, twice) to be transferred each time _setHaltFlag is called; while using an if statement reduces this to 9 bytes in the worst case and 7 bytes normally:

void Ds1302::_setHaltFlag(bool stopped)
{
    _prepareRead(REG_BURST);
    uint8_t seconds = _readByte();
    uint8_t minutes = _readByte();
    _end();

    if (stopped) seconds |= 0b10000000; else seconds &= ~0b10000000;

    _prepareWrite(REG_SECONDS);
    _writeByte(seconds);
    _end();

    _prepareRead(REG_MINUTES);
    uint8_t new_minutes = _readByte();
    _end();

    /* avoid race conditions at :59 */
    if (minutes != new_minutes) {
        _prepareWrite(REG_SECONDS);
        _writeByte(seconds & 0b10000000);
        _end();
    }
}

Let me know what solution you prefer, for now I'll update my PR (#9) to use this one above. Cheers and thanks again!

P.S. The DS1302 data sheet says on page 6 that «When writing to the clock registers in the burst mode, the first eight registers must be written in order for the data to be transferred». However from my testing this doesn't seem to cause any problems, as even just writing two bytes after _prepareWrite(REG_BURST) still correctly sets both the minutes and seconds register... maybe it means that the date is updated only after the transmission ends (by pulling CE low) or after 8 bytes; while the RAM registers get updated after each byte is sent? But I don't see why that would be important :sweat_smile:

[rafacouto]

Oh!, it's a recursive problem with edge time values (last second in minute, last minute in hour... and so on), the more high the less probability but always there...

I've just commit a new version of _setHaltFlag() that reads and writes the involved registers with read and write burst transactions:

1. read the current registers (burst of)
2. manipulate the halt flag on the seconds register (the 1st one)
3. WP register (the 8th one) is also changed
4. write modified registers (burst of)

I hope this change definively avoids race conditions on edge time values. I've coded this on the fly, without any validation (you saw I missed some signature on .h file :angel: ). Please, I appreciate if you could review and test... :-)

P.S: I read a "can" where you say "must" (DS1302 datasheet):

[photomultiplier]

Hello!

Now it works! It compiles correctly and there are no race conditions to be seen.

There's just one thing I haven't been able to test, which is the write protect flag... for some reason it seems to be broken on my DS1302: after setting it I can still write to other registers and if I read the flag again it always returns that it isn't set. So that's a mystery...

Regarding the data sheet, I was actually referring to another part, on page 6: But as I said it seems to not be true!

Cheers!

[rafacouto]

New release v1.1.0 with this feature! Thank you very much @photomultiplier :100: