Remove ioctl from TcpIpChannel #61

LasseRosenow · 2024-10-15T12:54:18Z

This changes the TcpIpChannel implementation in the following way

ioctl.h => sys/select.h
Use unistd.h for close instead of shutdown, which is not available in RIOT

The rationale behind this is to improve compatibility with platforms such as zephyr or RIOT which don't have full POSIX support.

This PR is a draft because I didn't have time to test it yet. There might be implementation bugs.

(I had some problems with compiling the example applications in examples/posix)

github-actions · 2024-10-15T12:55:31Z

Memory usage after merging this PR will be:

Memory Report

action_test_c

	from	to	increase (%)
text	71446	71651	0.29
data	976	976	0.00
bss	480	480	0.00
total	72902	73107	0.28

delayed_conn_test_c

	from	to	increase (%)
text	76067	76272	0.27
data	968	968	0.00
bss	480	480	0.00
total	77515	77720	0.26

event_queue_test_c

	from	to
text	23478	23478
data	648	648
bss	320	320
total	24446	24446

physical_action_test_c

	from	to	increase (%)
text	72685	72890	0.28
data	977	977	0.00
bss	1920	1920	0.00
total	75582	75787	0.27

port_test_c

	from	to	increase (%)
text	75999	76204	0.27
data	968	968	0.00
bss	480	480	0.00
total	77447	77652	0.26

reaction_queue_test_c

	from	to
text	23155	23155
data	648	648
bss	320	320
total	24123	24123

shutdown_test_c

	from	to	increase (%)
text	67976	68181	0.30
data	976	976	0.00
bss	480	480	0.00
total	69432	69637	0.30

startup_test_c

	from	to	increase (%)
text	67093	67298	0.31
data	976	976	0.00
bss	480	480	0.00
total	68549	68754	0.30

timer_test_c

	from	to	increase (%)
text	67079	67284	0.31
data	968	968	0.00
bss	480	480	0.00
total	68527	68732	0.30

trigger_value_test_c

	from	to
text	19312	19312
data	648	648
bss	320	320
total	20280	20280

LasseRosenow · 2024-10-15T12:58:34Z

src/platform/posix/tcp_ip_channel.c

+  timeout.tv_usec = 0;
+
+  // check if data is available
+  int select_result = select(socket + 1, &read_fds, NULL, NULL, &timeout);


I just gonna copy your comment from my previous PR to keep track of it:

@erlingrj wrote:

Do we actually need to check how much data is there before we read? Since this is running in a separate thread, we want it to block on recv and return with as much data is available. What is gained from checking how much data is there before the read?

(I know that this was there before your edits, and perhaps @tanneberger can answer better?)

erlingrj · 2024-10-16T17:16:31Z

Is this still a Draft or should we consider it for merging, @LasseRosenow? I like this approach since I can use it for Zephyr also

github-actions · 2024-10-16T17:19:08Z

Memory usage after merging this PR will be:

Memory Report

action_microstep_test_c

	from	to	increase (%)
text	71894	72099	0.29
data	976	976	0.00
bss	480	480	0.00
total	73350	73555	0.28

action_test_c

	from	to	increase (%)
text	71715	71920	0.29
data	976	976	0.00
bss	480	480	0.00
total	73171	73376	0.28

delayed_conn_test_c

	from	to	increase (%)
text	76371	76576	0.27
data	968	968	0.00
bss	480	480	0.00
total	77819	78024	0.26

event_queue_test_c

	from	to
text	23406	23406
data	648	648
bss	320	320
total	24374	24374

physical_action_test_c

	from	to	increase (%)
text	72950	73155	0.28
data	977	977	0.00
bss	1952	1952	0.00
total	75879	76084	0.27

port_test_c

	from	to	increase (%)
text	76337	76542	0.27
data	968	968	0.00
bss	480	480	0.00
total	77785	77990	0.26

reaction_queue_test_c

	from	to
text	23083	23083
data	648	648
bss	320	320
total	24051	24051

shutdown_test_c

	from	to	increase (%)
text	68208	68413	0.30
data	976	976	0.00
bss	480	480	0.00
total	69664	69869	0.29

startup_test_c

	from	to	increase (%)
text	67325	67530	0.30
data	976	976	0.00
bss	480	480	0.00
total	68781	68986	0.30

timer_test_c

	from	to	increase (%)
text	67325	67530	0.30
data	968	968	0.00
bss	480	480	0.00
total	68773	68978	0.30

trigger_data_queue_test_c

	from	to
text	19344	19344
data	648	648
bss	320	320
total	20312	20312

github-actions · 2024-10-16T17:51:50Z

Memory usage after merging this PR will be:

Memory Report

action_microstep_test_c

	from	to	increase (%)
text	71894	71099	-1.11
data	976	960	-1.64
bss	480	480	0.00
total	73350	72539	-1.11

action_test_c

	from	to	increase (%)
text	71715	70888	-1.15
data	976	960	-1.64
bss	480	480	0.00
total	73171	72328	-1.15

delayed_conn_test_c

	from	to	increase (%)
text	76371	75544	-1.08
data	968	952	-1.65
bss	480	480	0.00
total	77819	76976	-1.08

event_queue_test_c

	from	to
text	23406	23406
data	648	648
bss	320	320
total	24374	24374

physical_action_test_c

	from	to	increase (%)
text	72950	72123	-1.13
data	977	961	-1.64
bss	1952	1952	0.00
total	75879	75036	-1.11

port_test_c

	from	to	increase (%)
text	76337	75510	-1.08
data	968	952	-1.65
bss	480	480	0.00
total	77785	76942	-1.08

reaction_queue_test_c

	from	to
text	23083	23083
data	648	648
bss	320	320
total	24051	24051

shutdown_test_c

	from	to	increase (%)
text	68208	67381	-1.21
data	976	960	-1.64
bss	480	480	0.00
total	69664	68821	-1.21

startup_test_c

	from	to	increase (%)
text	67325	66498	-1.23
data	976	960	-1.64
bss	480	480	0.00
total	68781	67938	-1.23

timer_test_c

	from	to	increase (%)
text	67325	66498	-1.23
data	968	952	-1.65
bss	480	480	0.00
total	68773	67930	-1.23

trigger_data_queue_test_c

	from	to
text	19344	19344
data	648	648
bss	320	320
total	20312	20312

LasseRosenow · 2024-10-17T12:20:43Z

Is this still a Draft or should we consider it for merging, @LasseRosenow? I like this approach since I can use it for Zephyr also

I will add more examples to RIOT today and see if I can test if everything still works correctly and undraft it :)

github-actions · 2024-10-17T15:37:59Z

Memory usage after merging this PR will be:

Memory Report

action_microstep_test_c

	from	to	increase (%)
text	71894	71099	-1.11
data	976	960	-1.64
bss	480	480	0.00
total	73350	72539	-1.11

action_test_c

	from	to	increase (%)
text	71715	70888	-1.15
data	976	960	-1.64
bss	480	480	0.00
total	73171	72328	-1.15

delayed_conn_test_c

	from	to	increase (%)
text	76371	75544	-1.08
data	968	952	-1.65
bss	480	480	0.00
total	77819	76976	-1.08

event_queue_test_c

	from	to
text	23406	23406
data	648	648
bss	320	320
total	24374	24374

physical_action_test_c

	from	to	increase (%)
text	72950	72123	-1.13
data	977	961	-1.64
bss	1952	1952	0.00
total	75879	75036	-1.11

port_test_c

	from	to	increase (%)
text	76337	75510	-1.08
data	968	952	-1.65
bss	480	480	0.00
total	77785	76942	-1.08

reaction_queue_test_c

	from	to
text	23083	23083
data	648	648
bss	320	320
total	24051	24051

shutdown_test_c

	from	to	increase (%)
text	68208	67381	-1.21
data	976	960	-1.64
bss	480	480	0.00
total	69664	68821	-1.21

startup_test_c

	from	to	increase (%)
text	67325	66498	-1.23
data	976	960	-1.64
bss	480	480	0.00
total	68781	67938	-1.23

timer_test_c

	from	to	increase (%)
text	67325	66498	-1.23
data	968	952	-1.65
bss	480	480	0.00
total	68773	67930	-1.23

trigger_data_queue_test_c

	from	to
text	19344	19344
data	648	648
bss	320	320
total	20312	20312

github-actions · 2024-10-17T15:39:08Z

Coverage after merging tcp-ip-channel-no-ioctl into main will be

32.27%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
external/nanopb
pb_common.c	0%	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.c	0%	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, 1553, 1553, 1555,

erlingrj · 2024-10-18T00:27:18Z

@LasseRosenow I am merging this because I have some other work based off this branch that I want to merge. I hope this doesn't cause any problems for you

Remove ioctl from TcpIpChannel

4477b51

LasseRosenow mentioned this pull request Oct 15, 2024

Remove ioctl from TcpIpChannel #59

Closed

LasseRosenow commented Oct 15, 2024

View reviewed changes

LasseRosenow requested a review from tanneberger October 15, 2024 13:01

LasseRosenow changed the title ~~Retry: Remove ioctl from TcpIpChannel~~ Remove ioctl from TcpIpChannel Oct 16, 2024

erlingrj marked this pull request as ready for review October 16, 2024 17:16

Merge branch 'main' into tcp-ip-channel-no-ioctl

0d84971

Make NetworkcChannel blocking. Fix some typos. Remove some debug prints.

97ca086

Include posix TcpIpChannel when platform is RIOT

a014875

erlingrj merged commit cec0203 into main Oct 18, 2024
2 checks passed

erlingrj deleted the tcp-ip-channel-no-ioctl branch October 25, 2024 23:27

Remove ioctl from TcpIpChannel #61

Remove ioctl from TcpIpChannel #61

Conversation

LasseRosenow commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

action_test_c

delayed_conn_test_c

event_queue_test_c

physical_action_test_c

port_test_c

reaction_queue_test_c

shutdown_test_c

startup_test_c

timer_test_c

trigger_value_test_c

LasseRosenow Oct 15, 2024

Choose a reason for hiding this comment

erlingrj commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

action_microstep_test_c

action_test_c

delayed_conn_test_c

event_queue_test_c

physical_action_test_c

port_test_c

reaction_queue_test_c

shutdown_test_c

startup_test_c

timer_test_c

trigger_data_queue_test_c

github-actions bot commented Oct 16, 2024

action_microstep_test_c

action_test_c

delayed_conn_test_c

event_queue_test_c

physical_action_test_c

port_test_c

reaction_queue_test_c

shutdown_test_c

startup_test_c

timer_test_c

trigger_data_queue_test_c

LasseRosenow commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

action_microstep_test_c

action_test_c

delayed_conn_test_c

event_queue_test_c

physical_action_test_c

port_test_c

reaction_queue_test_c

shutdown_test_c

startup_test_c

timer_test_c

trigger_data_queue_test_c

github-actions bot commented Oct 17, 2024

erlingrj commented Oct 18, 2024