Fixing Static EthernetInterface IP #1645
Conversation
Prior to this, the ethernet init method always returned 0, which meant we didn't know if the interface had initialised correctly or if it had failed.
Prior to this change, trying to use a static IP address caused the call to EthernetInterface::connect() to fail (timeout). This was due to a race condition between the thread that added the interface with lwip's netif_add and the ethernet driver (in my case the k64f). The ethernet driver eventually tries to call the callback set for the netif by netif_set_link_callback, but this is only set after the netif_add call. If the netif_add_call thread hasn't been scheduled yet, the callback does nothing, meaning we wait for the netif_linked semaphore until it times out. In the case of a static IP, the race condition ensured that the interface didn't work.
|
Just realised I didn't commit the driver change. Will do so now. |
The previous value of 0x10000 led to a timeout that was over a minute long on the K64F platform, which is too long for many use cases. This has been changed to 0x800, which represents around 2 seconds on the K64F.
|
+1 This looks fine to me and should be backwards compatible |
| netif_add(&netif, ipaddr, netmask, gw, NULL, eth_arch_enetif_init, tcpip_input); | ||
| struct netif * ret; | ||
| ret = netif_add(&netif, ipaddr, netmask, gw, NULL, eth_arch_enetif_init, tcpip_input); | ||
| if( ret == NULL) { |
There was a problem hiding this comment.
please fix a space -> if(ret == NULL), use rebase or I'll do it when merging
| @@ -92,12 +94,15 @@ int EthernetInterface::init() { | |||
| return init_netif(NULL, NULL, NULL); | |||
| } | |||
|
|
|||
| int EthernetInterface::init(const char* ip, const char* mask, const char* gateway) { | |||
| int EthernetInterface::init(const char* ip, const char* mask, const char* gateway, const char * dns) { | |||
There was a problem hiding this comment.
This API change appears unrelated to the issue being discussed. IMHO it would be better as a separate PR.
|
Can one of the admins verify this patch? |
|
@geky Can you help out with this PR ? |
|
@geky any help here? |
|
I'm not really sure what the state of this pr is. @0xc0170 and @infinnovation have good comments that should be taken care of. Outside of those are there other concerns with this patch? |
|
@geky Looks like we have to take this patch, rebase and update it. |
|
@Eosis Lots has happened around networking since this patch. Please clean up and re-open if still valid |
When trying to add a static IP to the EthernetInterface I found that it wasn't possible, due to a lengthy timeout in the K64F phy drivers and a race condition exhibited by the EthernetInterface class itself.
I changed the timeout in the driver and proposed a fix for the EthernetInterface lock up.