[riot-notifications] [RIOT-OS/RIOT] RTC for stm32f1 (#11258)

Marian Buschsieweke notifications at github.com
Mon Mar 25 15:50:00 CET 2019


maribu requested changes on this pull request.

Thanks for your pull request. There are some things that need to be addressed. I did only check the fundamental stuff and didn't read the datasheet/documentation required for a full review. So there could be more issues that need to be worked out that I didn't spot during the brief look I had at this PR. I will look into this in more detail when the current issues are addressed.

> +        // Reset not activated
+        RCC->BDCR &= ~RCC_BDCR_BDRST;
+
+        //RTC clock enabled
+        //LSE oscillator clock used as RTC clock10: LSI oscillator clock used as RTC clock
+        RCC->BDCR |= RCC_BDCR_RTCEN | RCC_BDCR_RTCSEL_LSE;
+
+        // Second interrupt is enabled.
+        RTC->CRH |= RTC_CRH_SECIE;
+
+        // Enter configuration mode.
+        RTC->CRL |= RTC_CRL_CNF;
+
+        //If the input clock frequency (fRTCCLK) is 32.768 kHz, write 7FFFh in this register to get a signal period of 1 second.
+        RTC->PRLH = 0;
+        RTC->PRLL = 0x8000;         //тактирование от внешнего кварца

Please use English for comments and only use c-style comments. E.g.

``` C
/* this is a  c style comment */
// This is c++ style
```

(I'm aware that C starting from C99 mandates support for c++ style comments, but RIOT's coding convention explicitly forbids using c++ style comments.)

> @@ -2,6 +2,7 @@
 FEATURES_PROVIDED += periph_adc
 FEATURES_PROVIDED += periph_i2c
 FEATURES_PROVIDED += periph_pwm
+FEATURES_PROVIDED += periph_rtc

Please split everything related to the blue pill into a separate commit

> +        // Resets the entire Backup domain
+        RCC->BDCR |= RCC_BDCR_BDRST;
+        // Reset not activated
+        RCC->BDCR &= ~RCC_BDCR_BDRST;
+
+        //RTC clock enabled
+        //LSE oscillator clock used as RTC clock10: LSI oscillator clock used as RTC clock
+        RCC->BDCR |= RCC_BDCR_RTCEN | RCC_BDCR_RTCSEL_LSE;
+
+        // Second interrupt is enabled.
+        RTC->CRH |= RTC_CRH_SECIE;
+
+        // Enter configuration mode.
+        RTC->CRL |= RTC_CRL_CNF;
+
+        //If the input clock frequency (fRTCCLK) is 32.768 kHz, write 7FFFh in this register to get a signal period of 1 second.

How does this comment relate to your code? You're writing `0x8000` into `RTC->PRLL`, should that be `0x7fff` instead?

> @@ -322,4 +329,257 @@ void ISR_NAME(void)
     cortexm_isr_end();
 }
 
+#elif defined(CPU_FAM_STM32F1)
+
+void rtc_init(void)
+{
+    DEBUG("[RTC rtc_init]\n");
+
+    // Enable APB1 clocks
+    RCC->APB1ENR |= RCC_APB1ENR_PWREN | RCC_APB1ENR_BKPEN;
+
+    // Disable backup domain write protection
+    PWR->CR |= PWR_CR_DBP;
+
+    if ((RCC->BDCR & RCC_BDCR_RTCEN) != RCC_BDCR_RTCEN) // if RTC clock disabled

The idea here is to skip initialization if it already has happened, right?

Generally, RIOT drivers should not assume that hardware is in a defined/well behaving state. E.g. assume a watchdog timer triggered a reboot in the middle of a configuration change. In that case a full reconfiguration of that hardware is required during driver initialization.

Back to this PR: I think this implies that the state of the RTC can only be one of the following:

a) disabled
b) enabled and correctly configured

Lets say the RTC  is instead indeed enabled, but incorrectly configured. In that case the initialization would fail.

> +{
+    return (RTC->ALRH << 16) | RTC->ALRL;
+}
+
+static void _rtc_set_counter_val(uint32_t counter_val)
+{
+    _rtc_enter_config_mode();
+    RTC->CNTH = (counter_val & 0xffff0000) >> 16; // CNT[31:16]
+    RTC->CNTL = counter_val & 0x0000ffff;         // CNT[15:0]
+    _rtc_exit_config_mode();
+}
+
+// (UnixTime = 00:00:00 01.01.1970 = JD0 = 2440588)
+#define JULIAN_DATE_BASE    2440588
+
+// Julian day number calculation - https://en.wikipedia.org/wiki/Julian_day

Why should the RTC use the Julian calendar?

> The Gregorian calendar is the most widely used civil calendar in the world.

https://en.wikipedia.org/wiki/Gregorian_calendar

> +    return (RTC->ALRH << 16) | RTC->ALRL;
+}
+
+static void _rtc_set_counter_val(uint32_t counter_val)
+{
+    _rtc_enter_config_mode();
+    RTC->CNTH = (counter_val & 0xffff0000) >> 16; // CNT[31:16]
+    RTC->CNTL = counter_val & 0x0000ffff;         // CNT[15:0]
+    _rtc_exit_config_mode();
+}
+
+// (UnixTime = 00:00:00 01.01.1970 = JD0 = 2440588)
+#define JULIAN_DATE_BASE    2440588
+
+// Julian day number calculation - https://en.wikipedia.org/wiki/Julian_day
+static void _rtc_get_time_from_count(const uint32_t rtc_counter, struct tm *time)

Please just use [`localtime()`](https://linux.die.net/man/3/localtime) instead

> +    year = 100 * b + d - 4800 + (m / 10);
+
+    time->tm_year = year - 1900;
+    time->tm_mon = mon;
+    time->tm_mday = mday;
+    time->tm_hour = hour;
+    time->tm_min = min;
+    time->tm_sec = sec;
+    time->tm_wday = wday;
+
+    DEBUG("[RTC time_from_count] c=%lu; %04i-%02i-%02i %02i:%02i:%02i\n", rtc_counter,
+        time->tm_year + 1900, time->tm_mon + 1, time->tm_mday,
+        time->tm_hour, time->tm_min, time->tm_sec);
+}
+
+// Julian day number calculation - https://en.wikipedia.org/wiki/Julian_day

Please use the Gregorian calendar. (See above.)

> +
+    time->tm_year = year - 1900;
+    time->tm_mon = mon;
+    time->tm_mday = mday;
+    time->tm_hour = hour;
+    time->tm_min = min;
+    time->tm_sec = sec;
+    time->tm_wday = wday;
+
+    DEBUG("[RTC time_from_count] c=%lu; %04i-%02i-%02i %02i:%02i:%02i\n", rtc_counter,
+        time->tm_year + 1900, time->tm_mon + 1, time->tm_mday,
+        time->tm_hour, time->tm_min, time->tm_sec);
+}
+
+// Julian day number calculation - https://en.wikipedia.org/wiki/Julian_day
+uint32_t _rtc_get_count_from_time(struct tm *time)

Please just use [`mktime()`](https://linux.die.net/man/3/mktime) instead

> +    _rtc_set_counter_val(_rtc_get_count_from_time(time));
+    return 0;
+}
+
+int rtc_get_time(struct tm *time)
+{
+    uint32_t time_count_val = rtc_get_counter_val();
+    _rtc_get_time_from_count(time_count_val, time);
+    return 0;
+}
+
+int rtc_set_alarm(struct tm *time, rtc_alarm_cb_t cb, void *arg)
+{
+    (void)cb;
+    (void)arg;
+    if (cb != NULL) {

How about using `assrert()` here?

> +}
+
+int rtc_set_time(struct tm *time)
+{
+    _rtc_set_counter_val(_rtc_get_count_from_time(time));
+    return 0;
+}
+
+int rtc_get_time(struct tm *time)
+{
+    uint32_t time_count_val = rtc_get_counter_val();
+    _rtc_get_time_from_count(time_count_val, time);
+    return 0;
+}
+
+int rtc_set_alarm(struct tm *time, rtc_alarm_cb_t cb, void *arg)

This cannot be working, as both `cb` and `arg` are never used. The idea is that on the alarm `cb(arg)` is called. To do that, I would expect that both the function pointer and the argument are stored somewhere.

Have a lot at `struct isr_ctx;`, which is used by the other code and should be used here as well. Also the interrupt service routine `ISRNAME()` (yes, it is really named like that in the code) needs to be executed when the alarm is triggered. I didn't checked into the datasheet/documentation, so I have no idea how the isr needs to be registered.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11258#pullrequestreview-218346770
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190325/eb86b8ee/attachment-0001.html>


More information about the notifications mailing list