drivers: esp_at: implement bind() and recvfrom() for UDP sockets #68586

LonesomeUnixCowboy · 2024-02-06T06:35:49Z

Implement bind() and recvfrom() for UDP sockets. This is achived by setting remote field in net_pkt which in return makes recvfrom() fill in *src_addr. This only works in passiv mode. Also set net_if to non-dormant when enableing AP mode to make binding to a port and address possible and back to dormant when AP is disabled.

jukkar · 2024-02-06T07:56:33Z

drivers/wifi/esp_at/esp_offload.c

+		if (!IS_ENABLED(CONFIG_NET_IPV4) || addr->sa_family != AF_INET) {
+			return -EAFNOSUPPORT;
+		}


Looks like this is dead code. The check two lines above already checks that IPv4 is enabled so it cannot be disabled here.

Yes it is. Had some brain slippage, I apologize.

jukkar · 2024-02-06T07:59:38Z

drivers/wifi/esp_at/esp_offload.c

+	/* IP addr comes within quotation marks, which is disliked by
+	 *  conv function. So we remove them.
+	 */
+	char remote_ip_addr[18];


We should not use magic constants as they are hard to guess where the number comes from. There is INET_ADDRSTRLEN that could be used here.

Changed to INT_ADDRSTRLEN.

jukkar · 2024-02-06T08:00:27Z

drivers/wifi/esp_at/esp_offload.c


+	strcpy(remote_ip_addr, &argv[1][1]);


No strcpy allowed, you need to use strncpy that restricts the length.

changed to strncpy

jukkar · 2024-02-06T08:01:52Z

drivers/wifi/esp_at/esp_offload.c


+	strcpy(remote_ip_addr, &argv[1][1]);
+	remote_ip_addr[strlen(remote_ip_addr)-1] = 0;


Please do remote_ip_addr[sizeof(remote_ip_addr)] = '\0' instead as then the length is calculated at compile time instead of runtime.

changed to suggested

I noticed a bug with this. If the IP address is less than sizeof(remote_ip_addr) garbage would end up in remote_ip_addr. So I suggest this solution:

char remote_ip_addr[INET_ADDRSTRLEN]; size_t remote_ip_str_len = strlen(argv[1]); strncpy(remote_ip_addr, &argv[1][1], remote_ip_str_len - 2); remote_ip_addr[remote_ip_str_len - 2] = '\0';

jukkar · 2024-02-06T08:02:32Z

drivers/wifi/esp_at/esp_offload.c


+	strcpy(remote_ip_addr, &argv[1][1]);
+	remote_ip_addr[strlen(remote_ip_addr)-1] = 0;
+	if (0 > net_addr_pton(AF_INET, remote_ip_addr, &recv_addr->sin_addr)) {


Please reverse the ordering here, so var < 0

reversed order

jukkar · 2024-02-06T12:01:18Z

drivers/wifi/esp_at/esp_offload.c

+	char remote_ip_addr[INET_ADDRSTRLEN];
+	size_t remote_ip_str_len = strlen(argv[1]);

-	err = cmd_ciprecvdata_parse(sock, data->rx_buf, len, &data_offset,
-				    &data_len);
-	if (err) {
-		if (err == -EAGAIN) {
-			return -EAGAIN;
-		}
+	strncpy(remote_ip_addr, &argv[1][1], remote_ip_str_len - 2);
+	remote_ip_addr[remote_ip_str_len - 2] = '\0';


I do not quite follow the logic here, but guessing that -2 is there for the quotation marks? Perhaps a more clear comment could be added for that.
When copying strings, we should figure out the limit from the copy target, and not from the source.
So I suggest:

remote_ip_str_len = MIN(sizeof(remote_ip_addr) - 1, strlen(argv[1]) - 2); strncpy(remote_ip_addr, &argv[1][1], remote_ip_str_len); remote_ip_addr[remote_ip_str_len] = '\0';

This is not tested.

Added suggested solution and extended comment. Regarding test did you mean if remote_ip_str_len is to big for remote_ip_addr? I that case i added a check for that aswell.

I just meant that I wrote the code snippet without actually testing and verifying that it works ok.

I see. I have tested it and it works.

jukkar · 2024-02-06T15:21:26Z

drivers/wifi/esp_at/esp_offload.c

+	if (remote_ip_str_len > sizeof(remote_ip_addr) - 1) {
+		LOG_ERR("Received IP-address string it too long.");
+		err = -EIO;
+		return err;
+	}


This check is no-op, the remote_ip_str_len cannot be larger that the sizeof() as we just set the minimum value in previous line. You can remove the check, it is not really needed here.

Removed it.

jukkar

LGTM

LonesomeUnixCowboy · 2024-02-07T19:40:58Z

Thank you @jukkar 😊

LonesomeUnixCowboy · 2024-02-13T08:33:27Z

I have gone back from using MODEM_CMD to MODEM_CMD_DIRECT and extended cmd_ciprecvdata_parse to parse out peer IP and PORT from +CIPRECVDATA message.

Implement bind() and recvfrom() for UDP sockets. This is achived by setting remote field in net_pkt which in return makes recvfrom() fill in *src_addr. This is only implemented for passiv mode and CIPDINFO needs to be enabled. Also set net_if to non-dormant when enabling AP mode to make binding to a port and address possible. Signed-off-by: John Johnson <[email protected]>

LonesomeUnixCowboy requested a review from mniestroj as a code owner February 6, 2024 06:35

zephyrbot added the area: Wi-Fi Wi-Fi label Feb 6, 2024

zephyrbot requested review from jukkar, Kludentwo, krish2718 and rlubos February 6, 2024 06:36

zephyrbot assigned jukkar Feb 6, 2024

jukkar requested changes Feb 6, 2024

View reviewed changes

LonesomeUnixCowboy force-pushed the esp_at_bind_udp_ap branch 3 times, most recently from 8b1c972 to 74a11e8 Compare February 6, 2024 11:56

jukkar reviewed Feb 6, 2024

View reviewed changes

LonesomeUnixCowboy force-pushed the esp_at_bind_udp_ap branch 2 times, most recently from d1c611b to 42cdc57 Compare February 6, 2024 13:07

jukkar reviewed Feb 6, 2024

View reviewed changes

LonesomeUnixCowboy force-pushed the esp_at_bind_udp_ap branch from 42cdc57 to d99b399 Compare February 7, 2024 06:36

jukkar previously approved these changes Feb 7, 2024

View reviewed changes

LonesomeUnixCowboy dismissed jukkar’s stale review via efbf4d1 February 13, 2024 08:18

LonesomeUnixCowboy force-pushed the esp_at_bind_udp_ap branch from d99b399 to efbf4d1 Compare February 13, 2024 08:18

LonesomeUnixCowboy requested a review from jukkar February 13, 2024 08:33

LonesomeUnixCowboy force-pushed the esp_at_bind_udp_ap branch 3 times, most recently from 0034910 to c2fa409 Compare February 16, 2024 10:55

LonesomeUnixCowboy force-pushed the esp_at_bind_udp_ap branch from c2fa409 to 3a187c9 Compare February 19, 2024 12:30

mniestroj approved these changes Feb 20, 2024

View reviewed changes

jukkar approved these changes Feb 22, 2024

View reviewed changes

aescolar merged commit dbf3d6e into zephyrproject-rtos:main Feb 26, 2024
19 checks passed

WilliamGFish mentioned this pull request Feb 27, 2024

drivers: esp32_at: UDP Sockets Client (STATION) Connection Failure #69530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: esp_at: implement bind() and recvfrom() for UDP sockets #68586

drivers: esp_at: implement bind() and recvfrom() for UDP sockets #68586

LonesomeUnixCowboy commented Feb 6, 2024

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 6, 2024

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 6, 2024

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 6, 2024

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 6, 2024

LonesomeUnixCowboy Feb 6, 2024

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 6, 2024

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 6, 2024 •

edited

Loading

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 7, 2024

jukkar Feb 6, 2024

LonesomeUnixCowboy Feb 7, 2024

jukkar left a comment

LonesomeUnixCowboy commented Feb 7, 2024

LonesomeUnixCowboy commented Feb 13, 2024 •

edited

Loading


		strcpy(remote_ip_addr, &argv[1][1]);
		remote_ip_addr[strlen(remote_ip_addr)-1] = 0;

drivers: esp_at: implement bind() and recvfrom() for UDP sockets #68586

drivers: esp_at: implement bind() and recvfrom() for UDP sockets #68586

Conversation

LonesomeUnixCowboy commented Feb 6, 2024

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

LonesomeUnixCowboy Feb 6, 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

jukkar left a comment

Choose a reason for hiding this comment

LonesomeUnixCowboy commented Feb 7, 2024

LonesomeUnixCowboy commented Feb 13, 2024 • edited Loading

LonesomeUnixCowboy Feb 6, 2024 •

edited

Loading

LonesomeUnixCowboy commented Feb 13, 2024 •

edited

Loading