Skip to content

cpu/stm32: reworked RTC implementation#7504

Merged
haukepetersen merged 7 commits intoRIOT-OS:masterfrom
haukepetersen:opt_stm32_rtc
Aug 24, 2017
Merged

cpu/stm32: reworked RTC implementation#7504
haukepetersen merged 7 commits intoRIOT-OS:masterfrom
haukepetersen:opt_stm32_rtc

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Aug 23, 2017 •
edited by aabadie
Loading

Successor of #6504 and should solve #6502 in the same way.

This PR reworks the RTC driver for the STM32 family. It works now with all but the stm32f1 (which it never did, although the feature was enabled for the iotlab...). Also this rework fixes some bugs in the old driver, e.g. rtc_set did not always work as expected.

The test application got also some improvements.

What I did not check is, weather all nucleo boards have a low frequency oscillator on board, so that we can safely enable the LSE clock for all nucleo boards. But this is something we can always do later.

This PR should be tested for at least one member of each STM family. Tested so far for the

  • nucleo-f072
  • stm32f3discovery
  • nucleo-f411
  • nucleo-l152
  • nucleo-l476
  • nucleo144-f746
  • nucleo-l053
  • nucleo144-f207

Could someone verify the changes for a stm32f2 and a stm32l0? Thx.

@haukepetersen haukepetersen added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 23, 2017
@haukepetersen haukepetersen added this to the Release 2017.10 milestone Aug 23, 2017
@haukepetersen haukepetersen mentioned this pull request Aug 23, 2017
Closed
@haukepetersen haukepetersen force-pushed the opt_stm32_rtc branch 2 times, most recently from 63e9c80 to d5e02ac Compare August 23, 2017 13:54
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Aug 23, 2017

@haukepetersen, tested on nucleo-l053 and nucleo144-f207: works.

vincent-d
vincent-d requested changes Aug 23, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@vincent-d vincent-d left a comment

Choose a reason for hiding this comment

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

Just a minor comment

cpu/stm32_common/periph/rtc.c Outdated
(((uint32_t)byte2bcd(time->tm_mon + 1) << 8) & (RTC_DR_MT | RTC_DR_MU)) |
(((uint32_t)byte2bcd(time->tm_mday) << 0) & (RTC_DR_DT | RTC_DR_DU)));
/* set clock to 24-h mode and enable timestamps */
RTC->CR = RTC_CR_TSE;
Copy link
Copy Markdown
Member

@vincent-d vincent-d Aug 23, 2017

Choose a reason for hiding this comment

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

This should be done after prescaler setting

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Aug 24, 2017

Choose a reason for hiding this comment

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

fixed

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 23, 2017

It would be great if you could disable RTC output in init with this:

/* Disable alternate function outputs */
RTC->TAFCR = RTC_TAFCR_TSINSEL | RTC_TAFCR_TAMPINSEL;

that way we could close #7326 in favor of that one.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2017

Thanks for the tests and reviews.

I optimized the driver again:

  • do not enable alarm mode anymore (as we dont use it anyway)
  • reset config registers
  • harden EXTI channel initialization
  • add check for unlocking init mode
  • remove redundant init mode lines in rtc_set_time

@vincent-d: I think the driver should not use any pins as it is configured now (alarm and tamper are both not enabled). So the RTC->TAFCR = RTC_TAFCR_TSINSEL | RTC_TAFCR_TAMPINSEL; should not be needed as I see it. Do you agree with this?

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 24, 2017

@haukepetersen I need to check again, maybe this was because alarm was active, but I was not able to use PC13 as gpio, that's why I added that line.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2017

Which board did you encounter this on? Might be good if we find the exact cause for this...

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 24, 2017

Which board did you encounter this on?

On our own hardware. I'm retesting, and I'll try to reproduce on a nucleo.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2017

I just found some more specific doc for the f411 (ref manual, section 8.3.15 Selection of RTC_AF1 and RTC_AF2 alternate functions). What I can't really figure out though, is if one can disable the RTC pin output completely and how one would do this...

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2017

Just checked, on the nucleo-f411 I can use pin PC13 manually while the RTC creating events, without the RTC interfering with the pin state. So the current RTC driver should be fine and should not connect to any pins.

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 24, 2017

I ran some tests (on nucleo144-f207), it's becoming a bit more clear.
My main initialize rtc (rtc_init) then toggle PC13 indefinitely.

Status is:

  • current master: KO (no output on PC13)
  • this branch right after current master (no complete power down, as if vbackup was present): KO
  • this branch after a power down (remove power): OK

But if you do:

/* reset configuration */
RTC->CR = 0;
RTC->ISR = 0;

right after rtc_unlock();, this branch works, even without powering down.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2017

Ok, how about the change in the latest commit then? We just can't set RTC->ISR = 0, as we would leave the init mode which is needed for setting the prescaler below, so I changed your proposal to RTC->ISR = RTC_ISR_INIT.

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 24, 2017

as we would leave the init mode

Indeed!

Retested your latest commit, works fine.

@vincent-d
Copy link
Copy Markdown
Member

vincent-d commented Aug 24, 2017

ACK

@vincent-d vincent-d mentioned this pull request Aug 24, 2017
Closed
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2017

squashed.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Aug 24, 2017

ACKed and all green -> go.

Thanks again for the help here!

@haukepetersen haukepetersen merged commit 75333b6 into RIOT-OS:master Aug 24, 2017
@haukepetersen haukepetersen deleted the opt_stm32_rtc branch August 24, 2017 13:46
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Aug 24, 2017

Nice one!

@aabadie aabadie mentioned this pull request Nov 4, 2017
Closed
@kYc0o kYc0o mentioned this pull request Jan 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vincent-d vincent-d vincent-d approved these changes

Assignees

@vincent-d vincent-d

@aabadie aabadie

@MichelRottleuthner MichelRottleuthner

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2017.10

Development

Successfully merging this pull request may close these issues.

5 participants

@haukepetersen @aabadie @vincent-d @kaspar030 @MichelRottleuthner