Remove duplicated NetworkChannel return codes / state #143

LasseRosenow · 2024-11-29T17:26:26Z

Ready to review

github-actions · 2024-11-29T17:28:14Z

Benchmark results after merging this PR:

Benchmark results

Performance:

PingPongUc:
Best Time: 132.883 msec
Worst Time: 134.705 msec
Median Time: 133.723 msec

PingPongC:
Best Time: 176.228 msec
Worst Time: 180.540 msec
Median Time: 176.984 msec

ReactionLatencyUc:
Best latency: 49777 nsec
Median latency: 60162 nsec
Worst latency: 250425 nsec

ReactionLatencyC:
Best latency: 13391 nsec
Median latency: 59701 nsec
Worst latency: 117783 nsec

Memory usage:

PingPongUc:
text data bss dec hex filename
39572 752 8792 49116 bfdc bin/PingPongUc

PingPongC:
text data bss dec hex filename
46044 872 360 47276 b8ac bin/PingPongC

ReactionLatencyUc:
text data bss dec hex filename
29601 736 2080 32417 7ea1 bin/ReactionLatencyUc

ReactionLatencyC:
text data bss dec hex filename
41666 840 360 42866 a772 bin/ReactionLatencyC

github-actions · 2024-12-02T15:21:26Z

Memory usage after merging this PR will be:

Memory Report

action_empty_test_c

	from	to	increase (%)
text	59617	59319	-0.50
data	744	744	0.00
bss	10112	10112	0.00
total	70473	70175	-0.42

action_microstep_test_c

	from	to	increase (%)
text	60454	60156	-0.49
data	752	752	0.00
bss	10112	10112	0.00
total	71318	71020	-0.42

action_overwrite_test_c

	from	to	increase (%)
text	60291	59993	-0.49
data	744	744	0.00
bss	10112	10112	0.00
total	71147	70849	-0.42

action_test_c

	from	to	increase (%)
text	60227	59929	-0.49
data	752	752	0.00
bss	10112	10112	0.00
total	71091	70793	-0.42

delayed_conn_test_c

	from	to	increase (%)
text	61479	61181	-0.48
data	744	744	0.00
bss	10112	10112	0.00
total	72335	72037	-0.41

event_payload_pool_test_c

	from	to
text	18330	18330
data	624	624
bss	320	320
total	19274	19274

event_queue_test_c

	from	to
text	27597	27597
data	736	736
bss	480	480
total	28813	28813

nanopb_test_c

	from	to
text	42888	42888
data	904	904
bss	320	320
total	44112	44112

port_test_c

	from	to	increase (%)
text	61427	61129	-0.49
data	744	744	0.00
bss	10112	10112	0.00
total	72283	71985	-0.41

reaction_queue_test_c

	from	to
text	27277	27277
data	736	736
bss	480	480
total	28493	28493

request_shutdown_test_c

	from	to	increase (%)
text	60426	60128	-0.49
data	744	744	0.00
bss	10112	10112	0.00
total	71282	70984	-0.42

startup_test_c

	from	to	increase (%)
text	55777	55479	-0.53
data	752	752	0.00
bss	10752	10752	0.00
total	67281	66983	-0.44

tcp_channel_test_c

	from	to	increase (%)
text	60294	91172	51.21
data	1176	1224	4.08
bss	11136	21344	91.67
total	72606	113740	56.65

timer_test_c

	from	to	increase (%)
text	55668	55370	-0.54
data	744	744	0.00
bss	10752	10752	0.00
total	67164	66866	-0.44

LasseRosenow · 2024-12-02T15:59:41Z

Okay I think this should be ready for review now :) I am not entirely sure if the runtime internal stuff is implemented correctly.
I will also work on a follow-up PR (#147) now that simplifies the NetworkChannel interface as discussed and moves more into the channels internally.

tanneberger · 2024-12-02T19:03:43Z

src/federated.c

-        case LF_OK:
-          bundle->network_channel_state_changed(bundle);
+      ret = chan->try_connect(chan);
+      if (ret == LF_OK) {


Shouldn't also the channel state be updated if try_connect failes?

Actually I am not sure because the channel itself should have already notified the bundle, I would need to look deeper into the runtime code tomorrow to see if that is covered

tanneberger · 2024-12-02T19:07:08Z

src/platform/riot/coap_udp_ip_channel.c

@@ -7,17 +7,15 @@
 #include "net/sock/util.h"
 #include <arpa/inet.h>

-static bool _is_coap_initialized = false;
+static bool _is_globals_initialized = false;


we need those globals for some kind of handler function?

Yes I need Environment to get the channel bundle list in most callbacks to find the channel based on its socket. (See get_channel_by_remote or similar?)

tanneberger · 2024-12-02T19:07:56Z

src/platform/posix/tcp_ip_channel.c

@@ -198,7 +210,7 @@ static lf_ret_t TcpIpChannel_check_socket_error(int fd) {
  }
 }

-static lf_ret_t TcpIpChannel_try_connect_client(NetworkChannel *untyped_self) {
+static lf_ret_t _try_connect_client(NetworkChannel *untyped_self) {


the underscore is indicating that this is a "private" member function?

Yes as far as I understood it the TcpIpChannel Prefix is to mark the functions that implement the interface, so all other internal code structuring functions are easier to differentiate with for example included functions by using underscore to mark them as private

I am fine with this convention. But I would also be fine with omitting the _, since the functions are static they will not be visible outside this file anyways.

For me the main advantage of it is to help differentiate between our own static functions and for example included library functions. I can directly see that for example send_coap_message is a private function and not a RIOT coap API function.

But I also don't have a strong opinion on it

I mean within that c file of course

I would say:

TcpIpChannel_ means that it is a public facing API through function pointers on a struct, or through the header. _TcpIpChannel_ means private helper functions

Which is in accordance with the first

If you agree, then could you update the naming so its in accordance with this, and then we can merge this?

Yeah no problem

erlingrj · 2024-12-02T22:42:43Z

src/platform/posix/tcp_ip_channel.c

-      return LF_CONNECTION_CLOSED;
+      LF_INFO(NET, "Connection closed. Setting last known tag to FOREVER for all input ports");
+      if (self->federated_connection) {
+        self->federated_connection->network_channel_state_changed(self->federated_connection);


If I remember correctly, we talked about removing this call altogether and rely on the runtime checking the network channel state before waiting for input ports to become known at a tag.

erlingrj

This looks good, I believe this is the first of a two-stage change and I think my questions are both things that will be done in the next stage?

erlingrj · 2024-12-02T22:44:34Z

src/federated.c

-    case LF_OK:
-      break;
-    case LF_CONNECTION_CLOSED:
+    if (channel->send_blocking(channel, &msg) != LF_OK) {
      self->bundle->network_channel_state_changed(self->bundle);


If we remove this "callback"-way of handling network-channel state updates (see my other comment), then this call should probably also be dropped. Instead we check the network channel state each time before going to sleep waiting for some input port to become known at a tag

Regarding all the callback comments I think that change should go into the second PR. I tried to keep the scope of this one only within the deduplication of the return code and state :)

erlingrj · 2024-12-02T22:45:27Z

src/platform/posix/tcp_ip_channel.c

@@ -198,7 +210,7 @@ static lf_ret_t TcpIpChannel_check_socket_error(int fd) {
  }
 }

-static lf_ret_t TcpIpChannel_try_connect_client(NetworkChannel *untyped_self) {
+static lf_ret_t _try_connect_client(NetworkChannel *untyped_self) {


I am fine with this convention. But I would also be fine with omitting the _, since the functions are static they will not be visible outside this file anyways.

LasseRosenow · 2024-12-03T13:26:48Z

src/federated.c

-  case NETWORK_CHANNEL_STATE_LOST_CONNECTION:
-    for (size_t i = 0; i < self->inputs_size; i++) {
-      FederatedInputConnection *input = self->inputs[i];
-      input->last_known_tag = FOREVER_TAG;


I think I still don't understand the runtime fully enough, so I am not sure where to set this tag now instead

We could set it in Scheduler_acquire_tag, there we can check the state of the input port before we go to sleep waiting for more messages on it. We can add that in the next PR

LasseRosenow · 2024-12-03T13:27:39Z

Okay so I removed the state_changed callback, but there is one unclear thing remaining (see my comment)

LasseRosenow · 2024-12-04T00:09:10Z

Okay I implemented the naming convention and rebased with main

github-actions · 2024-12-04T00:12:42Z

Coverage after merging remove-redundant-network-channel-return-codes into main will be

70.61%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
src
action.c	81.90%	69.23%	100%	85.33%	120–121, 24, 43–46, 49, 51, 53, 56–58, 63–64, 73–74, 85–86
builtin_triggers.c	90.91%	70%	100%	96.77%	14, 18, 40, 43
connection.c	78.52%	51.16%	100%	88.66%	10, 104, 11, 110, 123–124, 136–137, 14, 14, 143, 145, 16–17, 21–22, 22, 22–23, 25, 27–28, 33, 48, 48, 48–49, 55, 60–62, 97
environment.c	78.35%	55.56%	84.62%	83.33%	12–13, 18, 20–21, 31, 35–36, 42–43, 51–52, 55–56, 60–61, 93–95
event.c	95.35%	92.86%	100%	96.15%	14–15
federated.c	5.41%	2.88%	7.69%	6.48%	100–102, 104, 104, 104–107, 109, 11, 111, 111, 111–112, 115, 118–119, 119, 119–120, 122–123, 125, 129, 13, 13, 13, 130, 132–134, 137, 139, 14, 140–144, 146–148, 15, 151–153, 153, 153–154, 154, 154–156, 158, 16, 161–162, 164–168, 17, 17, 17, 170–175, 177, 177, 177–180, 182, 182, 182–184, 184, 184–185, 189–190, 190, 190, 193–194, 198–199, 20, 200, 202, 202, 202, 204–208, 21, 211, 211, 211–214, 217–218, 218, 218–219, 22, 22, 22, 221–222, 225–226, 23, 231–232, 232, 232–233, 235, 237, 237, 237–239, 24, 24, 24, 240, 240, 240, 240, 240–249, 25, 250–252, 256, 259, 259, 259, 26, 260–261, 265, 268–269, 269, 269, 269, 27, 270–277, 279, 28, 28, 28, 285–287, 29, 299, 30, 30, 30, 30, 300, 303–306, 308, 308, 308–309, 31, 313–314, 314, 314, 316, 318–319, 319, 319, 32, 320, 320, 320–321, 321, 321–322, 322, 322–323, 323, 323–324, 324, 324, 326, 326, 326–327, 327, 327–328, 328, 328–329, 329, 329, 33, 331, 37, 37, 37, 37, 37–38, 40–43, 48, 48, 48, 48, 48–49, 52, 56–57, 59–62, 64, 64, 64–65, 65, 65, 67, 67, 67–69, 69, 69–71, 75–76, 80–81, 83–86, 88, 9, 90, 90, 90–91, 91, 91–92, 92, 92–93, 93, 93, 96–97, 99
logging.c	87.50%	80%	100%	88.64%	24, 37–39, 46, 59–60
port.c	81.43%	50%	100%	95.45%	10, 10, 10, 16, 20, 26, 26–28, 38, 38, 38–39
queues.c	89.94%	80.36%	100%	94.06%	108, 113, 119, 21–23, 47–48, 60–61, 84–88, 91–92
reaction.c	69.83%	54.55%	100%	77.61%	15, 17, 21–22, 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	50%	50%	50%	50%	16–17, 26–27, 33–35, 38–40
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	0%	0%	0%	0%	10, 3–4, 4, 4–5, 5, 5–6, 8–9
src/platform/posix
posix.c	52.73%	30%	66.67%	56%	100, 100, 100–102, 106, 16, 18, 20–21, 34–36, 38–40, 48–49, 54–59, 59, 59–62, 62, 62–64, 67, 73–74, 78, 81, 92–94, 94, 94–96, 98–99
tcp_ip_channel.c	70.44%	58.46%	100%	74.04%	101, 103, 107–108, 125–127, 132–135, 139–142, 161, 166, 166, 166, 170–171, 184–185, 188, 190, 190, 190, 192, 194, 197, 203–204, 206, 209, 224–225, 229–231, 233, 238–241, 244, 247, 250, 257–260, 263–266, 269–270, 301–303, 310, 315–317, 317, 317–318, 320–323, 332, 332–334, 356–359, 359, 359–361, 371–372, 372, 372–373, 373, 373–374, 378–379, 382, 394–395, 412–413, 417–418, 43, 433, 44, 463–464, 468–469, 473–474, 487–489, 52–53, 59–60, 60, 60–62, 66–68, 71–73, 87–88
src/schedulers/dynamic
scheduler.c	81.18%	62.22%	95%	87.39%	117, 119, 119, 126, 142, 194,

erlingrj

Great stuff Lasse!

LasseRosenow requested review from erlingrj and tanneberger and removed request for erlingrj December 2, 2024 15:57

LasseRosenow marked this pull request as ready for review December 2, 2024 15:58

LasseRosenow mentioned this pull request Dec 2, 2024

Simplify NetworkChannel interface #147

Closed

tanneberger reviewed Dec 2, 2024

View reviewed changes

erlingrj reviewed Dec 2, 2024

View reviewed changes

LasseRosenow commented Dec 3, 2024

View reviewed changes

LasseRosenow requested a review from erlingrj December 3, 2024 13:27

LasseRosenow added 5 commits December 4, 2024 01:08

Migrate runtime and coap channel

e401ced

Remove complicated LF_RETURN_TYPES from TcpIpChannel

65e239d

Fix examples and add ctor asserts

04fdbc1

Remove network_channel_state_changed callback

6e163d5

Implement naming convention

c45a551

LasseRosenow force-pushed the remove-redundant-network-channel-return-codes branch from 1bc27d9 to c45a551 Compare December 4, 2024 00:08

erlingrj approved these changes Dec 4, 2024

View reviewed changes

erlingrj merged commit 9092780 into main Dec 4, 2024
8 checks passed

erlingrj deleted the remove-redundant-network-channel-return-codes branch December 4, 2024 04:31

Remove duplicated NetworkChannel return codes / state #143

Remove duplicated NetworkChannel return codes / state #143

Conversation

LasseRosenow commented Nov 29, 2024 • edited Loading

github-actions bot commented Nov 29, 2024 • edited Loading

Performance:

Memory usage:

github-actions bot commented Dec 2, 2024 • edited Loading

action_empty_test_c

action_microstep_test_c

action_overwrite_test_c

action_test_c

delayed_conn_test_c

event_payload_pool_test_c

event_queue_test_c

nanopb_test_c

port_test_c

reaction_queue_test_c

request_shutdown_test_c

startup_test_c

tcp_channel_test_c

timer_test_c

LasseRosenow commented Dec 2, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlingrj 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

LasseRosenow commented Dec 3, 2024

LasseRosenow commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

erlingrj left a comment

Choose a reason for hiding this comment

LasseRosenow commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Dec 2, 2024 •

edited

Loading

LasseRosenow commented Dec 2, 2024 •

edited

Loading