Thanks. It looks much more clean now, and it works. Some questions:

• rtc_offset if a 3-element array with a check value before and after the "hot" value. The check values are set, but I see not place where these are verified? They are declared static, so the check must happen in rtc.c. I guess the idea was to check it at rtc object instantiation time and either reset the offset value or keep it.
• Your code divides ticks_ms() by 999 to get seconds. That looks wrong, and if fact it results in the RTC running slightly too fast, like 3-4 seconds per hour.

About code style:

• The code that you marked as "Circuitpython" looks like previous code from Micropython, inherited by Circuitpython, except for the unused check values. So it may not be needed to tag it.
• You can remove commented code.
• And you can squash most/all commits, since many of them contain code that is not used any more in the actual state.

Thanks! I'm about to shut down for the night so I'll pick this up tomorrow.

Git is one of the things I'm still far from comfortable with, I was unaware of the "squash" option. I just googled it and will take care of it tomorrow as well!

Good night. Testing with a division by 1000 results in proper time after an hour. Using 999 it was as expected 3 seconds ahead.

There is another hiccup in the code.
The time is calculated as mp_hal_ticks_ms(()/1000 + offset.mp_hal_ticks_ms() returns an unsigned 32 bit quantity, which will roll over after about 50 days. At that time, the time & date will switch back 50 days.

So you would have replace ticks_ms() by something like:

uint64_t ticks_ms_64(void) {
    // Compute: (rtc_overflows << 24 + COUNTER) * 1000 / 32768
    //
    // Note that COUNTER * 1000 / 32768 would overflow during calculation, so use
    // the less obvious * 125 / 4096 calculation (overflow secure).
    //
    // Make sure not to call this function within an irq with higher prio than the
    // RTC's irq.  This would introduce the danger of preempting the RTC irq and
    // calling mp_hal_ticks_ms() at that time would return a false result.
    uint32_t overflows;
    uint32_t counter;
    // guard against overflow irq
    RTC1_GET_TICKS_ATOMIC(rtc1, overflows, counter)
    return ((uint64_t)overflows << 9) * 1000 + (counter * 125 / 4096);
}

mp_uint_t mp_hal_ticks_ms(void) {
    return ticks_ms_64();
}

and call tick_ms_64() in rtc_get_time() and rtc_set_time() instead of mp_hal_ticks_ms().

Another note: time.sleep() depends not on RTC. So it can be moved in modutime.c.
The same applies to ticks_cpu() which effectively is defined a 0.

I don't think I got the squash right, it didn't seem to have the effect I thought it would.

Never mind, it just took a minute 😁

@robert-hh Thank you so much for taking the time to walk me through all this!

You're welcome. I had the board now 6 hours running with the suggested code change, and the time is still on spot. So it looks OK. I also had tested the state, when value of ticks_ms_64() exceed 32 bit.
Edit: Personally I would not have added a rtc.h file for just that single declaration. You can as well put that into rtc.c.
Note that you can extend an existing commit after a change with: git commit -a --amend and the do a force push.

Personally I would not have added a rtc.h file for just that single declaration. You can as well put that into rtc.c. Note that you can extend an existing commit after a change with: git commit -a --amend and the do a force push.

I wasn't real happy with that either but pwm.h was right there and didn't have much in it either so I wasn't sure. I'll give the amend a try to fix it. 😁

Whatever you prefer. But if you drop rtx.c, do not forget to do git rm rtc.h. You code is running here. I'll let it go for a the next hours.

Could I have just pushed to get the amend to go or was the --force needed?

Looks Good. Git complains, if the force option is needed,

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

See alternate machine.RTC implementation in #13340.

I tried to remove the STATIC keywords, however I don't know github well enough to bring this PR up to date. If someone wants to walk me through the command line steps I need to rebase? this, just let me know.

Thanks.

• first step is to brings you current master branch in sync with the MP master. I do not know if you have a separate upstream and origin remote. In the latter case, you have to sync your fork first at the github site with the sync button.
• next rebase your RTC branch on top of the master branch with git rebase master. That might cause some merge conflicts. You then have to look which one you choose. If the conflicts are at the new files you added, usually your new changes have to be included. That will bring in again STATIC works, which have to be edited. Once you change the code for resolving a merge conflict, issue git add -vu, which will add the changed files, and git rebase --continue. You may have to repeat these steps, until the rebase finished
• Once the rebase is done, build and test your code.
• If everything is fine, run git push -f origin nrf-rtc, assuming that origin points at your fork of the MicroPython repository.

Okay, I think I've rebased but Micropython has changed some things basic to my PR and the RTC changes don't work with my newly built firmware.

It's been a long time since I worked on this and only made it through the first time with a lot of guidance from robert-hh so I'm thinking since there's another PR that addresses this, unless there's something my PR has which that one doesn't, this one should just be passed over in favor of #13340

Well I stole one block of code from the other PR and the RTC seems to be running again on my local build. Github still sees a conflict which I'm not sure how to resolve and there is no time.time() method.

>>> dir(machine.RTCounter)
['__class__', '__name__', 'start', 'stop', 'FREQUENCY', 'ONESHOT', 'PERIODIC', '__bases__', '__dict__', 'counter', 'deinit']
>>> dir(time)
['__class__', '__name__', '__dict__', 'sleep', 'sleep_ms', 'sleep_us', 'ticks_add', 'ticks_cpu', 'ticks_diff', 'ticks_ms', 'ticks_us']

To get time.time(), you have to set in mpconfigport.h the value:
#define MICROPY_PY_TIME_TIME_TIME_NS (1)
But that requires the function mp_time_time_get() in the nrf port files, which does not exist. But that can easily be created with the data from the RTC, similar to how rtc_get_time() gets the seconds.

The errors mentioned for the PR are caused by the commit message:
Remove STATIC macro, which is strange since it changes a file in the webassembly port as well.

Thanks,

I managed to update the "Rebase attempt" commit message but I'm having trouble using git rebase -i HEAD~2 updating the "Remove Static macro" message.

Do you think it's worth getting this cleaned up considering there is another PR? If there's some advantage I'll keep plugging at it...

The only thing missing seems to be time.time(). You can easily create the missing mp_time_time_get() with the data from the RTC, similar to how rtc_get_time() gets the seconds.

There still one commit message formatting complaint. The signature line is missing. Both with the Remove STATIC Macro and Rebase attempt commits. This formal stuff can be a pain.

Edit: <joke> There are plenty of these lines in the commit ports/nrf: Squashing early commits. </joke>

I haven't worked on a Micropython PR since this one over a year ago so I've forgotten all the github hoops. I know you walked me through a squash last year to clean things up, but sadly, your joke has gone over my head 🤷 😁

I'll go ahead and get the time.time going as well....

The only thing I'm curious about is This branch cannot be rebased due to conflicts. There is still a conflict in mphalport.c:

void rtc1_init_time_ticks(void) {
    // Start the low-frequency clock (if it hasn't been started already)
<<<<<<< HEAD
    mp_nrf_start_lfclk();
=======
    if (!nrf_clock_lf_is_running(NRF_CLOCK)) {
        nrf_clock_task_trigger(NRF_CLOCK, NRF_CLOCK_TASK_LFCLKSTART);
        rtc_offset_check();
    }
>>>>>>> 8f6072493 (ports/nrf: Updates for review comments.)
    // Uninitialize first, then set overflow IRQ and first CC event
    nrfx_rtc_uninit(&rtc1);
    nrfx_rtc_init(&rtc1, &rtc_config_time_ticks, rtc_irq_time);
    nrfx_rtc_overflow_enable(&rtc1, true);
    RTC_RESCHEDULE_CC(rtc1, 0, RTC_TICK_INCREASE_MSEC)
    nrfx_rtc_enable(&rtc1);
}

So you have to resolve that in a commit (any of them). Preferred in ports/nrf: Updates for review comments., because that commit brings it up.

I've been manually fixing that via the github web interface, but it was coming back after every push from local. I'll see if I can figure out how to resolve it in the prots/nrf: Update for review comments. commit.

I also noticed a bunch of unexpected changes to individual nrf boards' mpconfigboard.h files, so I'm going through and taking those changes out.

A lot of that was changed in the commit ports/nrf: Rewrite using RTCounter logic.

unless there's something my PR has which that one doesn't

One thing that this pull request has over mine (#13340) is the use of RTC_OFFSET_CHECK_PREFIX/SUFFIX to check whether the offset is valid, and putting the offset variable in a special section. I have not understood yet why or in what scenario that is useful, more useful than my approach of just using a static variable initialized to zero. But I have not spent much thought on it yet. Maybe you can explain?

I started this PR by lifting the code from CircuitPython and then with the guidance of robert-hh rearranged it to work with Micropython so I am probably not the right person to ask. That being said, could it have something to do with maintaining the clock through soft resets?

Well for what it's worth, this seems to be working again although github is telling me the branch cannot be rebased due to conflicts. It's no longer identifying the conflicts though.

Edit: If there's interest in merging this, I'm happy to submit a fresh PR with theses changes if the rebasing message means this PR is borked.

It's still mphalport.c which causes the trouble. But maybe starting over make things look better.

OK, I will take a look at what CircuitPython is doing there with the validity check.

By the way, starting over does not mean that you need to make a new PR. You can just force-push to this one.

Well that didn't go well, I obviously did the force push wrong since it closed the PR :/