Port ethernet to EMAC #4918

KariHaapalehto · 2017-08-16T07:47:59Z

Updating EMAC to enable multiple interfaces
Untangling networking classes, making the abstractions a bit clearer to follow, etc
General refactoring
Removal of DEVICE_EMAC flag and introducing DEVICE_ETH and DEVICE_WIFI

Status

IN DEVELOPMENT

Updating EMAC to enable multiple interfaces Untangling networking classes, making the abstractions a bit clearer to follow, etc General refactoring Removal of DEVICE_EMAC flag and introducing DEVICE_ETH and DEVICE_WIFI

Remove lwip depencies

0xc0170 · 2017-08-17T14:21:58Z

cc @bulislaw @geky @kjbracey-arm @hasnainvirk

ipstack_lwip.c renamed as mbed_ipstack.c

geky · 2017-08-18T19:08:52Z

features/netsocket/mbed_ipstack.c

+
+#ifdef __cplusplus
+}
+#endif


Does this file compile without LWIP?

geky · 2017-08-18T19:11:13Z

@KariHaapalehto, @bulislaw
I think I'm a bit confused. Is this pr related to #4079?

KariHaapalehto · 2017-08-21T07:12:16Z

@geky , yes this is related to #4079, more precisely ONME-3145

mbed_ipstac.c renamed back to ipstack_lwip.c and moved to back \FEATURE_LWIP\lwip-interface

geky

Looks ok to me, although this should really be reviewed by @bulislaw on our side

geky · 2017-08-21T17:57:19Z

doxyfile_options

@@ -2061,7 +2061,7 @@ PREDEFINED             = DOXYGEN_ONLY            \
                         DEVICE_ANALOGOUT        \
                         DEVICE_CAN              \
                         DEVICE_ETHERNET         \
-                         DEVICE_EMAC             \


Don't we need to keep DEVICE_EMAC around for backwards compatibility until all devices are ported over to DEVICE_ETH?

geky · 2017-08-21T18:05:46Z

features/netsocket/EthernetInterface.cpp

    strncpy(_gateway, gateway ? gateway : "", sizeof(_gateway));
-    _gateway[sizeof(_gateway) - 1] = '\0';


Why were these removed? strncpy doesn't garuntee a null-terminator:
http://www.cplusplus.com/reference/cstring/strncpy/

geky · 2017-08-21T18:06:53Z

features/netsocket/EthernetInterface.cpp

-    return mbed_lwip_bringup_2(_dhcp, false,
+    nsapi_error_t err;
+    if (_stack.emac == NULL)
+        return NSAPI_ERROR_UNSUPPORTED;


nit: style should have brackets for all if statements:

if (_stack.emac == NULL) { return NSAPI_ERROR_UNSUPPORTED; }

geky · 2017-08-21T18:07:07Z

features/netsocket/EthernetInterface.cpp

+    mbed_ipstack_init();
+    err = mbed_ipstack_add_netif(_stack.emac, true);
+    if (err != NSAPI_ERROR_OK)
+        return err;


nit: brackets on if statements

geky · 2017-08-21T18:09:21Z

features/netsocket/mbed_ipstack.h

+ *
+ * @return    NSAPI_ERROR_OK on success, or error code
+ */
+void mbed_ipstack_init(void);


Documentation doesn't seem to match return type, should this return nsapi_error_t?

geky · 2017-08-21T18:10:24Z

features/netsocket/mbed_ipstack.h

+ * @param    default_if true if the interface should be treated as the default one
+ * @return              NSAPI_ERROR_OK on success, or error code
+ */
+nsapi_error_t mbed_ipstack_add_netif(emac_interface_t *emac, bool default_if);


Should this be "add_netif" still? netif is the lwip term, maybe "add_interface"?

geky · 2017-08-21T18:16:50Z

hal/emac_api.h

+ * @param emac     Emac interface
+ * @param address  An multicast group IPv4 address
+ */
+typedef void (*emac_add_multicast_group)(emac_interface_t *emac, uint8_t *address);


Is this taking in a string representation of an IP address or a byte representation? Should it actually be taking in the nsapi_addr_t struct?

… at K64F Change "uint8_t *addr" to "uint8_t addr[ETH_HWADDR_LEN]" at the emac_api.h Correct reset at emac_lwip.c

KariHaapalehto · 2017-08-25T07:23:34Z

Changes has been compiled with gcc_arm compiler targeting for k64f with following configurations:
configs/eth_v4.json
configs/eth_v6.json
configs/wifi_esp8266_v4.json
configs/mesh_6lowpan.json
configs/mesh_6lowpan_subg.json
configs/mesh_thread.json

sys_sem_* replaced with osSemaphore* so that \arch\sys_arch.h can be removed from emac_api.h

theotherjimmy · 2017-08-29T18:24:05Z

@KariHaapalehto It looks like you broke doxygen. Could you fix that error (it's in travis)?

without lwip Doxygen warning corrected

0xc0170 · 2017-08-31T11:04:17Z

.\mbed-os\features\netsocket\mbed_ipstack.c:8:22: fatal error: ns_trace.h: No such file or directory uvisor failure,please fix

hasnainvirk · 2017-09-01T07:11:58Z

@KariHaapalehto A general comment, I think you should squash one-liners into previous commits. Maybe its worth a try to actually go through your commits and squash them together if necessary.

0xc0170 · 2017-09-04T15:16:06Z

Please look at CI failures (both uvisor and jenkins CI report similar failures with includes not found)

Move emac_interface_t from emac_api.h to mbed_ipstack_lwip.h and rename it to mbed_ipstack_interface emac_interface_ops_t, mbed_ipstack_interface_t and _hw added to EthernetInterface.h and _stack removed. ipstack_lwip.c renamed as mbed_ipstack_lwip.c

kjbracey · 2017-09-26T12:22:44Z

Work picked up in #5202 - I think this can be closed?

Work picked up by Kari Haapalehto in ARMmbed#4918, from Bartek Szatkowsi's branch at ARMmbed#4079, commit 8c28917. Bartek's summary: * Porting ethernet to EMAC * Updating EMAC to enable multiple interfaces * Untangling networking classes, making the abstractions a bit clearer to follow, etc * General refactoring * Removal of DEVICE_EMAC flag and introducing DEVICE_ETH and DEVICE_WIFI Revisions since initial branch: * Refactor k64f_emac * Remove lwip depencies * Correct doxygen warnings * Add weak definitions for mbed_ipstack.h functions. * Remove emac_interface_t - drivers register with emac_interface_ops_t and void *, and get an mbed_ipstack_interface_t to pass to stack. * Reinstate use of EthInterface abstraction * ipstack_lwip.c renamed as mbed_ipstack_lwip.c * Correct and clarify HW address EMAC ops * Restore MBED_MAC_ADDR implementation * Integrate PPP support with mbed_ipstack_lwip.cpp

0xc0170 · 2017-09-26T13:58:47Z

I'll close this, please reopen if should not be.

KariHaapalehto added 2 commits August 16, 2017 10:45

Porting ethernet to EMAC

2bdd39b

Updating EMAC to enable multiple interfaces Untangling networking classes, making the abstractions a bit clearer to follow, etc General refactoring Removal of DEVICE_EMAC flag and introducing DEVICE_ETH and DEVICE_WIFI

Refactor k64f_emac

2016e49

Remove lwip depencies

0xc0170 added the needs: review label Aug 17, 2017

Remove netif.h from emac_api.h

605e32b

ipstack_lwip.c renamed as mbed_ipstack.c

SeppoTakalo requested review from kjbracey and mikaleppanen August 18, 2017 13:04

geky reviewed Aug 18, 2017

View reviewed changes

features/netsocket/mbed_ipstack.c Outdated

#ifdef __cplusplus

}

#endif

Copy link

Contributor

geky Aug 18, 2017

Choose a reason for hiding this comment

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

Does this file compile without LWIP?

Revert ipstack_lwip.c renaming.

3aae01c

mbed_ipstac.c renamed back to ipstack_lwip.c and moved to back \FEATURE_LWIP\lwip-interface

theotherjimmy changed the title ~~Porting ethernet to EMAC~~ Port ethernet to EMAC Aug 21, 2017

geky reviewed Aug 21, 2017

View reviewed changes

SeppoTakalo self-requested a review August 23, 2017 07:19

KariHaapalehto changed the base branch from master to feature-emac August 23, 2017 07:38

Review changes and IPV6 compiler corrections

db59fed

adbridge requested a review from bulislaw August 24, 2017 11:28

Define emac_interface_t as const at the emac_api.h and take it to use…

062ce13

… at K64F Change "uint8_t *addr" to "uint8_t addr[ETH_HWADDR_LEN]" at the emac_api.h Correct reset at emac_lwip.c

KariHaapalehto added 3 commits August 25, 2017 15:53

Remove sys_arch.h from emac_api.h

638888d

sys_sem_* replaced with osSemaphore* so that \arch\sys_arch.h can be removed from emac_api.h

Correct doxygen warnings

1d7a5f3

Remove lwip depency from EthernetInterface.h

fdf190e

theotherjimmy added needs: work and removed needs: review labels Aug 29, 2017

KariHaapalehto added 2 commits August 30, 2017 10:55

Temporary file mbed_iptack.c created, so that mbed-os can be compiled

185435e

without lwip Doxygen warning corrected

Add error traces to mbed_ipstack.c

743700c

Include ns_trace.h changed mbed_trace.h

a562087

kjbracey mentioned this pull request Sep 26, 2017

Networking update: general refactoring, unifying EMAC #5202

Closed

0xc0170 closed this Sep 26, 2017

0xc0170 removed the needs: work label Sep 26, 2017

		strncpy(_gateway, gateway ? gateway : "", sizeof(_gateway));
		_gateway[sizeof(_gateway) - 1] = '\0';

Port ethernet to EMAC #4918

Port ethernet to EMAC #4918

Uh oh!

Conversation

KariHaapalehto commented Aug 16, 2017 • edited by SeppoTakalo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Status

Uh oh!

0xc0170 commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Aug 18, 2017

Uh oh!

KariHaapalehto commented Aug 21, 2017

Uh oh!

geky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KariHaapalehto commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Aug 29, 2017

Uh oh!

0xc0170 commented Aug 31, 2017

Uh oh!

hasnainvirk commented Sep 1, 2017

Uh oh!

0xc0170 commented Sep 4, 2017

Uh oh!

kjbracey commented Sep 26, 2017

Uh oh!

0xc0170 commented Sep 26, 2017

Uh oh!

Uh oh!

KariHaapalehto commented Aug 16, 2017 •

edited by SeppoTakalo

Loading

0xc0170 commented Aug 17, 2017 •

edited

Loading

geky left a comment •

edited

Loading

KariHaapalehto commented Aug 25, 2017 •

edited

Loading