Driver checksum support for ICMP ping reply #92
Conversation
| @@ -1844,6 +1844,9 @@ uint8_t ucProtocol; | |||
|
|
|||
| usRequest = ( uint16_t ) ( ( uint16_t )ipICMP_ECHO_REQUEST << 8 ); | |||
There was a problem hiding this comment.
Hello,
Thanks for contributing to FreeRTOS @Spadi0.
Can you move this statement (along with the comment on line #1843) inside the #else as well?
Because in this case, the compiler will generate a warning saying the operation result is unused if ( ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM != 1 ).
Additionally, you might want to either add ( void ) usRequest; to the #if block. Again to avoid unused variable compiler warning.
Your code should look something like this after the changes:
#if ( ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM == 1 )
( void ) usRequest;
pxICMPHeader->usChecksum = 0;
#else
/* due to compiler warning "integer operation result is out of range" */
usRequest = ( uint16_t ) ( ( uint16_t )ipICMP_ECHO_REQUEST << 8 );
if( pxICMPHeader->usChecksum >= FreeRTOS_htons( 0xFFFFU - usRequest ) )
{
pxICMPHeader->usChecksum = pxICMPHeader->usChecksum + FreeRTOS_htons( usRequest + 1U );
}
else
{
pxICMPHeader->usChecksum = pxICMPHeader->usChecksum + FreeRTOS_htons( usRequest );
}
#endif
Thanks
There was a problem hiding this comment.
Also, @htibosch, can you have a look at this as well?
There was a problem hiding this comment.
Hello Sam, I can not replicate the problem you mention both on a STM32F407 DISCO and a STM32F746G DISCO board.
I don't have a F429, but I think that the EMAC peripheral is the same,
I did this test both with ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM enabled and disabled.
@Aniruddha: your patch is correct, but this is the responsibility of the NetworkInterface. You can see it here around line 657:
:
if( pxPacket->xICMPPacket.xIPHeader.ucProtocol == ( uint8_t ) ipPROTOCOL_ICMP )
{
pxPacket->xICMPPacket.xICMPHeader.usChecksum = ( uint16_t )0u;
}Many EMACS require that the value of the ICMP checksum field is set to 0x00.
Some EMACS don't do ICMP checksums and they depend on the code you disabled here above.
@Spadi0 Now what makes you think that the ICMP checksum on the wire is not correct?
If you saw that in WireShark, you might be mislead. Most PC's and laptops have checksum offloading activated, and therefore Wireshark doesn't get to see the original checksum.
While developing, I recommend to switch off checksum offloading in the laptop and let Wireshark verify all checksums.
If you want you can attach (a zipped) PCAP file to your post.
There was a problem hiding this comment.
stm32f429_icmp.zip
Hi @htibosch,
I do see what you mean about some EMACs requiring the checksum value. My bad. However, I am not sure what to change it to. I feel that a new config option would need to be added.
About me being sure the checksum was invalid: it initially came about when I attempted to ping the device using my computer, and got no reply. After looking at Wireshark, it was saying the checksum was invalid - I now realise the checksum was being offloaded (as it was set to 0x00). After implementing the change which was commited, it started working, and I thought that the EMAC was not bothering to calculate a checksum as one was already inserted.
As it would turn out, my reasoning was wrong, but my solution wasn't. After disabling checksum offloading, Wireshark now displays a checksum (which is invalid). I have attached the zipped PCAP file for you.
As for you not being able to replicate this issue, I am not sure what to say. I am using the STM32429I-EVAL1, in case it is relevent. It is worth noting that I did not use the included STM32F4 NetworkInterface (as I was intentionally not using the STM32 HAL for this project, and thought it would be a good learning experience), and wrote my own. It functions fine (as far as I can tell, at least), but there may be some configuration option that the HAL uses which I don't in my own driver.
I feel it is still worth exploring this issue though, because like you said, many EMACs require the ICMP checksum field to be set to 0x00, and while it can be implemented in the driver (which could be done fairly easily by using the already provided NetworkInterface files as reference), this doesn't seem to be documented anywhere. And even if it was, it would be easier for an end user to add a configuration option than to implement it themselves.
I would be happy to do some more work on this, just require the guidance and direction.
Many thanks,
Sam
There was a problem hiding this comment.
I do see what you mean about some EMACs requiring the checksum
value. My bad. However, I am not sure what to change it to.
I feel that a new config option would need to be added.
I don't think so, your NetworkInterface.c can solve this, as in the example that I showed.
I also have a NetworkInterface for STMFxx that doesn't make use of HAL but the so-called standard-peripheral driver in stead. Is that what you did as well?
Most users just grab a NetworkInterface and they expect it to work without a minimum of configuration. Remember that FreeRTOSIPConfig.h must be maintained by the users.
I found several types of behaviour:
IP, TCP and UDP :
- checksum offloading work with any preparation.
ICMP :
- some EMAC works without any preparation
- most EMAC's ( STM32Fx ) expect us to clear the checksum-field
- some EMAC's can not do ICMP
When you look at the existing drivers, they all make sure that the ICMP checksum will be set, without depending on an extra macro.
You can look at the STM32Fxx driver to see how it was solved for the STM32F platform.
But if you want, you can send or share your driver and I will look at it.
There was a problem hiding this comment.
Is not the main point of checksum offloading offload the mcu and let it the hardware calculate checksum instead?
The example proposed is more a workaround than a solution. The target calculate then clear and after recalculate the checksum in hardware.
However i am not as good to code in a performat way :-( so is performance not an issue in my case...
Most users just grab a NetworkInterface and they expect it to work without a minimum of configuration. Remember that FreeRTOSIPConfig.h must be maintained by the users.
I am new to freeRTOS and creating FreeRTOSIPConfig.h was not easy. The main concern is however a missing template file as stated in #65. I don' think that adding some more macro, if well documented, will add difficulty.
See issue FreeRTOS#87 and FreeRTOS#92.
|
Hello @Spadi0, it seems that @htibosch has suggested a fix for the problem being pointed out by you. @Spadi0, Thanks again for pointing out the issue and taking the time to suggest and discuss a potential fix. I shall close this PR now. |
Description
When using
ipconfigREPLY_TO_INCOMING_PINGS, it does not currently support letting the driver calculate the ICMP checksum, even whenipconfigDRIVER_INCLUDED_TX_IP_CHECKSUMis set to 1. It fills up the ICMP checksum field with data, which messes with the hardware checksum calculation (at least, it did for me whilst using an STM32F429).Test Steps
ipconfigREPLY_TO_INCOMING_PINGSto 1Related Issue
I found an issue that was already open, created by someone else (#87).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.