search

picocomputer / rp6502

Picocomputer 6502 firmware
BSD 3-Clause "New" or "Revised" License
85 stars 23 forks source link

WIP: API real time clock (issue #14) #26

Closed brentward closed 1 year ago

brentward commented 1 year ago

I just saw your notes so I'll rebase this to the dev branch before it is done.

brentward commented 1 year ago

I've tested this with my changes to the SDK and a simple program that uses the SDK to change the time and then read it back. I've also confirmed that when I write a rom to the file system the file metadata gets updated to the RTC time.

rumbledethumps commented 1 year ago

Don't use malloc. The only malloc in the whole project has already leaked and now I maintain a fork of pico-extras. Also, yours leaks.

Don't add new files to the fatfs folder. FatFs doesn't have a clean way to upgrade, so every change makes more work when merging new versions - forever. Changing ffconf.h is reluctantly ok because the author forces you into that.

No need to rebase. GitHub let's you change the target branch if you hit the edit button, which I did before you rebased and made this PR more difficult to review. Make a new PR.

There's potential memory access violations in rtc_api_set_time. Both a stray pointer and an unaligned read which will take down the entire kernel. I'll annotate on the new PR.

brentward commented 1 year ago

It isn't working anymore after merging in the changes from the develop branch. While I try to get it working again I'll make the changes you suggest.

rumbledethumps commented 1 year ago

I've had to toss away many a rebase myself. Probably all of them.

I'm going to make a new interface to read the stack safely. Don't worry about the memory access problems yet. I even did this myself as a side effect of refactoring, so a safe interface is needed.

Can you make it so apps know if the time is set? Don't start the RTC unless its set and return an error when requesting a time that's not yet been set. Also, keep the FatFs locked at whatever it's doing now, 1980-1-1 0:0:0 I think, until the time is set.

Think about WiFi too. Return codes like "Set", "Not set", and "NTP pending" would be useful. An app that depends on time can wait on NTP pending, ask for manual input if it's not set, and do nothing if it's set.

brentward commented 1 year ago

I've made some changes so the RTC won't start until it is set and if it isn't set FatFs use the NORTS values for the timestamp. I've started adding some response codes and I'm giving some thought about WiFi and how to indicate NTP pending.

The stack access memory issues are making it hard to test my changes. I'm getting errors depending on how I access the response. Are there any places in the existing code that I can look at for an example of how to properly read and write to the stack? When I was writing this I was looking at static void api_write_impl(bool is_xram) and static void api_read_impl(bool is_xram) in api/std.c

rumbledethumps commented 1 year ago

I'm adding variadic arguments right now. I knew this was coming, that's why stack access doesn't have a good interface yet. I need some time to finish. The stack is not easy to use at this level yet but it's central to everything so I won't rush it.

Whatever you did to clean up that rebase looks good. I can see just your changes now.