Re: [PATCH v2 06/14] net-lwip: add ping command

+#include <net-lwip.h> +#include <time.h>

+#define PING_DELAY_MS 1000 +#define PING_TIMEOUT_MS 10000 +/* Additional data size to include in the packet */ +#define PING_DATA_SIZE 32 +/* Ping identifier - must fit on a u16_t */ +#define PING_ID 0xAFAF

+static const ip_addr_t *ping_target; +static struct raw_pcb *ping_pcb; +static u16_t ping_seq_num;

As a general note, u8_t u16_t etc are lwip constructs. Can we not use them and instead use the original definition? uint8_t etc. I am not sure introducing another define is what we want. Tom?

+static bool ping_target_alive;

+static u8_t ping_recv(void *arg, struct raw_pcb *pcb, struct pbuf *p,

•       const ip_addr_t *addr)
  

+{

• struct icmp_echo_hdr *iecho;
• if (addr->addr != ping_target->addr)

• return 0;
  

• if ((p->tot_len >= (IP_HLEN + sizeof(struct icmp_echo_hdr))) &&

•    pbuf_remove_header(p, IP_HLEN) == 0) {
  

• iecho = (struct icmp_echo_hdr *)p->payload;
  

• if ((iecho->id == PING_ID) &&
  

•     (iecho->seqno == lwip_htons(ping_seq_num))) {
  

• 	ping_target_alive = true;
  

• 	printf("host %s is alive\n", ipaddr_ntoa(addr));
  

• 	pbuf_free(p);
  

• 	return 1; /* eat the packet */
  

• }
  

• /* not eaten, restore original packet */
  

• pbuf_add_header(p, IP_HLEN);
  

• }
• return 0; /* don't eat the packet */

+}

+static int ping_raw_init(void) +{

• ping_pcb = raw_new(IP_PROTO_ICMP);
• if (!ping_pcb)

• return -ENOMEM;
  

• raw_recv(ping_pcb, ping_recv, NULL);

Instead of defining a global variable ping_target_alive can we instead pass the ptr of void *recv_arg? We can then fill in that private ptr with the status

• raw_bind(ping_pcb, IP_ADDR_ANY);
• return 0;

+}

+static void ping_raw_stop(void) +{

• if (ping_pcb != NULL) {

• raw_remove(ping_pcb);
  

• ping_pcb = NULL;
  

• }

+}

+static void ping_prepare_echo(struct icmp_echo_hdr *iecho, u16_t len) +{

• size_t i;
• size_t data_len = len - sizeof(struct icmp_echo_hdr);
• ICMPH_TYPE_SET(iecho, ICMP_ECHO);
• ICMPH_CODE_SET(iecho, 0);
• iecho->chksum = 0;
• iecho->id = PING_ID;
• iecho->seqno = lwip_htons(++ping_seq_num);
• /* Fill the additional data buffer with some data */
• for(i = 0; i < data_len; i++) {

• ((char *)iecho)[sizeof(struct icmp_echo_hdr) + i] = (char)i;
  

• }

is this additional data used? And if they are shouldn't we care about endianess?

• iecho->chksum = inet_chksum(iecho, len);

+}

+static void ping_send_icmp(struct raw_pcb *raw, const ip_addr_t *addr) +{

• struct pbuf *p;
• struct icmp_echo_hdr *iecho;
• size_t ping_size = sizeof(struct icmp_echo_hdr) + PING_DATA_SIZE;
• p = pbuf_alloc(PBUF_IP, (u16_t)ping_size, PBUF_RAM);
• if (!p)

• return;
  

• if ((p->len == p->tot_len) && (p->next == NULL)) {

• iecho = (struct icmp_echo_hdr *)p->payload;
  

• ping_prepare_echo(iecho, (u16_t)ping_size);
  

• raw_sendto(raw, p, addr);
  

• }
• pbuf_free(p);

+}

+static void ping_send(void *arg) +{

• struct raw_pcb *pcb = (struct raw_pcb *)arg;
• ping_send_icmp(pcb, ping_target);
• sys_timeout(PING_DELAY_MS, ping_send, ping_pcb);

+}

+static int ping_loop(const ip_addr_t* addr) +{

• ulong start;
• int ret;
• printf("Using %s device\n", eth_get_name());
• ret = ping_raw_init();
• if (ret < 0)

• return ret;
  

• ping_target = addr;
• start = get_timer(0);
• ping_send(ping_pcb);
• do {

• eth_rx();
  

eth_rx() has a ret value. Don't we have to check it here?

• if (ping_target_alive)
  

• 	break;
  

• sys_check_timeouts();
  

• if (ctrlc()) {
  

• 	printf("\nAbort\n");
  

• 	break;
  

• }
  

• } while (get_timer(start) < PING_TIMEOUT_MS);
• sys_untimeout(ping_send, ping_pcb);
• ping_raw_stop();
• ping_target = NULL;
• if (ping_target_alive) {

• ping_target_alive = false;
  

• return 0;
  

• }
• printf("ping failed; host %s is not alive\n", ipaddr_ntoa(addr));
• return -1;

+}

+int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) +{

• ip_addr_t addr;
• if (argc < 2)

• return CMD_RET_USAGE;
  

• if (!ipaddr_aton(argv[1], &addr))

• return CMD_RET_USAGE;
  

• if (ping_loop(&addr) < 0)

• return CMD_RET_FAILURE;
  

• return CMD_RET_SUCCESS;

+}

2.40.1

Regards /Ilias