-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Driver checksum support for ICMP ping reply #92
Closed
Closed
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 avoidunused variable
compiler warning.Your code should look something like this after the changes:
Thanks
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
:
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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :
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.
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...
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.