tests/periph_rtc: fix system locks in ISR by gschorcht · Pull Request #13035 · RIOT-OS/RIOT

gschorcht · 2020-01-06T17:33:04Z

Contribution description

This PR fixes a system lock that occurs when the newlib with real locking functionality is used instead of the newlib_nano.

tests/periph calls the functions rtc_get_alarm and rtc_set_alarm from an ISR. Depending on the implementation of periph_rtc, these functions may use newlib functions such as localtime and mktime, which in turn call the newlib lock function __tz_lock. This can lead to a system lock if the newlib with real lock functionality is used instead of the newlib_nano.

This PR moves the call of the rtc_get_alarm and rtc_set_alarm functions from interrupt context to the thread context of the main thread. The ISR just sends a message to the main thread to inform the about the interrupt. The interrupt is then handled in thread context.

The problem was figured out while testing PR #12955.

Testing procedure

The PR has to be tested together with PR #12955.

make BOARD=esp32-wroom-32 -C tests/periph_rtc flash test

In PR #12955, tests/periph_rtc blocks without this PR, but works with this PR.

Issues/PRs references

Fixes partially issue #12732
Prerequisite for PR #12955

benpicco · 2020-01-11T15:51:01Z

This also makes the code a better example as not so much stuff is happening in the interrupt.
But in that vein, you can make it slightly simpler by using a mutex instead of a message.

Just call mutex_lock() in the loop and mutex_unlock() in the callback - no need for global variables, you can just pass the mutex as RTC Callback argument.

gschorcht · 2020-01-12T04:51:14Z

But in that vein, you can make it slightly simpler by using a mutex instead of a message.

Just call mutex_lock() in the loop and mutex_unlock() in the callback - no need for global variables, you can just pass the mutex as RTC Callback argument.

Hm, even though mutex_unlock should also work in ISRs, I'm a bit careful and avoid the use of mutexes in ISR at all. Furthermore, msg_send/msg_recv IPC is the standard mechanism used in RIOT to trigger threads from an ISR, see tests/driver_css811, tests/driver_ltc4150, tests_driver_sds011, and all netdev drivers.

Therefore, I would prefer to keep this mechanism. The global variable can be avoided in same way as with mutex. I pushed a small fixup.

gschorcht · 2020-01-26T10:02:29Z

Hm, even though mutex_unlock should also work in ISRs, I'm a bit careful and avoid the use of mutexes in ISR at all. Furthermore, msg_send/msg_recv IPC is the standard mechanism used in RIOT to trigger threads from an ISR, see tests/driver_css811, tests/driver_ltc4150, tests_driver_sds011, and all netdev drivers.

Therefore, I would prefer to keep this mechanism. The global variable can be avoided in same way as with mutex. I pushed a small fixup.

@benpicco Is it okay for you to keep the mutex?

benpicco · 2020-01-26T15:09:42Z

Sorry I forgot about this PR.
I think sending a message is a bit more convoluted than just unlocking a mutex, but I won't block because of that - it's a good improvement.
Please squash.

gschorcht · 2020-01-26T16:07:31Z

Sorry I forgot about this PR.

No problem, I'm usually pinging after some time?

gschorcht · 2020-01-26T17:03:24Z

Squashed.

benpicco · 2020-01-26T17:17:25Z

Hm, with this patch the test stops working on mcb2388 - I'll investigate that.

gschorcht · 2020-01-26T17:33:41Z

Really? Strange.

smlng

try to add a msg queue to the main, maybe that helps to make it work on the failing platforms

smlng · 2020-01-27T07:44:15Z

tests/periph_rtc/main.c

@@ -54,15 +56,10 @@ static void inc_secs(struct tm *time, unsigned val)
 static void cb(void *arg)
 {
    (void)arg;


not needed, arg is used in msg_try_send

smlng · 2020-01-27T07:44:36Z

tests/periph_rtc/main.c

-        inc_secs(&time, PERIOD);
-        rtc_set_alarm(&time, cb, NULL);
-    }
+    memset(&msg, 1, sizeof(msg_t));


why? I don't think that's necessary.

benpicco · 2020-01-29T18:53:56Z

OK, this is such a silly bug: The RTC callback argument was just never set on lpc2387…

Still, I think the mutex solution is much simpler:

diff --git a/tests/periph_rtc/main.c b/tests/periph_rtc/main.c
index 4cb3e6e86..7ec6120f2 100644
--- a/tests/periph_rtc/main.c
+++ b/tests/periph_rtc/main.c
@@ -53,16 +53,7 @@ static void inc_secs(struct tm *time, unsigned val)
 
 static void cb(void *arg)
 {
-    (void)arg;
-
-    puts("Alarm!");
-
-    if (++cnt < REPEAT) {
-        struct tm time;
-        rtc_get_alarm(&time);
-        inc_secs(&time, PERIOD);
-        rtc_set_alarm(&time, cb, NULL);
-    }
+    mutex_unlock(arg);
 }
 
 int main(void)
@@ -76,6 +67,8 @@ int main(void)
         .tm_sec  = 57
     };
 
+    mutex_t rtc_mutex = MUTEX_INIT_LOCKED;
+
     puts("\nRIOT RTC low-level driver test");
     printf("This test will display 'Alarm!' every %u seconds for %u times\n",
             PERIOD, REPEAT);
@@ -91,12 +84,25 @@ int main(void)
     /* set initial alarm */
     inc_secs(&time, PERIOD);
     print_time("  Setting alarm to ", &time);
-    rtc_set_alarm(&time, cb, NULL);
+    rtc_set_alarm(&time, cb, &rtc_mutex);
 
     /* verify alarm */
     rtc_get_alarm(&time);
     print_time("   Alarm is set to ", &time);
     puts("");
 
+    while (1) {
+
+        mutex_lock(&rtc_mutex);
+        puts("Alarm!");
+
+        if (++cnt < REPEAT) {
+            struct tm time;
+            rtc_get_alarm(&time);
+            inc_secs(&time, PERIOD);
+            rtc_set_alarm(&time, cb, &rtc_mutex);
+        }
+    }
+
     return 0;
 }

It also saves 688 bytes of ROM on samr21-xpro compared to the message based solution.
I know it's not very relevant for an test case, but it still servers as an example for users.

gschorcht · 2020-02-01T23:06:55Z

I'm still a little reluctant to use the mutex API in an ISR even if an unlock is not blocking. How about using a thread flag as a synchronization mechanism.

benpicco · 2020-02-02T10:06:43Z

I'm still a little reluctant to use the mutex API in an ISR even if an unlock is not blocking.

But why? This is a common pattern.

gschorcht · 2020-02-02T11:16:15Z

I'm still a little reluctant to use the mutex API in an ISR even if an unlock is not blocking.

But why? This is a common pattern.

Hm, even in RIOT? In FreeRTOS there are special functions xSemaphoreTakeFromISR and xSemaphoreGiveFromISR which are obviously callable from an ISR.

If we start using mutex_unlock in an example for an ISR, a user might think that he could also call mutex_lock inside an ISR. Even if mutex_lock is documented as blocking, the documentation could be improved by providing a clear warning that mutex_lock must not be called from within an ISR.

benpicco · 2020-02-02T12:10:40Z

Hm, even in RIOT?

Yes even in RIOT. (picked those at random)

If we start using mutex_unlock in an example for an ISR, a user might think that he could also call mutex_lock inside an ISR.

Not necessarily if they think about it for a bit.

the documentation could be improved by providing a clear warning that mutex_lock must not be called from within an ISR.

Yea that's probably a good idea.

gschorcht · 2020-02-02T12:13:52Z

Ok I pushed the changes, let's see whether Murdock is happy. BTW, this was the only test that failed in PR #12955. If this PR is merged, Murock should be happy also with PR #12955.

gschorcht · 2020-02-02T12:19:34Z

Hm, even in RIOT?

Yes even in RIOT. (picked those at random)

Ok, that's good to know.

If we start using mutex_unlock in an example for an ISR, a user might think that he could also call mutex_lock inside an ISR.

Not necessarily if they think about it for a bit.

I already ran into problems with a driver (I don't remember which one) that tried to read the sensor data via I2C from an ISR. The problem was that I2C uses of course a mutex to coordinate the acccess.

I can provide a PR with a warning in mutex documentation.

benpicco · 2020-02-02T13:00:08Z

I already ran into problems with a driver (I don't remember which one) that tried to read the sensor data via I2C from an ISR. The problem was that I2C uses of course a mutex to coordinate the acccess.

Yea but that issue there is that it's not obvious that I2C and SPI operations involve acquiring a mutex.
We should probably also add a warning there and add an assert() to mutex_lock() that checks that it isn't being called in interrupt context.

I can provide a PR with a warning in mutex documentation.

That'd be great!

benpicco

Much simpler now :)
Please squash.

benpicco · 2020-02-02T13:01:55Z

tests/periph_rtc/main.c

-        inc_secs(&time, PERIOD);
-        rtc_set_alarm(&time, cb, NULL);
-    }
+    mutex_unlock((mutex_t *)arg);


You don't need to cast the void*

Changed. I squashed it directly, I hope that's ok for this small change.

`tests/periph` calls the functions `rtc_get_alarm` and `rtc_set_alarm` from an ISR. Depending on the implementation of `periph_rtc`, however, these functions use `newlib` functions such as `localtime` and `mktime`, which in turn call the `newlib` lock function `__tz_lock`. This can lead to a system lock if the `newlib` with real lock functionality is used instead of the `newlib_nano`.

gschorcht · 2020-02-02T14:30:33Z

We should probably also add a warning there and add an assert() to mutex_lock()

That was suggested in any discussion about mutex and ISRs in the past (I couldn't find it again).

assert does not work in ISRs on platforms that implement newlibs' locking mechanism using mutexes, because assert uses printf, which in turn uses a file lock for stdout, which in turn tries to lock a mutex. Then we have exactly the same problem we tried to fix with this PR.

gschorcht · 2020-02-02T14:51:22Z

Ok, all lights are green.

gschorcht mentioned this pull request Jan 6, 2020

cpu/esp*: code deduplication and cleanup #12955

Merged

gschorcht force-pushed the tests/periph_rtc_fix branch from c503534 to bc6457e Compare January 26, 2020 16:06

smlng reviewed Jan 27, 2020

View reviewed changes

benpicco mentioned this pull request Jan 29, 2020

cpu/lpc2387: rtc: set rtc callback arg #13234

Merged

benpicco mentioned this pull request Jan 29, 2020

cpu/lpc2387: rtc: set rtc callback arg [backport 2020.01] #13235

Merged

benpicco approved these changes Feb 2, 2020

View reviewed changes

benpicco reviewed Feb 2, 2020

View reviewed changes

gschorcht force-pushed the tests/periph_rtc_fix branch from d839083 to c01768b Compare February 2, 2020 14:02

gschorcht force-pushed the tests/periph_rtc_fix branch from c01768b to 46dbd86 Compare February 2, 2020 14:04

benpicco merged commit f760625 into RIOT-OS:master Feb 2, 2020

leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020

gschorcht deleted the tests/periph_rtc_fix branch July 31, 2025 08:06

Conversation

gschorcht commented Jan 6, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Jan 11, 2020

Uh oh!

gschorcht commented Jan 12, 2020

Uh oh!

gschorcht commented Jan 26, 2020

Uh oh!

benpicco commented Jan 26, 2020

Uh oh!

gschorcht commented Jan 26, 2020

Uh oh!

gschorcht commented Jan 26, 2020

Uh oh!

benpicco commented Jan 26, 2020

Uh oh!

gschorcht commented Jan 26, 2020

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

smlng Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

smlng Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco commented Jan 29, 2020

Uh oh!

gschorcht commented Feb 1, 2020

Uh oh!

benpicco commented Feb 2, 2020

Uh oh!

gschorcht commented Feb 2, 2020

Uh oh!

benpicco commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gschorcht commented Feb 2, 2020

Uh oh!

gschorcht commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco Feb 2, 2020

Choose a reason for hiding this comment

Uh oh!

gschorcht Feb 2, 2020

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Feb 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gschorcht commented Feb 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benpicco commented Feb 2, 2020 •

edited

Loading

gschorcht commented Feb 2, 2020 •

edited

Loading

benpicco commented Feb 2, 2020 •

edited

Loading

gschorcht commented Feb 2, 2020 •

edited

Loading