Ping command for Portenta C33 #424
Conversation
per1234
added
type: enhancement
pennam
force-pushed
the
add-ping-command
branch
from
9f494e9 to
c115098
Compare
pennam
force-pushed
the
add-ping-command
branch
from
c115098 to
d81171e
Compare
| ICMPH_TYPE_SET(iecho, ICMP_ECHO); | ||
| ICMPH_CODE_SET(iecho, 0); | ||
| iecho->chksum = 0; | ||
| iecho->id = 0xAFAF; |
There was a problem hiding this comment.
In order to avoid strange cases of stray icmp packets I would make this not fixed, either an incremental id(faster) or random(better), and keep it in the context recv_callback_data, for it to match correctly
There was a problem hiding this comment.
isn't the random sequence number requestCbkData.seqNum enough?
There was a problem hiding this comment.
Quoting rfc792, I see that there is not a preferred method. Using both we lower the chances of possible collisions. I don't see any reason to enforce this change.
For future implementations I would even consider matching against the source and destination IP.
Identifier
If code = 0, an identifier to aid in matching echos and replies,
may be zero.Sequence Number
If code = 0, a sequence number to aid in matching echos and
replies, may be zero.
|
|
||
| struct pbuf *p; | ||
| struct icmp_echo_hdr *iecho; | ||
| size_t ping_size = sizeof(struct icmp_echo_hdr) + 32; |
There was a problem hiding this comment.
I would add a comment explaining why we need the icmp packet needs to be at least 64 bytes in size
libraries/lwIpWrapper/src/CNetIf.cpp
Outdated
| if ((p->tot_len >= (PBUF_IP_HLEN + sizeof(struct icmp_echo_hdr))) && | ||
| pbuf_remove_header(p, PBUF_IP_HLEN) == 0) { | ||
| iecho = (struct icmp_echo_hdr *)p->payload; | ||
|
|
||
| if ((iecho->id == 0xAFAF) && (iecho->seqno == lwip_htons(request->seqNum))) { | ||
| /* do some ping result processing */ | ||
| request->endMillis = millis(); | ||
| pbuf_free(p); | ||
| return 1; /* eat the packet */ | ||
| } | ||
| /* not eaten, restore original packet */ | ||
| pbuf_add_header(p, PBUF_IP_HLEN); | ||
| } |
There was a problem hiding this comment.
I would rewrite the logic of this function, to improve its readability by applying the return early pattern.
| if ((p->tot_len >= (PBUF_IP_HLEN + sizeof(struct icmp_echo_hdr))) && | |
| pbuf_remove_header(p, PBUF_IP_HLEN) == 0) { | |
| iecho = (struct icmp_echo_hdr *)p->payload; | |
| if ((iecho->id == 0xAFAF) && (iecho->seqno == lwip_htons(request->seqNum))) { | |
| /* do some ping result processing */ | |
| request->endMillis = millis(); | |
| pbuf_free(p); | |
| return 1; /* eat the packet */ | |
| } | |
| /* not eaten, restore original packet */ | |
| pbuf_add_header(p, PBUF_IP_HLEN); | |
| } | |
| if (p->tot_len < (PBUF_IP_HLEN + sizeof(struct icmp_echo_hdr)) || | |
| pbuf_remove_header(p, PBUF_IP_HLEN) != 0) { | |
| return 0; /* do not consume the packet */ | |
| } | |
| iecho = (struct icmp_echo_hdr *)p->payload; | |
| if (iecho->id != request->id || iecho->seqno != lwip_htons(request->seqNum)) { | |
| /* not consumed, restore original packet */ | |
| pbuf_add_header(p, PBUF_IP_HLEN); | |
| return 0; | |
| } | |
| /* do some ping result processing */ | |
| request->endMillis = millis(); | |
| pbuf_free(p); | |
| return 1; /* consume the packet */ |
There was a problem hiding this comment.
It would also make sense to check the ip header for the protocol field to match the value 1 reserved for ICMP packets, instead of checking for the size of the pbuf.
Reference rfc790 and Iana IP Protocol Numbers
pennam
force-pushed
the
add-ping-command
branch
2 times, most recently
from
d81171e to
55ea2d0
Compare
This ensure access to existing data if icmp callback is fired after ping timeout
Add the Ping command for the network interfaces (WiFi and Ethernet) of Portenta C33