UARTChannel

[tanneberger]

No description provided.

[github-actions]

Memory usage after merging this PR will be:

Memory Report

action_empty_test_c

	from	to	increase (%)
text	60821	60824	0.00
data	752	752	0.00
bss	11360	11360	0.00
total	72933	72936	0.00

action_microstep_test_c

	from	to	increase (%)
text	61692	61695	0.00
data	760	760	0.00
bss	11424	11424	0.00
total	73876	73879	0.00

action_overwrite_test_c

	from	to	increase (%)
text	61529	61532	0.00
data	752	752	0.00
bss	11424	11424	0.00
total	73705	73708	0.00

action_test_c

	from	to	increase (%)
text	61433	61436	0.00
data	760	760	0.00
bss	11424	11424	0.00
total	73617	73620	0.00

deadline_test_c

	from	to	increase (%)
text	57063	57066	0.01
data	768	768	0.00
bss	10784	10784	0.00
total	68615	68618	0.00

delayed_conn_test_c

	from	to	increase (%)
text	62425	62428	0.00
data	752	752	0.00
bss	10272	10272	0.00
total	73449	73452	0.00

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	27616	0.00
data	736	736	0.00
bss	480	480	0.00
total	28832	28832	0.00

nanopb_test_c

	from	to	increase (%)
text	42886	42886	0.00
data	904	904	0.00
bss	320	320	0.00
total	44110	44110	0.00

port_test_c

	from	to	increase (%)
text	62373	62376	0.00
data	752	752	0.00
bss	10272	10272	0.00
total	73397	73400	0.00

reaction_queue_test_c

	from	to	increase (%)
text	27448	27448	0.00
data	736	736	0.00
bss	480	480	0.00
total	28664	28664	0.00

request_shutdown_test_c

	from	to	increase (%)
text	61664	61667	0.00
data	752	752	0.00
bss	11424	11424	0.00
total	73840	73843	0.00

startup_test_c

	from	to	increase (%)
text	56762	56765	0.01
data	760	760	0.00
bss	10784	10784	0.00
total	68306	68309	0.00

tcp_channel_test_c

	from	to	increase (%)
text	96828	96846	0.02
data	1256	1256	0.00
bss	21376	21408	0.15
total	119460	119510	0.04

timer_test_c

	from	to	increase (%)
text	56653	56656	0.01
data	752	752	0.00
bss	10784	10784	0.00
total	68189	68192	0.00

[github-actions]

Benchmark results after merging this PR:

Benchmark results

Performance:

PingPongUc:
Best Time: 202.729 msec
Worst Time: 207.193 msec
Median Time: 204.365 msec

PingPongC:
Best Time: 169.416 msec
Worst Time: 178.555 msec
Median Time: 171.142 msec

ReactionLatencyUc:
Best latency: 20532 nsec
Median latency: 60211 nsec
Worst latency: 296113 nsec

ReactionLatencyC:
Best latency: 23941 nsec
Median latency: 60418 nsec
Worst latency: 341854 nsec

Memory usage:

PingPongUc:
text data bss dec hex filename
40884 760 7440 49084 bfbc bin/PingPongUc

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

ReactionLatencyUc:
text data bss dec hex filename
30689 744 2080 33513 82e9 bin/ReactionLatencyUc

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

[erlingrj]

This is looking very good. There is various feedback and a few potential problems (e.g. poll should read out all messages in the buffer, not just the first.

Also I think calling the buffers receive_buffer and send_buffer will be clearer so there is no confusion in the logic that is reading and writing to those buffers.

[erlingrj]

For this to work, UartPollChannel_poll must always be called within a critical section, so that interrupts are disabled. A few other places I added _locked postfix to functions that has to be called from critical sections. Maybe this would be wise for this one as well?

[tanneberger]

I would suggest we protect updates to the receive_buffer_index but the receive_buffer I would not protect.

[erlingrj]

Hmm, I do not think this is safe. What happens e.g. if the interrupt occurs between line 79 and 84? Say we have parsed out a message, and that there are 4 bytes left. Well, if the interrupt occurs, then really there should be 5 bytes left, so when we do the memcpy to copy the bytes back to the beginning of the buffer, we forget the fifth byte and it is lost.

I think the memcpy has to be part of the critical section as well

[tanneberger]

What I would prefer to this is turning the receive_buffer_index into an atomic, then we could get rid of that lock.

I think only protecting the receive_buffer_index is enough, because the interrupts are sequential so the incrementing is sequential and overwriting the receive_buffer_index also gets sequenialized because we lock this region.

[erlingrj]

If you want it lock-free I think you need to turn it into a circular buffer.

[erlingrj]

This is starting to look real good, nice progress. I pushed a few trivial fixes, but I think there still is some race condition between the ISR and the runtime. The other day we discussed lock-free queues, but the current implementation needs lock (or disabling interrupts). We could make it lock free by implementing a circular buffer between the ISR and the runtime (but we would then have to move the data from this circular buffer to a "linear" buffer before feeding it forward to the deserialization pipeline.

Also, I realized that we should expose more UART settings to the user.

Keep up the good work!!

[erlingrj]

Getting there, but I think the critical section must be larger!

[erlingrj]

Hmm, I do not think this is safe. What happens e.g. if the interrupt occurs between line 79 and 84? Say we have parsed out a message, and that there are 4 bytes left. Well, if the interrupt occurs, then really there should be 5 bytes left, so when we do the memcpy to copy the bytes back to the beginning of the buffer, we forget the fifth byte and it is lost.

I think the memcpy has to be part of the critical section as well

[erlingrj]

What is the LED used for, I cant see any code that uses something LED related?

[tanneberger]

@erlingrj there is not include/reactor-uc/network_channel/uart_channel.h for the platform independent uart enums.

[github-actions]

Coverage after merging uart into main will be

70.59%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
action.c	77.69%	65.63%	100%	81.18%	134–135, 24, 42–45, 48, 50–51, 54–56, 62–63, 70–72, 72, 72–75, 81–82, 93–94
builtin_triggers.c	90.91%	70%	100%	96.77%	14, 18, 40, 43
connection.c	77.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.c	82.18%	60%	92.31%	86.76%	12–13, 18, 20–21, 31, 35–36, 42–43, 59–60, 64–65, 97–99
event.c	95.35%	92.86%	100%	96.15%	14–15
federated.c	5.23%	2.73%	7.69%	6.33%	10, 100, 102, 104, 104, 104–105, 109, 11, 112–113, 113, 113–114, 116–117, 119, 123–124, 126–128, 13, 131, 133–138, 140–142, 145–147, 147, 147–148, 148, 148–149, 15, 15, 15, 150, 152, 155–156, 158–159, 16, 160–162, 164–169, 17, 171, 171, 171–174, 176, 176, 176–178, 178, 178–179, 18, 183–184, 184, 184, 187–188, 19, 19, 19, 192–194, 196, 196, 196, 198–202, 205, 205, 205–208, 211–212, 212, 212–213, 215–216, 219, 22, 220, 225–226, 226, 226–227, 229, 23, 231, 231, 231–234, 234, 234, 234, 234, 234–239, 24, 24, 24, 240–243, 243, 243–244, 246, 248–249, 25, 250–256, 26, 26, 26, 260, 263, 263, 263–265, 269, 27, 272–273, 273, 273, 273–279, 28, 280–281, 283, 289, 29, 29, 29, 290–291, 30, 30, 30, 30, 30, 303–304, 307–309, 31, 310, 312, 312, 312–313, 317–318, 318, 318, 320, 322–323, 323, 323–324, 324, 324–325, 325, 325–326, 326, 326–327, 327, 327–328, 328, 328–329, 329, 329, 33, 331, 331, 331–332, 332, 332–333, 333, 333–334, 334, 334–335, 335, 335, 337, 36, 36, 36, 36, 36–37, 41–42, 46–47, 49–52, 54, 54, 54–55, 55, 55, 57, 57, 57–59, 59, 59–61, 65–66, 70–71, 73–76, 78, 80, 80, 80–81, 81, 81–82, 82, 82–83, 83, 83, 86–87, 89–92, 94, 94, 94–97, 97, 97–98
logging.c	88.52%	83.33%	100%	89.36%	25, 38–40, 47, 60–61
network_channel.c	69.23%	62.50%	100%	70.59%	42, 42, 42, 47–50, 59
port.c	78.08%	45.83%	100%	93.33%	10, 10, 10, 16, 20, 25, 25–27, 27, 27–28, 39, 39, 39–40
queues.c	89.94%	80.36%	100%	94.06%	108, 113, 119, 21–23, 47–48, 60–61, 84–88, 91–92
reaction.c	71.19%	54.55%	100%	79.71%	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.c	69.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.c	37.50%	25%	50%	40%	16–17, 26–27, 33–34, 34, 34–35, 37–38, 41–42, 42, 42–43, 45–46
tag.c	40.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.c	95%	66.67%	100%	100%	14, 25
trigger.c	100%	100%	100%	100%
util.c	41.67%	33.33%	33.33%	46.67%	12–13, 13, 13–16, 18–20, 4–5
src/platform/posix
posix.c	53.15%	30%	66.67%	56.58%	100–101, 101, 101–103, 107, 16, 18, 20–21, 35–37, 39–41, 49–50, 55–60, 60, 60–63, 63, 63–65, 68, 74–75, 79, 82, 93–95, 95, 95–97, 99
tcp_ip_channel.c	65.16%	51.42%	94.12%	74.44%	100, 103–104, 104, 104–105, 119–120, 122, 124, 128–129, 137, 140–141, 141, 141–142, 147–148, 148, 148–149, 155–156, 156, 156–158, 172, 175, 179, 179, 179, 179, 179–180, 180, 180–181, 181, 181–182, 184, 186, 186, 186–187, 196, 203–204, 208–209, 213, 213, 213–215, 215, 215–216, 218, 218, 218–219, 219, 219, 221, 223–224, 228, 228, 228–229, 249, 262–263, 263, 263–264, 270, 275–276, 276, 276–277, <

[erlingrj]

Looks good. But perhaps changing the number of spaces should be a separate PR. Something is not correctly configured with Spotless as it does not complain on main and you have changed every line of the code-generator. It makes it very hard to see the actual changes

[erlingrj]

If you want it lock-free I think you need to turn it into a circular buffer.

[erlingrj]

Should this be INT?

[erlingrj]

Should this also be INT?