RTCv1 + GCC7: rtc_decode() produces random output Topic is solved

[Thargon]

Hi,

the RTCv1 driver produces random output when reading the current time for binaries compiled with GCC7.

I am running an STM32F103, so the RTCv1 LLD applies.
The compiler I use is GCC7: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
I have enabled GCC option -std=c11, but the issue persists when compiling with -std=c99.

When calling rtcGetTime() it will again call rtc_lld_get_time(), which is specific to the LLD.
The latter then first calls rtcSTM32GetSecMsec() which reads the actual register values. I have checked those and this function seems to work fine.
rtc_lld_get_time() furthermore calls rtc_decode(), and here things start to get strange. rtc_decode() relies on localtime_r() to convert the tv_sec variable (a uint32_t holding the value of the RTC CNT registers) into a struct tm value tim. The result, however, seems to be quite random, so that the content of tim is unusable.
However, when I revert back to an older version of GCC (arm-none-eabi-gcc (GNU Tools for ARM Embedded Processors) 6.2.1 20161205 (release) [ARM/embedded-6-branch revision 243739]), everything works as expected.

I see two things that could be the cause of this issue: First, the pointer to tv_sec is casted to a time_t*, but since time_t is not well defined by the C standard, this cast might have become invalid for GCC7. Second, localtime_r() might fail internally, which could be caused by a GCC bug or configuration issues. Obviously, the former guess is the more probable one
Anyway, I don't see the reason, why to call localtime_r() at all, especially since the time_t type involved is not well specified. As long as the origin date (e.g. 01-01-1980 like for the RTCDateTime type) is specified, the conversion could be easily done manually.

I would gladly commit a patch, if the issue can be confirmed by others!

Best regards,
Thomas

[Giovanni]

Hi,

It could be caused by a C library change more than a compiler change, anyway, it would be great if you could analyse the issue, RTC needs also another change, alarm callback handling so it could be a good time rework it.

Giovanni

[Thargon]

Hi,

I investigated a little and could fix the issue by only adding a few lines to hat_rtc_lld.c. I only had to slightly modify the rtc_decode() function (see below).

The issue was probably caused by the minor detail, that since C11 time_t is not defined to be an arithmetic type, but a real type. As a result, casts of pointer as it was done in RTCv1 are illegal with C11:

Code: Select all

// file: RTCv1/hal_rtc_lld.c

static void rtc_decode(uint32_t tv_sec,
                       uint32_t tv_msec,
                       RTCDateTime *timespec) {
  struct tm tim;
  struct tm *t;

  /* If the conversion is successful the function returns a pointer
     to the object the result was written into.*/
#if defined(__GNUC__) || defined(__CC_ARM)
  t = localtime_r((time_t *)&(tv_sec), &tim);    // FAIL: illegal cast from arithmetic to real
  osalDbgAssert(t != NULL, "conversion failed");
#else
  t = localtime(&tv_sec);                        // FAIL: illegal cast from arithmetic to real
  memcpy(&tim, t, sizeof(struct tm));
#endif

  rtcConvertStructTmToDateTime(&tim, tv_msec, timespec);
}

The issue can be easily fixed, though. Instead of propagating the pointer to the function argument (which is of arithmetic type uint32_t) to the localtime functions, I introduced an additional variable of type_t and let the compiler handle the type conversion:

Code: Select all

// file: RTCv1/hal_rtc_lld.c

static void rtc_decode(uint32_t tv_sec,
                       uint32_t tv_msec,
                       RTCDateTime *timespec) {
  struct tm tim;
  struct tm *t;
  const time_t time = tv_sec;                    // copy with implicit type conversion

  /* If the conversion is successful the function returns a pointer
     to the object the result was written into.*/
#if defined(__GNUC__) || defined(__CC_ARM)
  t = localtime_r(&time, &tim);
  osalDbgAssert(t != NULL, "conversion failed");
#else
  t = localtime(&time);
  memcpy(&tim, t, sizeof(struct tm));
#endif

  rtcConvertStructTmToDateTime(&tim, tv_msec, timespec);
}

Since my solution requires slightly more memory, I leave the decision to you whether you want to keep the old, more efficient code as well by using a switch, like

Code: Select all

#if (__STDC_VERSION__ >= 201112L)
/* C11 compatible code */
#else
/* more efficient code */
#endif


Best regards,
Thomas

[Giovanni]

I would not go down to checking for compiler versions, thanks for the patch, I will integrate it.

Giovanni

[Giovanni]

Hi,

Fixed as bug #964.

Giovanni

[XXzzz]

i also have this question.and then i fix the source throught time_t.but when i get time .the first time is right,and then it return some random number. then i found something wrong in

Code: Select all

rtcConvertStructTmToDateTime(&tim, tv_msec, timespec);


when i comment it and use struct tm. i got the right time.

Code: Select all

int rtc_get_time(struct tm *time_now)
{

    uint32_t tv_sec, tv_msec;

    rtcSTM32GetSecMsec(&RTCD1, &tv_sec, &tv_msec);

    struct tm tim;
    struct tm *t;
    const time_t time = tv_sec; // copy with implicit type conversion

    /* If the conversion is successful the function returns a pointer
     to the object the result was written into.*/
    t = localtime_r(&time, &tim);
    osalDbgAssert(t != NULL, "conversion failed");

    memcpy(time_now, &tim, sizeof(struct tm));
}


i use the chibios 172 .toolchain:arm-none-eabi-gcc 7.0