Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Driver checksum support for ICMP ping reply #92

Closed
wants to merge 3 commits into from
Closed

Driver checksum support for ICMP ping reply #92

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ad7ea1
Driver checksum support for ICMP ping reply
Spadi0 Jun 17, 2020
fbc6a6a
Merge branch 'master' into master
cobusve Sep 28, 2020
f44911a
Merge branch 'master' into master
AniruddhaKanhere Oct 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/FreeRTOS_IP.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1947,6 +1947,9 @@ uint8_t ucProtocol;

usRequest = ( uint16_t ) ( ( uint16_t )ipICMP_ECHO_REQUEST << 8 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @htibosch, can you have a look at this as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Spadi0 Spadi0 Jun 23, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@e-sr e-sr Jun 30, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#if ( ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM == 1 )
pxICMPHeader->usChecksum = 0;
#else
if( pxICMPHeader->usChecksum >= FreeRTOS_htons( 0xFFFFU - usRequest ) )
{
pxICMPHeader->usChecksum = pxICMPHeader->usChecksum + FreeRTOS_htons( usRequest + 1U );
Expand All @@ -1955,6 +1958,7 @@ uint8_t ucProtocol;
{
pxICMPHeader->usChecksum = pxICMPHeader->usChecksum + FreeRTOS_htons( usRequest );
}
#endif
return eReturnEthernetFrame;
}

Expand Down