Platform support for Embedded targets #106

lhstrh · 2022-09-03T00:28:10Z

This PR creates runtime support for (bare-iron) single-threaded targets with interrupts.

Polish existing NRF52 support
- Move logic from wait_until to lf_sleep_until
- Factor lf_sleep_until into smaller functions
Settle on API changes
- Move LF_MIN_SLEEP_DURATION to platform support
- Remove ARDUINO directive and add NO_TTY or something along those lines
- ...
Merge changes into lock-time
Merge main into lock-time
General cleanup, comments, and removal of spurious .c-file includes

Fixes #109
Companion of lf-lang/lingua-franca#1348.

…mon.c but still has linking problems

…duino

edwardalee

The strategy you have followed here relies on the compiler to detect physical actions and set NUMBER_WORKERS, choosing the threaded runtime. This is why the functions in lf_os_single_threaded_support.c can be empty. However, this has a downside: It means the mutex mechanism of the unthreaded runtime is essentially untested. In fact, I have my doubts it is correct. The mutex is held only for very short times compared to the threaded runtime. Seems like there is real risk of lurking race conditions(?).

Only the Arduino is using this, and that doesn't look correct to me because _lf_timer_interrupted seems to never get reset. Do we have any way to test the Arduino implementation?

What I had in mind was to actually allow physical actions with the unthreaded runtime. This means the mutex acquisition and release should closely mimic (or exactly match) what reactor_threaded.c does. This also means we could have regression tests that have physical actions with the unthreaded runtime running on all the platforms.

core/platform/lf_linux_support.c

core/platform/lf_os_single_threaded_support.c

core/platform/lf_arduino_support.c

core/reactor.c

…ake sure we advance to the correct tag

edwardalee · 2022-12-11T14:01:51Z

It's been a while since I looked at this code, but I think the new API for entering a critical section will take care of a big part of the problem, which is that tracing calls write to a common data structure. Alternatively, this could be refactored to have a separate data structure per worker. I think the reason for needing to create a thread is to write to a file when the data structure fills up. I guess this could be handled differently in the unthreaded runtime with conditional code.

erlingrj · 2022-12-11T21:16:51Z

It's been a while since I looked at this code, but I think the new API for entering a critical section will take care of a big part of the problem, which is that tracing calls write to a common data structure. Alternatively, this could be refactored to have a separate data structure per worker. I think the reason for needing to create a thread is to write to a file when the data structure fills up. I guess this could be handled differently in the unthreaded runtime with conditional code.

OK, so you are saying that if we are using the unthreaded runtime with tracing enabled, critical_section_enter() should result in acquiring a global mutex, which is also acquired by the tracing thread? This is not implemented as of now. Actually, in the unthreaded runtime, critical_section_enter() does nothing. I guess it should at least disable interrupts.

I don't see the place in trace.c where we are accessing a data structure shared with the runtime. I.e. where is trace.c calling critical_section_enter()

erlingrj · 2022-12-15T02:36:37Z

@lhstrh. After a converation with @edwardalee about tracing today I have concluded that the current implementation is sufficient. The tracing only needs to synchronize access "internally". This is done with a mutex local to tracing.c. It seemed to be an edge case when we had unthreaded+tracing. But I don't think it matters. How it looks is like this:

From all the source files including plaform.h EXCEPT tracing.c will see the unthreaded API and its implementation of critical section etc. For the single source file tracing.c it will see the threaded API (and its implementation, the function definitions are in the header files as well). So when we do some tracing the program will call into tracing.c and during that period it will get access to the threaded API and can synchronize with the flushing thread.

I will do a pass through the PR and address Edwards comments and fix what I see, then I guess we could do a final review and merge?

Co-authored-by: Edward A. Lee <eal@eecs.berkeley.edu>

… lock-time

erlingrj · 2022-12-15T02:33:34Z

core/platform/lf_arduino_support.c

+
+/**
+ * @brief Sleep until an absolute time.
+ * FIXME: For improved power consumption this should be implemented with a HW timer and interrupts.


Yes, sleep until is for internal use only and should only be called from a critical section, while advancing time. Maybe the API should be: _lf_sleep_until_locked?

erlingrj · 2022-12-15T02:34:22Z

core/platform/lf_arduino_support.c

+    return 0;
+}
+
+int lf_critical_section_enter() {


Thanks for suggestion will add this!

erlingrj · 2022-12-15T06:45:15Z

core/platform/lf_nrf52_support.c

+ *  set appropriately (see `man 2 clock_gettime`).
+ */
+int lf_clock_gettime(instant_t* t) {
+    if (t == NULL) {


core/reactor.c

include/core/platform.h

lhstrh

Left some comments...

core/platform/lf_arduino_support.c

include/core/platform.h

core/platform/lf_arduino_support.c

core/platform/lf_nrf52_support.c

core/threaded/reactor_threaded.c

lhstrh · 2022-12-16T23:05:16Z

core/trace.c

@@ -29,14 +29,19 @@ THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 * Include this file instead of trace.h to get tracing.
 * See trace.h file for instructions.
 */
-
+// FIXME: Should be renamed if we use the hack below maybe: LF_INCLUDE_TRACE_FILES


Also this FIXME should not be left unaddressed.

Yes. My suggestion is to call it LF_TRACE, and the macro below can be called _LF_TRACE. The second macro should never be messed with by the user and is only for getting the threaded API from platform.h even if the unthreaded runtime is used

lhstrh · 2022-12-16T23:06:16Z

include/core/platform.h

@@ -42,8 +42,20 @@ THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef PLATFORM_H
 #define PLATFORM_H

-#if defined(ARDUINO)
+// FIXME: We could also use a single flag since this is truly binary


I will remove the fixme. I think it is okay to have 2 flags (THREADED and UNTHREADED)

Hm, that sounds confusing though. What if both are true?

core/trace.c

include/core/platform/lf_nrf52_support.h

Co-authored-by: Marten Lohstroh <marten@berkeley.edu>

edwardalee · 2022-12-21T01:06:22Z

What is the status of this lock-time PR?

lhstrh · 2022-12-21T01:14:27Z

It's almost ready, but there are still a few small things that need @erlingrj's attention (comments ☝️). Once addressed, we can merge.

erlingrj · 2022-12-19T17:11:13Z

include/core/platform.h

@@ -42,8 +42,20 @@ THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef PLATFORM_H
 #define PLATFORM_H

-#if defined(ARDUINO)
+// FIXME: We could also use a single flag since this is truly binary


I will remove the fixme. I think it is okay to have 2 flags (THREADED and UNTHREADED)

include/core/platform.h

core/trace.c

erlingrj · 2022-12-19T17:17:21Z

core/platform/lf_windows_support.c

 #else
-#include "lf_C11_threads_support.h"
-#endif
+#include "lf_os_single_threaded_support.c"


The Windows implementation is moved to its header-file (like it is for macOS and Linux). This makes it possible to have both the threaded and the unthreaded API present in the same binary.

core/threaded/reactor_threaded.c

erlingrj · 2022-12-19T17:21:52Z

core/trace.c

@@ -29,14 +29,19 @@ THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 * Include this file instead of trace.h to get tracing.
 * See trace.h file for instructions.
 */
-
+// FIXME: Should be renamed if we use the hack below maybe: LF_INCLUDE_TRACE_FILES


Yes. My suggestion is to call it LF_TRACE, and the macro below can be called _LF_TRACE. The second macro should never be messed with by the user and is only for getting the threaded API from platform.h even if the unthreaded runtime is used

core/trace.c

core/platform/lf_arduino_support.c

erlingrj · 2022-12-21T18:20:36Z

core/platform/lf_os_single_threaded_support.c

+#endif
+
+int lf_critical_section_enter() {
+    // FIXME: Is this what we want? Dont we want to grab a mutex here? 


I think the unthreaded runtime + Linux is intended to be a very special case where you should not use other threads to schedule physical actions. I think the macro above would catch such a scenario. I think we can close this for now. I will remove the fixme

Co-authored-by: Marten Lohstroh <marten@berkeley.edu>

edwardalee and others added 9 commits August 20, 2022 07:55

Added lock functions to use for unthreaded execution

15690bb

Moved comment to where it belongs

bdd3c1f

Renamed functions (see Zulip)

6726fc9

Work in progress toward moving critical section code into reactor_com…

6c7e6c6

…mon.c but still has linking problems

Add file I forgot to check in

b70632c

Get single-threaded build to work on Linux.

b683e61

Got to the point where Minimal compiles on Linux

f77809e

Moving more functions into reactor_common.c

60304ce

Treat the event queue as a critical section in reactor.c

cab3647

lhstrh added the feature New feature label Sep 3, 2022

lhstrh and others added 10 commits September 2, 2022 17:55

Silly workaround to get RTI to build again

3faaad8

Fix type error in POSIX thread support

23673f0

Added implementations of critical section code for single-threaded Ar…

2994d08

…duino

Attempt to fix compilation issue

ad619dd

Another fix

44663da

More tinkering with const keyword

bd20b2d

More tinkering with const keyword

d23bd40

Address more compilation issues

b2ddd98

Update signatures of implementations

922fe32

Debugging serialization tests

ac0a936

edwardalee reviewed Sep 3, 2022

View reviewed changes

Peter Donovan and others added 6 commits September 3, 2022 19:50

More const char * nonsense.

d5074cd

More const char * nonsense.

2ce7bd0

More const char * nonsense.

c8d6a17

Address failing tracing tests.

7a6559c

Address test failures on Windows.

9729715

Fix races on Windows.

1760bce

edwardalee mentioned this pull request Sep 5, 2022

Safe handling of physical actions in single-threaded C runtime lf-lang/lingua-franca#1348

Merged

1 task

lhstrh added 2 commits September 7, 2022 18:16

Make sure that if sleep is interrupted, we check the event queue to m…

a7d2029

…ake sure we advance to the correct tag

Cleanup

12cc014

erlingrj added 4 commits December 10, 2022 21:59

Remove old SSH CI file

7057436

Hack more at the windows support

049aa99

More fixes

4109b8d

More windows

8ca3d0f

Make windows platform API static since it is defined in headers

b6d3a0e

erlingrj and others added 4 commits December 14, 2022 22:48

Apply suggestions from code review

56d0cbc

Co-authored-by: Edward A. Lee <eal@eecs.berkeley.edu>

Address Edwards feedback on the Arduino support

847b7ce

Add code-review feedback

8361765

Merge branch 'lock-time' of https://github.com/lf-lang/reactor-c into…

e0ea9a6

… lock-time

erlingrj reviewed Dec 15, 2022

View reviewed changes

Update include/core/platform.h

a91f605

lhstrh commented Dec 16, 2022

View reviewed changes

erlingrj and others added 2 commits December 19, 2022 09:25

Apply suggestions from Marten

ea29aad

Co-authored-by: Marten Lohstroh <marten@berkeley.edu>

Rename nested critical section variable

70a8f77

lhstrh and others added 2 commits December 20, 2022 17:24

Merge branch 'main' into lock-time

af73a48

Update pointer to lf

6369aa9

erlingrj reviewed Dec 21, 2022

View reviewed changes

erlingrj and others added 5 commits December 21, 2022 10:22

Apply suggestions from code review

becb567

Co-authored-by: Marten Lohstroh <marten@berkeley.edu>

Add comment on empty critical section impl in os unthreaded

8962371

Update trace macro

c58b354

Update lf-ref

073f175

update lf ref

965e9b2

lhstrh merged commit d97620b into main Dec 21, 2022

lhstrh deleted the lock-time branch December 21, 2022 22:54

petervdonovan mentioned this pull request Jan 9, 2023

Changes to facilitate arduino-cli support #138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform support for Embedded targets #106

Platform support for Embedded targets #106

lhstrh commented Sep 3, 2022 •

edited

Loading

edwardalee left a comment

edwardalee commented Dec 11, 2022

erlingrj commented Dec 11, 2022 •

edited

Loading

erlingrj commented Dec 15, 2022

erlingrj Dec 15, 2022

erlingrj Dec 15, 2022

erlingrj Dec 15, 2022

lhstrh left a comment

lhstrh Dec 16, 2022

erlingrj Dec 19, 2022

lhstrh Dec 16, 2022

erlingrj Dec 19, 2022

lhstrh Dec 21, 2022

edwardalee commented Dec 21, 2022

lhstrh commented Dec 21, 2022

erlingrj Dec 19, 2022

erlingrj Dec 19, 2022

erlingrj Dec 19, 2022

erlingrj Dec 21, 2022

Platform support for Embedded targets #106

Platform support for Embedded targets #106

Conversation

lhstrh commented Sep 3, 2022 • edited Loading

edwardalee left a comment

Choose a reason for hiding this comment

edwardalee commented Dec 11, 2022

erlingrj commented Dec 11, 2022 • edited Loading

erlingrj commented Dec 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhstrh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardalee commented Dec 21, 2022

lhstrh commented Dec 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhstrh commented Sep 3, 2022 •

edited

Loading

erlingrj commented Dec 11, 2022 •

edited

Loading