Add threading implementation for the ESP32 #69

danicampora · 2017-04-08T21:39:21Z

This makes the ESP32 port pass all the threading tests, except for stress_aes.py which requires too much heap.

There's one change I'm not sure of: danicampora@d61fdf4

@dpgeorge is that assert still needed?

dpgeorge · 2017-04-09T01:24:04Z

extmod/moduselect.c

        MICROPY_EVENT_POLL_HOOK
+        MP_THREAD_GIL_ENTER();


Did you try putting the GIL exit/enter calls within the EVENT_POLL_HOOK macro itself? That's how stmhal's threading does it (see EVENT_POLL_HOOK in stmhal/mpconfigport.h).

dpgeorge · 2017-04-09T01:26:42Z

py/gc.c

-                    #if MICROPY_PY_THREAD
-                    // TODO need to think about reentrancy with finaliser code
-                    assert(!"finaliser with threading not implemented");
-                    #endif


This needs some thought. It has the same isses that wipy 1.0 originally had with interrupts and threading: when the GC runs it has the GIL, and so if a finaliser runs it also has the GIL. Then in the VM the finaliser may release the GIL which is not good because the GC is in the middle of running.

OK... I'll leave that one to you. So far we don't allow any Python code to run within interrupts context, so we should be fine.

dpgeorge · 2017-04-09T01:29:01Z

tests/thread/stress_create.py

@@ -15,7 +15,8 @@ def thread_entry(n):
        _thread.start_new_thread(thread_entry, (thread_num,))
        thread_num += 1
    except MemoryError:
-        pass
+        # let other threads run
+        time.sleep(0.05)


This shouldn't be necessary. I think the reason you originally needed it on your esp32 port was because in certain cases the VM would never release the GIL. This could happen if there was a short, infinite loop that had a try/except block which always raised an exception. This would reset the GIL divisor back to its maximum and therefore the counter would never reach zero and never release the GIL. This is fixed in the latest master.

Ummm, I have tried in several different ways, and without the time.sleep() line, it always gets stuck raising the memory error interrupt.

dpgeorge · 2017-04-09T01:30:29Z

esp32/mpthreadport.c

+                // move the start pointer
+                thread = th->next;
+            }
+            // explicitely release all its memory


"explicitly" (no second "e")

dpgeorge · 2017-04-09T01:34:16Z

esp32/mphalport.c

 }

 void mp_hal_delay_us(uint32_t us) {
+    MP_THREAD_GIL_EXIT();


For efficiency and accuracy I would not release the GIL during a microsecond sleep.

dpgeorge · 2017-04-09T01:34:44Z

esp32/mphalport.c

@@ -95,6 +103,7 @@ void mp_hal_delay_ms(uint32_t ms) {
    struct timeval tv_start;
    struct timeval tv_end;
    uint64_t dt;
+    MP_THREAD_GIL_EXIT();


If EVENT_POLL_HOOK does the release then this is not necessary.

dpgeorge · 2017-04-09T01:36:12Z

esp32/mphalport.c

@@ -44,12 +44,14 @@ ringbuf_t stdin_ringbuf = {stdin_ringbuf_array, sizeof(stdin_ringbuf_array)};

 int mp_hal_stdin_rx_chr(void) {
    for (;;) {
+        MP_THREAD_GIL_EXIT();


This is not needed if EVENT_POLL_HOOK does the release.

…ad ready.

@dpgeorge

@dpgeorge please check this.

… will block.

Let other threads run while the select operation is blocking.

…run. We need this on the FreeRTOS ports to allow tasks with the same priority to share the cpu.

…eset.

danicampora · 2017-04-10T07:45:00Z

@dpgeorge I worked on your remarks (except 2 of them that need more thought). I also added an extra test to verify that threads are killed during a soft reset.

dpgeorge · 2017-04-10T07:55:15Z

@danicampora thanks! I will test the code and let you know about the 2 outstanding points (finalisers and the mod to the existing test).

dpgeorge · 2017-04-11T02:38:20Z

I tested the patch and it works pretty well. Some issues:

The new thread_deinit.py test makes some subsequent tests crash (when running lots of tests at once). I don't think it's a good idea to have a test like this leave threads running after it exits. Note that the raise SystemExit line in that test does not do a soft reset, it just exits the script and drops to the REPL, so the threads are still running in the background. machine.soft_reset() could be used instead, but that function is not yet standard across all ports. So maybe this test can be left as an "integration" test that is run offline / separately, at least until we work out a way to incorporate it into the test suite.
The stress_create.py test does indeed fail without the time.sleep(0.05) line. The reason is that in FreeRTOS, deleted threads are cleaned up by the idle task and the idle task runs at the lowest priority. So if there is a thread that is doing anything except sleeping then the idle task will never run and thread memory will never be reclaimed. This will be a general problem in real programs that can't be solved simply by putting a time.sleep() somewhere (since all active threads would need to sleep at exactly the same time). To quote http://www.freertos.org/RTOS-idle-task.html:

The idle task is responsible for freeing memory allocated by the RTOS to tasks that have since been deleted. It is therefore important in applications that make use of the vTaskDelete() function to ensure the idle task is not starved of processing time. The idle task has no other active functions so can legitimately be starved of microcontroller time under all other conditions.

This seems to be an inherent limitation in the design of FreeRTOS and I see only one way to fix it: make all MicroPython tasks run at the idle priority (the lowest priority). This will probably affect overall performance.

dpgeorge · 2017-04-11T02:42:13Z

@danicampora to make progress here I suggest to merge the additions/changes to esp32/ and leave out the other things (tests, py changes). The latter are then things to fix at a later date (well, soon, but not right now).

danicampora · 2017-04-11T07:52:13Z

The new thread_deinit.py test makes some subsequent tests crash (when running lots of tests at once).

Umm OK. I tested this myself running several tests at once and it seemed to be fine. But you are right, we should use machine.soft_reset() instead.

@danicampora to make progress here I suggest to merge the additions/changes to esp32/ and leave out the other things (tests, py changes). The latter are then things to fix at a later date (well, soon, but not right now).

OK, sounds good.

danicampora · 2017-04-11T07:53:01Z

@dpgeorge please merge it as you suggest. Thanks!

dpgeorge · 2017-04-12T04:00:39Z

Ok, this was merged in dc82def, f704375 and ff0ca1e

Then I updated upstream to fix the thread+finaliser issue and merged that, see c7e8c6f

The test for soft reset was not merged (see reason above).

The one outstanding issue is that tests/thread/stress_create.py does not work because FreeRTOS does not reclaim deleted threads unless all remaining threads are idle.

danicampora · 2017-04-12T08:02:11Z

@dpgeorge awesome, thanks!

esp32: Add threading implementation, disabled for the time being.

f0dc4bf

danicampora force-pushed the threading branch 5 times, most recently from ce7e3f6 to 422f413 Compare April 8, 2017 22:21

dpgeorge reviewed Apr 9, 2017

View reviewed changes

Daniel Campora added 6 commits April 9, 2017 11:28

esp32: Trace the registers when doing a gc collect. Also make it thre…

53f6a7b

…ad ready.

py: Remove the assert related to threading and finalizers.

d576d8e

@dpgeorge please check this.

esp32: Enable threading. Make sure to exit the GIL everytime a thread…

2f024c4

… will block.

extmod/moduselect.c: Exit and enter the GIL on every select loop.

da3ce31

Let other threads run while the select operation is blocking.

tests/thread/stress_create.py: Add a delay to allow other threads to …

bf26d88

…run. We need this on the FreeRTOS ports to allow tasks with the same priority to share the cpu.

esp32: Exit and enter the GIL inside MICROPY_EVENT_POLL_HOOK.

b1aac37

danicampora force-pushed the threading branch from 422f413 to b1aac37 Compare April 9, 2017 09:28

Daniel Campora added 2 commits April 9, 2017 11:29

esp32/mpthreadport.c: Fix typo.

4089b91

tests/thread: Add test to verify _thread module de-init during soft r…

55753e0

…eset.

dpgeorge closed this Apr 12, 2017

Add threading implementation for the ESP32 #69

Add threading implementation for the ESP32 #69

Uh oh!

Conversation

danicampora commented Apr 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danicampora Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danicampora commented Apr 10, 2017

Uh oh!

dpgeorge commented Apr 10, 2017

Uh oh!

dpgeorge commented Apr 11, 2017

Uh oh!

dpgeorge commented Apr 11, 2017

Uh oh!

danicampora commented Apr 11, 2017

Uh oh!

danicampora commented Apr 11, 2017

Uh oh!

dpgeorge commented Apr 12, 2017

Uh oh!

danicampora commented Apr 12, 2017

Uh oh!

Uh oh!

danicampora commented Apr 8, 2017 •

edited

Loading

danicampora Apr 9, 2017 •

edited

Loading