Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add very basic support for FlexPRET platform #29

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

magnmaeh
Copy link
Member

@magnmaeh magnmaeh commented Oct 1, 2024

Current status: Can run with the following output. Seems to hang when it should print hello world - could be lots of reasons for that.

magnus@magnus-XPS-15-9570:~/dev/lingua-franca/test/uC/main$ ./bin/fp-lf-uc 
[0]: Assembling environment
[0]: Assigning levels
[0]: Running program

See branch reactor-uc-flexpret-support for the LF compiler changes too

@magnmaeh magnmaeh added enhancement New feature or request platform Platform support labels Oct 1, 2024
@magnmaeh magnmaeh linked an issue Oct 1, 2024 that may be closed by this pull request
@magnmaeh
Copy link
Member Author

magnmaeh commented Oct 3, 2024

I think this should be ready for review now

@magnmaeh magnmaeh marked this pull request as ready for review October 3, 2024 15:56
Copy link
Collaborator

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the effort Magnus! Just a couple of suggestions. Would be great if you could create a FlexPRET example under examples/flexpret and quickly document how to get it running. (No need to document the whole process of installing FlexPRET, just state the assumptions or refer to documentation).

A bigger question is this: How to correctly implement critical_section_enter/leave?

The purpose of that is to allow scheduling of physical actions. There are three types of programs we must handle:

  1. Programs where another FP thread is scheduling physical actions
  2. PRograms where an ISR within the same FP thread is scheduling physical actions.
  3. Both (1) and (2).

For (1) we need to acquire a lock in order to ensure mutual exclusion. In (2) it is enough to disable interrupts on the FP thread. For (3) I guess we must do both.

I think the best solution might be:
In enter_critical_section we do one of two things:
if in ISR: Then do nothing
if not in ISR: acquire lock and disable interrupts
In leave_critical_section:
if in ISR: Do nothing
if not in ISR: release lock and enable interrupts.

Also, currently we treat it as an error if there are nested critical sections. Lets try and keep it like this if we can

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
* One variable per hardware thread so each thread knows whether an async
* event occurred
*/
static volatile bool _async_event_occurred[FP_THREADS] = THREAD_ARRAY_INITIALIZER(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only support single-threaded operation, so putting a single variable on the PlatformFlexpret struct seems better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm okay, now I'm a bit confused between the comment you left and what you're saying here. You say we need to handle "Programs where another FP thread is scheduling physical actions", but here we only support single-threaded operation. Can these two statements be true at the same time?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that currently we only support the reactor-uc runtime managing a single thread. However, we can have other FP threads running asynchronous to the reactor-uc runtime. E.g. running a networking stack, and scheduling events into the reactor-uc runtime through a physical actin

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is probably OK for a networking stack, it is exactly this pattern that I'm trying to avoid having LF programmers use. Our goal should be that the LF programmer never explicitly starts a thread or acquires a mutex.

* leave (disable ints) -> leave -> enter (enable ints) -> leave (disable ints) ->
* enter (enable ints) -> enter
*/
static volatile int _critical_section_num_nested[FP_THREADS] = THREAD_ARRAY_INITIALIZER(0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would go for a single variable on the platform struct. BTW I am trying to avoid nested critical sections. If it is possible

Copy link
Contributor

Coverage after merging 22-add-platform-support-for-flexpret into main will be

35.11%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
external/nanopb
   pb_common.c0%0%0%0%100, 100, 100, 100, 100, 102, 106, 110, 110, 110, 110, 110, 112, 116, 119, 122, 124, 126, 126, 126, 129–132, 14, 14, 14, 142–144, 15, 150–152, 152, 152, 152, 152, 154, 156, 158, 160–161, 163, 166, 168, 17, 171–172, 172, 172, 177, 18, 181, 184–185, 188, 190–192, 195, 197, 197, 197, 199, 20, 20, 20, 20, 20, 201, 201, 201, 203, 207, 210, 210, 210, 215, 22, 221, 224, 226, 226, 226, 229, 231, 231, 231–232, 232, 232, 235, 238, 238, 238, 24, 241–242, 246, 248, 248, 248, 25, 250, 254, 26, 260, 263, 265, 265, 265, 267, 269, 269, 269, 27, 272–273, 277, 28, 286–287, 29, 290, 292, 295, 297, 300, 302, 302, 302, 304, 306, 306, 306, 308, 308, 308, 308, 308, 310, 313, 313, 313, 313, 313, 315, 32, 320, 34, 36–41, 44, 46–48, 50–55, 58, 60–63, 65–70, 74, 74, 74, 77–78, 8, 82, 84, 84, 84, 86, 88, 88, 88–89, 89, 89–90, 90, 90, 93, 97
   pb_decode.c0%0%0%0%100, 1000–1001, 1003–1004, 1007, 1007, 1007, 1009, 1009, 1009, 1011, 1011, 1011–1012, 1012, 1012, 1016, 1016, 1016, 1022, 1022, 1022, 1024, 1024, 1024–1025, 1027, 1030, 1030, 1030, 1032, 1032, 1032, 1034, 1038, 1038, 1038, 104, 104, 104, 1042, 1042, 1042, 1042, 1042, 1045, 1045, 1045, 1047, 1047, 1047, 1049, 105, 105, 105, 1050, 1053, 1053, 1053, 1055, 1059, 1059, 1059, 1061, 1063, 1063, 1063–1064, 1066, 1066, 1066, 1069, 1074, 1074, 1074–1076, 108, 108, 108, 1082, 1082, 1082, 1082, 1082, 1084, 1084, 1084, 1089, 1089, 1089, 109, 109, 109, 1090, 1090, 1090, 1092, 1092, 1092, 1095–1097, 1100, 1103, 1103, 1103–1104, 1104, 1104, 1106–1107, 1110, 1110, 1110–1111, 1115, 1115, 1115–1116, 1116, 1116, 1118, 1118, 1118, 1123, 1125, 1125, 1125, 1129, 1129, 1129–1130, 1133, 1133, 1133, 1135, 1135, 1135–1136, 1136, 1136, 1140, 1140, 1140, 1142–1143, 1143, 1143, 1145, 1145, 1145, 115, 115, 115, 1151, 1154, 1158, 1158, 1158, 116, 1160, 1165, 1165, 1165–1166, 1168, 1170, 1170, 1170–1171, 1179, 118, 1182, 1186, 1193, 120, 125, 127, 127, 127–128, 128, 128, 131, 131, 131–132, 132, 132, 1336, 1341, 1346, 1349, 1349, 1349–1350, 1352–1353, 1356, 1359, 1359, 1359–1360, 1362, 1362, 1362–1363, 1365, 1367, 1370, 1377, 1377, 1377–1378, 138, 1382, 1389, 1393, 140, 1400, 1400, 1400–1401, 1405, 1416, 1420, 1422, 1425, 1427, 1427, 1427, 143, 1430, 1430, 1430–1431, 1434, 1434, 1434–1436, 1436, 1436–1438, 1438, 1438–1440, 1440, 1440–1441, 1443, 1443, 1443, 1445, 1445, 1445–1446, 1446, 1446, 1448, 1456, 1456, 1456, 1458, 1458, 1458–1459, 1463, 1463, 1463–1464, 1472, 1472, 1472–1473, 1475, 1479, 1479, 1479–1481, 1481, 1481–1483, 1483, 1483–1485, 1485, 1485–1486, 1488, 1488, 1488, 1490, 1490, 1490–1491, 1491, 1491, 1493, 1497, 1503, 1503, 1503–1504, 1506, 1506, 1506–1507, 1507, 1507, 1509–1510, 1510, 1510–1511, 1511, 1511, 1513, 1513, 1513, 1516, 1516, 1516, 1528, 1528, 1528–1529, 1529, 1529–1530, 1533–1534, 1537, 1541, 1543, 1543, 1543–1544, 1546, 1546, 1546–1547, 1547, 1547, 1550, 1552, 1552, 1552–1553,

@erlingrj
Copy link
Collaborator

Give me a ping when you want me to do another review, Magnus :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request platform Platform support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants