Fix MPS2 CM3DS ethernet driver #15284
Conversation
ciarmcom
added
the
release-type: patch
|
@ramielkhatibPQS, thank you for your changes. |
|
Can you create 4 commits based on the description your provided? Each of the item in the list you have should be own commit (it will be helpful and simplify our review). |
mergify
bot
removed
needs: review
devices: arm
component: connectivity
release-type: patch
|
I updated the PR to use 4 commits as requested. Please review. Thank you! |
|
Thank you, easier to review now. What we are missing are the details from here in the commit messages (the subjects are there but can you add additional info from the description in this pull request to the commit messages?). it's best to have them along the code than only here on Github. |
|
Did you run any networking tests or how was this tested? |
0xc0170
added
needs: review
release-type: patch
The sleep_for function is updated to use the chrono time arguments since the regular ones are deprecated.
The function SMSC9220_EMAC::low_level_input should create a heap for the packet equal to the size of the message (most of which are couple hundred bytes). The current code uses maximum frame size (1522 bytes) for each packet. This will cause the heap to quickly fill up. In fact, the default memory size (lwip.mem-size) used for this heap is 1600 bytes. This means that once you have one other packet allocated (extremely common), the heap allocation will always fails. Also, it is recommend to increase the default lwip.mem-size because that amount is very small especially if you send or receive a few large packets in the network. This is NOT done in current commit.
The function smsc9220_receive_by_chunks loads data from the Ethernet port. It is expected to return the Ethernet frame without the 4 CRC bytes. However, it is required to call the Ethernet data port register (32-bit) an amount equal to the number of frame words (including the 4 CRC bytes) to pop all frame words. The current code doesn't call the register for the last word (which has CRC data). This causes subsequent calls to have this missed word at the beginning. The impact of this is huge as the high level API is getting fed wrong data. The fix adds one additional call to the data port register.
The functions smsc9220_receive_by_chunks and smsc9220_send_by_chunks are supposed to implement receiving and sending packets by chunks. However, the functions SMSC9220_EMAC::low_level_input and SMSC9220_EMAC::link_out, which call them respectively, already require or assemble the full packet. Also, smsc9220_receive_by_chunks doesn't implement the "chunks" part. This commit renames the functions to smsc9220_receive_packet and smsc9220_send_packet. The functions now do their operations by word instead of by bytes. The functions SMSC9220_EMAC::low_level_input and SMSC9220_EMAC::link_out already handle allocation, continuity and word alignment of the packet buffer.
|
I updated the commits with the description. For the tests, here is my setup:I connected a PC and the MPS2+ directly through an Ethernet cable to isolate their network. For the PC side:
For the MPS2+ side:I had the CM3DS flashed on the FPGA. I ran this simple code which should link the two devices and attempt to acquire an IP address through DHCP. #include "mbed.h"
#include "EthernetInterface.h"
EthernetInterface net;
int main()
{
int ret;
// Bring up the ethernet interface
printf("Ethernet socket example\n");
ret = net.connect();
printf("net.connect returned: %x\n", ret);
// Bring down the ethernet interface
ret = net.disconnect();
printf("net.disconnect returned: %x\n", ret);
return 0;
}To reproduce bug 1 (heap):The MPS2+ will send a DHCP discover and almost immediately get a DHCP offer. The heap allocation line (cbfda0e) will fail for the DHCP offer because there will not be 1522 bytes available in the heap memory (1600 bytes by default). Also, notice that the variable To reproduce bug 2 (packet one word short):Print the for(uint32_t i = 0; i < packet_length_byte; i++)
printf("%02x", data[i]);
printf("\n");The DHCP protocol should work as follows
The first packet DHCP OFFER will show up exactly like in wireshark. However the DHCP ACK packet will be off by 4 bytes in comparison with wireshark. |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
added
release-version: 6.16.0
Release-pending
and removed
release-type: patch
Summary of changes
The ethernet driver for the target MPS2 CM3DS has several issues. Some of these issues are severe for this target as the high level API function won't function properly. Here is the summary of changes in this PR:
Impact of changes
This PR will only affect users who are using MPS2 CM3DS. There is no implication for any other target. The current driver doesn't function properly, so this PR should actually fix ethernet issues for users of this target.
Migration actions required
Fix bugs for the ethernet driver of MPS2 CM3DS.
Documentation
Bug 1:
The function
SMSC9220_EMAC::low_level_inputshould create a heap for the packet equal to the size of the message (most of which are couple hundred bytes). The current code uses maximum frame size (1522 bytes) for each packet. This will cause the heap to quickly fill up. In fact, the default memory size (lwip.mem-size) used for this heap is 1600 bytes. This means that once you have one other packet allocated (extremely common), the heap allocation will always fails.Also, I recommend increasing the default
lwip.mem-sizebecause that amount is very small especially if you send or receive a few large packets in the network. This is NOT done in current PRBug 2:
The function
smsc9220_receive_by_chunksloads data from the Ethernet port. It is expected to return the Ethernet frame without the 4 CRC bytes. However, you are required to call the Ethernet data port register (32-bit) an amount equal to the number of frame words (including the 4 CRC bytes) to pop all frame words. The current code doesn't call the register for the last word (which has CRC data). This causes subsequent calls to have this missed word at the beginning. The impact of this is huge as the high level API is getting fed wrong data. I added a fix by adding one additional call. The function is also updated because of the improvement below.Improvement:
The functions
smsc9220_receive_by_chunksandsmsc9220_send_by_chunksare supposed to implement receiving and sending packets by chunks. However, the functionsSMSC9220_EMAC::low_level_inputandSMSC9220_EMAC::link_out, which call them respectively, already require or assemble the full packet. Also,smsc9220_receive_by_chunksdoesn't implement the "chunks" part.I renamed the functions to
smsc9220_receive_packetandsmsc9220_send_packet. The functions now do their operations by word instead of by bytes. The functionsSMSC9220_EMAC::low_level_inputandSMSC9220_EMAC::link_outalready handle allocation, continuity and word alignment of the packet buffer. Some of the change ideas are taken from here.Deprecation fix:
I updated the function calls for
sleep_forto use the chrono time arguments since the regular ones are deprecated.Pull request type
Test results
Reviewers