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 maxwait, start time coordination and system events #206

Merged
merged 67 commits into from
Feb 18, 2025
Merged

Conversation

erlingrj
Copy link
Collaborator

@erlingrj erlingrj commented Jan 28, 2025

This PR took on a lot of weight unfortunately and now contains three major features:

  1. The maxwait property.
    This PR adds a maxwait property for each reaction with an optional STP violation handler. This includes changes the the code-generator and the grammar. In particular we must find the maximum maxwait associated with any reaction triggered by an input. We also add an intended_tag field to events so that we can detect the STP violations.

  2. Startup coordination.
    We also add a StartupCoordinator which each federate will have one of. It does a blocking gossip algorithm at boot negotiating the startup tag of the federation.

  3. SystemEvents
    Finally, this PR adds the notion of system events and a system event queue. Still this is not used, but I will use it for implementing the ClockSync algorithm. The idea is that any received coordination message will be put on the SystemEventQueue so that we can do blocking IO in response to received messages.

Copy link
Contributor

github-actions bot commented Jan 28, 2025

Benchmark results after merging this PR:

Benchmark results

Performance:

PingPongUc:
Best Time: 166.667 msec
Worst Time: 167.706 msec
Median Time: 166.683 msec

PingPongC:
Best Time: 169.468 msec
Worst Time: 176.789 msec
Median Time: 169.881 msec

ReactionLatencyUc:
Best latency: 31491 nsec
Median latency: 60030 nsec
Worst latency: 105071 nsec

ReactionLatencyC:
Best latency: 57797 nsec
Median latency: 60620 nsec
Worst latency: 86930 nsec

Memory usage:

PingPongUc:
text data bss dec hex filename
40010 760 8072 48842 beca bin/PingPongUc

PingPongC:
text data bss dec hex filename
45826 880 360 47066 b7da bin/PingPongC

ReactionLatencyUc:
text data bss dec hex filename
30002 744 2312 33058 8122 bin/ReactionLatencyUc

ReactionLatencyC:
text data bss dec hex filename
41536 848 360 42744 a6f8 bin/ReactionLatencyC

@erlingrj erlingrj marked this pull request as ready for review February 6, 2025 13:36
@erlingrj erlingrj changed the title [DRAFT] Add safe-to-assume-absent functionality Add max-wait property for each reaction with optional handler Feb 6, 2025
Copy link
Contributor

github-actions bot commented Feb 6, 2025

Memory usage after merging this PR will be:

Memory Report

action_empty_test_c

from to increase (%)
text 60860 62710 3.04
data 752 752 0.00
bss 11360 12224 7.61
total 72972 75686 3.72

action_microstep_test_c

from to increase (%)
text 61731 63581 3.00
data 760 760 0.00
bss 11424 12288 7.56
total 73915 76629 3.67

action_overwrite_test_c

from to increase (%)
text 61568 63418 3.00
data 752 752 0.00
bss 11424 12288 7.56
total 73744 76458 3.68

action_test_c

from to increase (%)
text 61472 63322 3.01
data 760 760 0.00
bss 11424 12288 7.56
total 73656 76370 3.68

deadline_test_c

from to increase (%)
text 57102 58838 3.04
data 768 768 0.00
bss 10784 11648 8.01
total 68654 71254 3.79

delayed_conn_test_c

from to increase (%)
text 62464 64096 2.61
data 752 752 0.00
bss 10272 13056 27.10
total 73488 77904 6.01

event_payload_pool_test_c

from to increase (%)
text 18331 18331 0.00
data 624 624 0.00
bss 320 320 0.00
total 19275 19275 0.00

event_queue_test_c

from to increase (%)
text 27616 28667 3.81
data 736 728 -1.09
bss 480 480 0.00
total 28832 29875 3.62

nanopb_test_c

from to increase (%)
text 42884 43346 1.08
data 904 1288 42.48
bss 320 320 0.00
total 44108 44954 1.92

port_test_c

from to increase (%)
text 62412 64012 2.56
data 752 752 0.00
bss 10272 12928 25.86
total 73436 77692 5.80

reaction_queue_test_c

from to increase (%)
text 27448 27897 1.64
data 736 728 -1.09
bss 480 480 0.00
total 28664 29105 1.54

request_shutdown_test_c

from to increase (%)
text 61703 63553 3.00
data 752 752 0.00
bss 11424 12288 7.56
total 73879 76593 3.67

startup_test_c

from to increase (%)
text 56801 58436 2.88
data 760 760 0.00
bss 10784 11648 8.01
total 68345 70844 3.66

tcp_channel_test_c

from to increase (%)
text 96880 95159 -1.78
data 1256 1640 30.57
bss 21408 12928 -39.61
total 119544 109727 -8.21

timer_test_c

from to increase (%)
text 56692 58436 3.08
data 752 752 0.00
bss 10784 11648 8.01
total 68228 70836 3.82

@erlingrj erlingrj changed the title Add max-wait property for each reaction with optional handler Add maxwait, start time coordination and system events Feb 17, 2025
StartupCoordinationState_HANDSHAKING = 2,
StartupCoordinationState_NEGOTIATING = 3,
StartupCoordinationState_RUNNING = 4
} StartupCoordinationState;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the connecting step here the waiting for the network channel to connect?

@@ -71,7 +75,7 @@ typedef int64_t interval_t;
/**
* Microstep instant.
*/
typedef uint32_t microstep_t;
typedef int32_t microstep_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A microstep can be negative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, thanks!

Copy link
Collaborator

@LasseRosenow LasseRosenow left a comment

Choose a reason for hiding this comment

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

Looking good so far, nice work! I just wrote some small comments / questions above.
Curious why the RIOT test is still failing though. But I could look into it when I have more free time I guess.

@erlingrj
Copy link
Collaborator Author

Thanks for the review. The segfault caught by RIOT was a mistake by me. Running tests on many different platforms turns out to be an asset as we are more likely to catch memory bugs

Copy link
Contributor

Coverage after merging add-staa into main will be

65.16%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   action.c76.42%65.63%100%79.31%136–137, 24, 42–45, 48, 50–51, 54–57, 63–64, 71–73, 73, 73–76, 82–84, 95–96
   builtin_triggers.c90.91%70%100%96.77%14, 18, 40, 43
   connection.c77.33%51.16%100%86.73%10, 104, 11, 110, 123–124, 136–137, 14, 14, 143, 145–146, 148, 16–17, 21–22, 22, 22–23, 25, 27–28, 33, 48, 48, 48–49, 55, 60–62, 97
   environment.c77.17%53.13%92.31%84.15%111–112, 124–126, 14–15, 20, 24–26, 26, 26–28, 28, 28, 34–35, 44, 48–49, 55–56, 72–73, 77–78
   event.c95.35%92.86%100%96.15%14–15
   federated.c6.83%3.33%10%8.43%10, 100, 102, 102, 102–103, 105, 105, 105, 105, 105–106, 106, 106–108, 11, 111, 111, 111–113, 115–116, 119–121, 121, 121–122, 122, 122–124, 126, 129, 13, 130, 132–136, 14, 140–142, 144, 144, 144, 146–149, 15, 150, 153, 153, 153–156, 159, 16, 160, 160, 160–161, 163–164, 167–168, 173–174, 174, 174–175, 177, 179, 179, 179, 18, 18, 18, 180–182, 182, 182, 182, 182, 182–189, 19, 19, 19, 190–191, 191, 191–192, 194, 196–205, 209, 21, 21, 21, 212, 212, 212–214, 218, 22, 221–222, 222, 222, 222–227, 229, 23, 23, 23, 230–231, 233, 239, 24, 240–241, 25, 254–255, 255, 255–256, 256, 256–257, 257, 257–258, 258, 258–259, 259, 259–260, 260, 260–261, 261, 261, 263, 263, 263–264, 264, 264–265, 265, 265–266, 266, 266–267, 267, 267, 269, 29–30, 34–35, 37–40, 42, 44, 44, 44–45, 45, 45–46, 46, 46–47, 47, 47, 50, 52–55, 57, 57, 57–60, 60, 60–61, 63, 65–66, 66, 66–67, 71, 74–75, 75, 75–76, 78–79, 81, 85–86, 88–90, 93, 95–99
   logging.c88.52%83.33%100%89.36%25, 38–40, 47, 60–61
   network_channel.c69.23%62.50%100%70.59%42, 42, 42, 47–50, 59
   port.c74.68%42.86%100%91.49%10, 10, 10, 15, 15, 15–16, 22, 26, 31, 31–33, 33, 33–34, 45, 45, 45–46
   queues.c86.21%79.03%100%88.37%114–118, 121–122, 139, 144, 150, 29, 29, 33–38, 61–62, 86, 86, 90–95
   reaction.c71.43%54.55%100%80%15, 17, 21, 28–31, 31, 31–32, 42, 45, 47, 52–53, 53, 53–55, 55, 55–56, 73, 89–91, 91, 91–94, 94, 94–95
   reactor.c69.33%51.52%100%82.28%10, 101–102, 14–19, 22, 28, 30, 32–37, 37, 37–38, 38, 38, 43, 55, 58–59, 59, 59–60, 60, 60–61, 63, 77–78, 81–82, 82, 82–83, 83, 83–84, 86, 91
   serialization.c37.50%25%50%40%16–17, 26–27, 33–34, 34, 34–35, 37–38, 41–42, 42, 42–43, 45–46
   startup_coordinator.c0%0%0%0%100–101, 101, 101, 101, 101–109, 111, 120–123, 123, 123–125, 127, 127, 127–131, 134, 136–139, 147–148, 161–162, 162, 162–164, 169–170, 170, 170–171, 174, 174, 174–176, 179–180, 180, 180–186, 188, 190–192, 195–198, 21, 210, 212, 214, 214, 214, 214, 214, 214–217, 217, 217, 217, 217, 217, 217–219, 221–223, 223, 223–228, 228, 228–229, 229, 229–230, 230, 230, 230, 230, 230, 230, 232, 232, 232, 234, 234, 234–236, 236, 236–238, 24, 240–241, 244, 247–249, 25, 25, 25, 251, 253, 253, 253, 253, 253, 253, 253–262, 262, 262–266, 269, 27, 270–273, 28–29, 29, 29–30, 32, 32, 32, 34, 34, 34–35, 37, 39, 39, 39–40, 45, 45, 45, 47, 50, 54–55, 61–63, 63, 63–65, 69, 71, 71, 71–75, 75, 75, 77, 80–81, 83, 85, 92–93, 93, 93, 93, 93–94, 94, 94
   tag.c40.19%31.48%60%47.92%14, 14–15, 17, 17–18, 23–24, 24, 24, 24, 24–25, 27, 27, 27, 27, 27–28, 30, 30, 30–31, 33–34, 34, 34–35, 37, 37, 37, 37, 37–38, 40, 40, 40, 40, 40–41, 43, 53–54, 63, 63–64, 83–85, 85, 85, 85, 85, 85, 85, 85, 85, 85, 85–87, 89
   timer

@erlingrj erlingrj merged commit 8f6b03a into main Feb 18, 2025
12 checks passed
@erlingrj erlingrj deleted the add-staa branch February 18, 2025 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants