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
4 participants
Add the Ping command for the network interfaces (WiFi and Ethernet) of Portenta C33