Library converts datetime_t incorrectly, causing example to fail

maxgerhardt commented 3 years ago

Describe the bug

This library does

https://github.com/khoih-prog/RP2040_RTC/blob/f7650a1890b2e67d4bd76f927a5cd784fe1ff440/src/DateTime_Generic.h#L456-L477

to convert a DateTime into datetime_t as needed by the Pico SDK's rtc_set_datetime function. However, the library fails to initialize the tm.dotw (day-of-the-week) member, and as such the field will have a random value from the stack.

Calling into rtc_set_datetime with such a datastructure leads to the validation function failing

static bool valid_datetime(datetime_t *t) {
    // Valid ranges taken from RTC doc. Note when setting an RTC alarm
    // these values are allowed to be -1 to say "don't match this value"
    if (!(t->year >= 0 && t->year <= 4095)) return false;
    if (!(t->month >= 1 && t->month <= 12)) return false;
    if (!(t->day >= 1 && t->day <= 31)) return false;
    if (!(t->dotw >= 0 && t->dotw <= 6)) return false;
    if (!(t->hour >= 0 && t->hour <= 23)) return false;
    if (!(t->min >= 0 && t->min <= 59)) return false;
    if (!(t->sec >= 0 && t->sec <= 59)) return false;
    return true;
}

bool rtc_set_datetime(datetime_t *t) {
    if (!valid_datetime(t)) {
        return false;
    }
    // rest of code
}

Since if (!(t->dotw >= 0 && t->dotw <= 6)) return false; triggers. (Since variable is allocated on the stack it has an initial random value, 0-255, only by chance if the value is 0-6 it passes this check)

Hence, the example Time with NiNa (https://github.com/khoih-prog/RP2040_RTC/tree/main/examples/Time/RP2040_RTC_Time_WiFiNINA) module that uses this codepath fails randomly.

13:04:31.792 -> Start RP2040_RTC_Time_WiFiNINA on MBED NANO_RP2040_CONNECT with WiFiNINA using WiFiNINA_Generic Library
13:04:31.792 -> RP2040_RTC v1.0.6
13:04:32.515 -> Please upgrade the firmware
13:04:32.515 -> Connecting to WPA SSID: xxx
13:04:35.829 -> You're connected to the network, IP = xxxxx
13:04:36.829 -> Packet received
13:04:36.829 -> Seconds since Jan 1 1900 = 3844757075
13:04:36.829 -> Unix time = 1635768275
13:04:36.829 -> rtc_set_datetime failed
13:04:37.343 -> rtc_set_datetime failed [repeated multiple times until the loop ends]
13:04:41.835 -> The UTC time is 12:04:35
13:04:41.835 -> ============================
13:04:41.835 -> 00:00:00 Tue 31 Dec 2047 UTC
13:05:42.250 -> ============================

And thus the example does not run correctly.

Steps to Reproduce

Use the https://github.com/khoih-prog/RP2040_RTC/tree/main/examples/Time/RP2040_RTC_Time_WiFiNINA example with the platformio.ini

[env:nanorp2040connect]
platform = raspberrypi
board = nanorp2040connect
framework = arduino
lib_ldf_mode = chain+
lib_compat_mode = strict
lib_deps =
   paulstoffregen/Time@^1.6.1
   khoih-prog/WiFiNINA_Generic@^1.8.13
   khoih-prog/RP2040_RTC@^1.0.6
   khoih-prog/Timezone_Generic@^1.7.1
lib_ignore =
   WiFi101
   WiFiEspAT
   WiFi
   ESP_AT_Lib
   ESP8266_AT_WebServer
   EthernetENC
   Ethernet2
   Ethernet3
   EthernetLarge
   Ethernet
   WiFiWebServer
   EthernetWebServer

Expected behavior

The library converts to datetime_t correctly without leaving any datamember uninitialized.

Actual behavior

Library does not initialize the .dotw member causing a probabilistic failure of rtc_set_datetime().

Debug and AT-command log (if applicable)

None appliciable.

Screenshots

None appliciable.

Information

Please ensure to specify the following:

PlatformIO core 5.2.2
RP2040 Core Version: Arduino-mbed RP2040 v2.5.2
RP2040 Board type: NANO_RP2040_CONNECT
Contextual information (e.g. what you were trying to achieve): Run the NTP synchronization example
Simplest possible steps to reproduce
Anything that might be relevant in your opinion, such as:
- Operating system: Windows 10

Additional context

Discussed in the PlatformIO forum thread.

khoih-prog commented 3 years ago

Thank for the bug report.

The fix will be as follows and will be introduced in the next release

  bool rtc_set_datetime(DateTime dt) 
  {
    datetime_t tm;

    tm.year   = dt.year();       
    tm.month  = dt.month();
    tm.day    = dt.day();
    tm.hour   = dt.hour();
    tm.min    = dt.minute();
    tm.sec    = dt.second();

    tm.dotw   = dt.dayOfTheWeek();

#if RTC_DEBUG    
    Serial.print("Year = "); Serial.print(tm.year);
    Serial.print(", Mo = "); Serial.print(tm.month);
    Serial.print(", day = "); Serial.print(tm.day);
    Serial.print(", hour = "); Serial.print(tm.hour);
    Serial.print(", min = "); Serial.print(tm.min);
    Serial.print(", sec = "); Serial.println(tm.sec);
#endif   

    return (rtc_set_datetime(&tm));
  }

The Terminal output with the new bug fix

Start RP2040_RTC_Time_WiFiNINA on MBED NANO_RP2040_CONNECT with WiFiNINA using WiFiNINA_Generic Library
RP2040_RTC v1.0.6
Timezone_Generic v1.7.1
Connecting to WPA SSID: HueNet1
You're connected to the network, IP = 192.168.2.87
Packet received
Seconds since Jan 1 1900 = 3844779246
Unix time = 1635790446
The UTC time is 18:14:06
============================
18:14:07 Mon 01 Nov 2021 UTC
14:14:07 Mon 01 Nov 2021 EDT
============================
18:15:07 Mon 01 Nov 2021 UTC
14:15:07 Mon 01 Nov 2021 EDT
============================
18:16:07 Mon 01 Nov 2021 UTC
14:16:07 Mon 01 Nov 2021 EDT
============================
18:17:07 Mon 01 Nov 2021 UTC
14:17:07 Mon 01 Nov 2021 EDT

Could you please check and verify if there any more issue with PIO.

Thanks and regards

khoih-prog commented 3 years ago

Hi @maxgerhardt

The new RP2040_RTC releases v1.0.7 has just been published.

Your contribution has been noted in Contributions and Thanks

Releases v1.0.7

Fix bug in rtc_set_datetime(). Check Library converts datetime_t incorrectly, causing example to fail #4

khoih-prog / RP2040_RTC