The RTC driver topic

[Giovanni]

Please keep this on hold, I want to analyze impact first.

Giovanni

[barthess]

I have suspended development of RTC driver and merged changes to trunk. Timekeeping on F4x works. For realization of callback functionality we need to review EXTI driver.

[Giovanni]

In my opinion the v2 RTC driver should not implement any callback functionality, the RTC does not support interrupts. It has to be done through the existing EXT driver.

Giovanni

[barthess]

You are right. I again began to complicate things.

[CheBuzz]

I think something might be broken in latest revision. Compiling the STM32F4-Discovery demo, I get:

Compiling ../../test/test.c
In file included from ../../os/hal/include/rtc.h:72:0,
from ../../os/hal/include/hal.h:53,
from ../../test/test.c:30:
../../os/hal/platforms/STM32/RTCv2/rtc_lld.h:72:2: error: #error "invalid source selected for RTC clock"

*edit - Found the problem. mcuconf.h needs "#define STM32_RTC STM32_RTC_LSI"

[barthess]

Hi there.
First of all, we need to stabilize API to release a stable version of OS. Therefore all development moved to "rtc_dev" branch.

Second thing is little offtopic. If you use battery for powering backup domain than you must enable power voltage detector to properly shut down core when main supply is off. Also, on STM32F4x in 100 pins (or lager) package you must connect PDR_ON pin to VDD. I forgot to do that on my board and observe power consumption about 2.8mA from battery.

[Giovanni]

Hi barthess,

I moved RTCv1 clock initialization to the HAL driver and removed it from RTCv1, could you please verify it? We need to finalize the F4 version too.

Giovanni

[barthess]

I will check it tomorrow.
About F4 version. Full feature driver for it requires changes in HL API but you have decline that changes. We need to discuss new API.

[barthess]

Ok, now it works (rev. 3790). Added lock for threadsafe and interruptsafe usage.
Also I think that disabling write access to BKP register is pointless. Write protection needed only during startup phase to protect from parasitic write. Lets programmer to think about safe writing to BKP.

About HL API for F4x. Let it be like for F1x. Interrupts will be configured in EXTI driver (like in PVD driver). Only rtcSetCallback() function became pointless but it may be eclosed in #if defined(STM32F4XX) ... #endif clause. I will have time to work with RTC on F4x only on this weekend.

[Giovanni]

Let's finish the F1 first, there are problems with the latest changes and in general.

1) The function acquire just waits for the previous write to finish, the previous implementation was better and more descriptive of how HW actually works. The real acquire and release would be eventually RTC->CRL |= RTC_CRL_CNF and RTC->CRL &= ~RTC_CRL_CNF. Those really acquire and release a resource. Note that the ST library does exactly that.
2) You cannot use chSysLock() and chSysUnlock() from the initialization function, it is invoked before the kernel is initialized, the state checker would not like it.
3) There is no requirement for atomicity so locks are not required, if we decide that function should be atomic then locks should be handled in the high level driver and not left to the implementation of the LLD.

About various changes:

Code: Select all

static void rtc_lld_acquire(void) {

  /* Waits registers write completion.*/
BEGIN:
  while ((RTC->CRL & RTC_CRL_RTOFF) == 0)
    ;
  chSysLock();
  if ((RTC->CRL & RTC_CRL_RTOFF) == 0){
    chSysUnlock();
    goto BEGIN;
  }

After exiting the while loop the flag is already no more zero, why you test it again? the RM does not requires this extra check nor the ST library does that.

Code: Select all

  /* All interrupts initially disabled.*/
  if (RTC->CRH != 0){
    rtc_lld_acquire();
    RTC->CRH = 0;
    rtc_lld_release();

Why test if it is different to zero before setting it to zero? it looks redundant.

Code: Select all

    RTC->CRL &= ~(RTC_CRL_OWF | RTC_CRL_ALRF | RTC_CRL_SECF);
    RTC->CRH &= ~(RTC_CRH_OWIE | RTC_CRH_ALRIE | RTC_CRH_SECIE);

Why not just write zero like it was in the previous implementation? CRH contains only those flags, about CRL I am not sure.

There is also another thing that worries me, from the RM:

In addition, writing to any RTC register is only enabled if the previous write operation is
finished. To enable the software to detect this situation, the RTOFF status bit is provided in
the RTC_CR register to indicate that an update of the registers is in progress. A new value
can be written to the RTC registers only when the RTOFF status bit value is ’1’.

Apparently the wait operation is required each time any register is accessed, in the driver we don't do this (and the ST library doesn't do that as well...), in many places we write several registers in sequence.

I have way too many ??????? in by head about this driver.

Giovanni