Skip to content

kinetis: Refactor RTC into RTT, add RTC wrapper.#2406

Merged
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-rtt
Feb 14, 2015
Merged

kinetis: Refactor RTC into RTT, add RTC wrapper.#2406
jnohlgard merged 1 commit intoRIOT-OS:masterfrom
jnohlgard:pr/kinetis-rtt

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Feb 8, 2015

This is a refactor of the RTC module in kinetis_common which is closer to the actual CPU hardware. The Kinetis RTC module is what RIOT refers to as an RTT (a second counter). Added is also an RTC wrapper layer which could be used by any other platform without RTC implementation, but with a working RTT implementation (STM32F1 for example).

Question: How do I integrate a generic RTC implementation for platforms with RTT but without RTC into the current RIOT structure?
Should I add some check for FEATURES=(periph_rtt and not periph_rtc) in some Makefile in the drivers directory?

@jnohlgard jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Feb 8, 2015
@jnohlgard jnohlgard added the Area: drivers Area: Device drivers label Feb 8, 2015
@thomaseichinger
Copy link
Copy Markdown
Member

thomaseichinger commented Feb 9, 2015

Greatness! Just "ported" it to stm32f1 and works out of the box. I can open a PR against yours or do it separately.
Regarding your question:
I think this would simply provide an additional feature e.g. FEATURES = periph_rtt periph_rtc .... Although not implemented on physical hardware, it implements a periph interface and the overhead is not too big.

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 9, 2015

@thomaseichinger since the two changes are not really dependent on each other you might as well just open a separate PR for the STM32F1.

@jnohlgard jnohlgard mentioned this pull request Feb 9, 2015
Closed
45 tasks
@jnohlgard jnohlgard removed the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Feb 10, 2015
thomaseichinger
thomaseichinger reviewed Feb 12, 2015
View reviewed changes
cpu/kinetis_common/rtt.c Outdated
Copy link
Copy Markdown
Member

@thomaseichinger thomaseichinger Feb 12, 2015

Choose a reason for hiding this comment

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

Where does this come from? I can't find it anywhere...

Copy link
Copy Markdown
Member Author

@jnohlgard jnohlgard Feb 12, 2015

Choose a reason for hiding this comment

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

It is supposed to be defined in periph_conf.h for the board. The purpose is to activate the module clock gate.

@thomaseichinger
Copy link
Copy Markdown
Member

thomaseichinger commented Feb 12, 2015

Code looks good to me and the wrapper is tested on other platforms too.
As minor comment, is it feasible to replace RTC_Type rtc = RTT_DEV with RTC_Type rtt = RTT_DEV? This way it is easier on the eyes for me. Also there still exist a lot of RTC in the comments, maybe you could replace them when not meaning the hardware module. (For others it could be beneficial to mention Freescale's RTC is covered by RIOT's RTT in the file description.)

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 13, 2015

Updated addressing comments by @thomaseichinger :

  • rtc->rtt variable rename
  • File comment in rtt.c

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 13, 2015

Travis is happy except for squashing. Squash & go?

@jnohlgard jnohlgard added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 13, 2015
@thomaseichinger
Copy link
Copy Markdown
Member

thomaseichinger commented Feb 14, 2015

ACK, squash & go.

@jnohlgard jnohlgard removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 14, 2015
@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 14, 2015

squashed, rebased on latest master, will merge as soon as Travis goes green. (the rebase made the PR jump ahead about 130 commits, which is why I wait for the CI)

@jnohlgard
Copy link
Copy Markdown
Member Author

jnohlgard commented Feb 14, 2015

I realized waiting for Travis is pretty dumb since none of the kinetis_common utilizing platforms have been merged yet. Go!

jnohlgard pushed a commit that referenced this pull request Feb 14, 2015
Merge pull request #2406 from gebart/pr/kinetis-rtt
4b814c7 
kinetis: Refactor RTC into RTT, add RTC wrapper.
@jnohlgard jnohlgard merged commit 4b814c7 into RIOT-OS:master Feb 14, 2015
@jnohlgard jnohlgard mentioned this pull request Feb 14, 2015
Merged
@jnohlgard jnohlgard deleted the pr/kinetis-rtt branch February 27, 2015 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@thomaseichinger thomaseichinger

Labels

Area: drivers Area: Device drivers Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jnohlgard @thomaseichinger