8000 [ports/rp2] new machine_rtc.c module by astrorafael · Pull Request #6932 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

[ports/rp2] new machine_rtc.c module #6932

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
Closed
wants to merge 2 commits into from
Closed

[ports/rp2] new machine_rtc.c module #6932

wants to merge 2 commits into from

Conversation

astrorafael
Copy link

This is a basic implementation of machine_rtc.c module. No interrupt handling for the time being and conforms (I hope) to the interface documented in the MicroPython 1.14 official documentation.

It seems that somehow, updating my local & remote repos with upstream and doing git reset --mixed, made my previous PR #6929 to close. Sorry for the inconveniences.

The python test code is this:

from machine import RTC
from utime import sleep, sleep_ms

def test_main():
    rtc = RTC(0)
    print(rtc)
    print(rtc.now())
    remaining = rtc.alarm(0, 5000, repeat=False)
    while (remaining >0):
        print("{0:d} ms. left".format(remaining))
        sleep(1)
        remaining = rtc.alarm_left()
    print("done")

def test_cancel():
    rtc = RTC(0)
    print(rtc)
    print(rtc.now())
    remaining = rtc.alarm(0, 5000, repeat=False)
    while (remaining >0):
        print("{0:d} ms. left".format(remaining))
        sleep(1)
        remaining = rtc.alarm_left()
        rtc.cancel()

def test_1sec():
    rtc = RTC(0)
    rtc.init((2015, 1, 1, 1, 0, 0, 0, None))
    sleep_ms(1002)
    print(rtc.now())

@astrorafael
micropython/.gitignore: Ignore .vscode dir.
5264dec 
Signed-off-by: Rafael González <astrorafael@gmail.com>
@astrorafael
ports/rp2: Added basic machine.RTC class.
bf00161 
A basic machine.RTC class conforming to the API published in
http://docs.micropython.org/en/v1.14/library/machine.RTC.html
with no interrupts (no RTC.irq() method).

Signed-off-by: Rafael González <astrorafael@gmail.com>
@kadamski
Copy link
Contributor
kadamski commented Feb 19, 2021 •
edited
Loading

This is a duplicate of #6928.

Also it implements the "old" API.. This is the API described in the documentation but it is not really used by the ports, see #5733.

I guess @dpgeorge should first resolve the issue with the documentation vs common implementation so we know which API should indeed be used. I've seen Thonny and Rshell uses the API as described in #5733 so my pull request implemented it.

@astrorafael astrorafael mentioned this pull request Feb 19, 2021
Closed
kadamski
kadamski reviewed Feb 19, 2021
View reviewed changes
ports/rp2/machine_rtc.c
// RTC.now() method
STATIC mp_obj_t machine_rtc_now(mp_obj_t self_in) {
datetime_t t;
rtc_get_datetime(&t);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, this could return an error so just to be safe, it would be good to check the return value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will take care of it if this implementation gets the green light.

ports/rp2/machine_rtc.c
mp_obj_new_int(t.min),
mp_obj_new_int(t.sec),
mp_obj_new_int(0), // usecs
mp_const_none // tzinfo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example in the documetation (which sadly is not followed by the ports) puts 0 here instead of None.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, if we are using the standard datetime tuple convention, we should return None, as tzinfo is an object.

@astrorafael
Copy link
Author
astrorafael commented Feb 19, 2021 •
edited
Loading

Well, In fact, it is a duplicate of #6923 :-), which I messed up with my bad abilities at git.
I read the esp32 and the pyb implementation. I thought that they predated the API and were retained not to break existing code. I trusted the API in the official documentation. Too bad it is outdated.
This never happened to me, two developers contributing with the same functionality. At least we can say that this is a long wanted feature.
What should I do now ? Close the PR?
It is ok for me.

@kadamski
Copy link
Contributor

Well, In fact, it is a duplicate of #6923 :-), which I messed up with my bad abilities at git.

Anyways, we have now two PR open for machine.RTC on rp2. Yours and mine. Mine is simpler as it only supports setting/getting the time and nothing else but it uses the API that is compatible with other ports. Yours implements more but it uses API from the docs which seems outdated. Also you set the default time to some specific time as documented by the docs but @dpgeorge set different default time in main.c for some reason.

I read the esp32 and the pyb implementation. I thought that they predated the API and were retained not to break existing code. I trusted the API in the official documentation. Too bad it is outdated.
This never happened to me, two developers contributing with the same functionality. At least we can say that this is a long wanted feature.

Well, it was a low hanging fruit, wasn't it? :)

What should I do now ? Close the PR?
It is ok for me.

I don't think so. @dpgeorge should make his mind about the API and #5733. If mine implementation is accepted, you could add your alarms handling on top of that. Otherwise maybe he will prefer your implementation and mine is just thrown away. That being said, well, the project has almost 400 PRs hanging there so.. we'll see what happens.

I personally think that clarifying the API is more important than either of our implementations so lets hope this gives some momentum for fixing this. API inconsistencies are, in my opinion, the biggest issue in micropython.

@astrorafael
Copy link
Author

I personally think that clarifying the API is more important than either of our implementations so lets hope this gives some momentum for fixing this. API inconsistencies are, in my opinion, the biggest issue in micropython.

Totally agreed.

@dpgeorge
Copy link
Member

RTC support was added in 37d01d4 from #6928.

That added very basic RTC support. As discussed above we need to decide on a cross-port RTC API before adding more features like alarms and IRQs.

But thanks for your effort!

@dpgeorge dpgeorge closed this Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kadamski kadamski kadamski left review comments

Assignees
No one assigned
Labels
port-rp2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@astrorafael @kadamski @dpgeorge
0