gnrc_netif2: Introduction of a new GNRC network interface API by miri64 · Pull Request #7370 · RIOT-OS/RIOT

miri64 · 2017-07-16T12:03:23Z

This unifies and consolidates the APIs of gnrc_netdev, gnrc_netif, gnrc_ipv6_netif, and gnrc_sixlowpan_netif. ~~Currently there are only header definitions defined to start some discussion~~. Basically instead of splitting up the data upon several structs they are unified in one, which is protected by a struct-wise (recursive) mutex. The mutex is necessary, since beside the interface's thread the 6LoWPAN thread and the IPv6 thread will access the struct to save some valuable time otherwise wasted in IPC. GNRC externally the interface is supposed to be accessed via NETAPI_GET and NETAPI_SET. The required netopts still need to be defined in part.

TODO:

unittests

This PR is part of the network layer remodelling effort:

miri64 · 2017-07-16T12:06:26Z

$ cloc $(git show --pretty="" --name-only)
       3 text files.
       3 unique files.                              
       0 files ignored.

http://cloc.sourceforge.net v 1.60  T=0.01 s (244.6 files/s, 76873.7 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                     2             73            654            195
C                                1              5             14              2
-------------------------------------------------------------------------------
SUM:                             3             78            668            197
-------------------------------------------------------------------------------

miri64 · 2017-07-17T15:39:43Z

Done with the implementation (I hope ^^). Now some unittests are missing. The stack integration and shell command I would prefer to do in a separate PR, since this PR already is quite large...

miri64 · 2017-07-25T20:51:21Z

(no longer WIP btw)

… into HEAD

aabadie

Briefly went though this one and found some issues with doxygen.
Will see if I can provide an adaption of #7356 in a separate PR.

aabadie · 2017-08-17T12:36:48Z

sys/include/net/gnrc/netif2.h

+     * @param[in] netif The network interface.
+     *
+     * This is called after the default settings were set, right before the
+     * interface's thread starts receiving messages. It is not necessary lock


'It doesn't necessary lock'

What? That doesn't make any sense.

'It is not necessary lock the interface's mutex gnrc_netif_t::mutex, since the thread will already lock it.'
I think there's a typo here, that's why I proposed:
'It doesn't necessary lock...'
Maybe some rephrasing is required.

But that negates the the sentence... Lock in is not required, since the thread will do that.

*locking

Mh... But if I wrote it like you quoted me (can't check, am on a phone), then a "to" is missing. Will fix if it is so.

"It doesn't necessary..." is in any way grammatically wrong ;)

+1 for the missing 'to'.

aabadie · 2017-08-17T12:38:46Z

sys/include/net/gnrc/netif2.h

+     *
+     * @return  The number of bytes actually sent on success
+     * @return  -EBADMSG, if the @ref net_gnrc_netif_hdr in @p pkt is missing
+     *          or of an unexpected format.


'or is in an unexpected format' ?

aabadie · 2017-08-17T12:40:08Z

sys/include/net/gnrc/netif2.h

+     *
+     * @pre `netif != NULL`
+     *
+     * @note The function takes the bytes received from via


'from via' sounds weird

aabadie · 2017-08-17T12:48:19Z

sys/include/net/gnrc/netif2.h

+    int (*get)(gnrc_netif2_t *netif, gnrc_netapi_opt_t *opt);
+
+    /**
+     * @brief   Get an option from the network interface


Should be 'Sets an option' ?
Next line should also be updated.

aabadie · 2017-08-17T12:49:10Z

sys/include/net/gnrc/netif2.h

+    gnrc_pktsnip_t *(*recv)(gnrc_netif2_t *netif);
+
+    /**
+     * @brief   Get an option from the network interface


aabadie · 2017-08-17T12:52:39Z

sys/include/net/gnrc/netif2/conf.h

+#endif
+
+/**
+ * @brief   Number of multicast addressed needed for @ref net_gnrc_rpl "RPL".


s/addressed/addresses/

aabadie · 2017-08-17T12:57:40Z

sys/include/net/gnrc/netif2/internal.h

+                              const ipv6_addr_t *addr);
+
+/**
+ * @brief   Get state from address flags


Gets ? This is no consistent in other places as well.

miri64 · 2017-08-17T17:30:24Z

Addressed @aabadie's comments.

miri64 · 2017-08-17T17:35:54Z

Rebased

bergzand

Not a lot of technical remarks, mostly spelling and/or grammar related comments.

If I've got the time later today, I'd like to look a bit better at gnrc_netif2.c.

bergzand · 2017-10-06T10:25:03Z

sys/include/net/gnrc/netif2.h

+     *
+     * @note The function re-formats the content of @p pkt to a format expected
+     *       by the netdev_driver_t::send() method of gnrc_netif_t::dev and
+     *       releases the it before returning (so no additional release should


"...releases the packet before..."

bergzand · 2017-10-06T10:35:26Z

sys/include/net/gnrc/netif2.h

+ *
+ * @return  @p out.
+ */
+char *gnrc_netif2_addr_to_str(const uint8_t *addr, size_t addr_len, char *out);


Does it make sense here to return an input parameter? Why not return the size of the formatted string?

strings are usually \0 terminate, so returning its length is not required (a non-fitting out is an error case) (this function is mostly used for debugging, so no machine short-cuts required).

bergzand · 2017-10-06T10:36:44Z

sys/include/net/gnrc/netif2/conf.h

+/**
+ * @brief   Maximum number of network interfaces
+ *
+ * @note    Intentially not calling it `GNRC_NETIF2_NUMOF` to not require


Intentionally

bergzand · 2017-10-06T10:43:33Z

sys/include/net/gnrc/netif2/conf.h

+#define GNRC_NETIF2_L2ADDR_MAXLEN   (IEEE802154_LONG_ADDRESS_LEN)
+#elif   MODULE_NETDEV_ETH
+#define GNRC_NETIF2_L2ADDR_MAXLEN   (ETHERNET_ADDR_LEN)
+#elif   MODULE_CC110X


It feels ugly to me to have a switch depending directly on a device driver. I don't know if there is a better solution for this though.

The CC110X has a proprietary protocol that needs adressing. I do not see it as a dependency on the device driver but on the protocol.

Never mind then :)

bergzand · 2017-10-06T10:57:12Z

sys/include/net/gnrc/netif2/flags.h

+ * @attention   GHC not implemented yet
+ * @see [RFC 7400, section 3.3](https://tools.ietf.org/html/rfc7400#section-3.3)
+ */
+#define GNRC_NETIF2_FLAGS_6LO_BACKBONE              (0x00000800U)


Shouldn't this be something like GNRC_NETIF2_FLAGS_6LO_GHC?

No, this has nothing to do with GHC, but with multi-border-router management. Will update the comment above accordingly.

Okay, I assumed the comment to be correct here.

bergzand · 2017-10-06T12:29:46Z

sys/net/gnrc/netif2/gnrc_netif2.c

+                                  ~GNRC_NETIF2_IPV6_ADDRS_FLAGS_STATE_MASK) |
+                                 GNRC_NETIF2_IPV6_ADDRS_FLAGS_STATE_VALID);
+                uint8_t pfx_len = (uint8_t)(opt->context >> 8U);
+                /* acquire locks a recursive mutex so we are save calling this


Should be "...are safe calling..."

bergzand · 2017-10-06T12:34:39Z

sys/net/gnrc/netif2/gnrc_netif2.c

+            break;
+        case NETOPT_IPV6_ADDR_REMOVE:
+            assert(opt->data_len == sizeof(ipv6_addr_t));
+            /* acquire locks a recursive mutex so we are save calling this


bergzand · 2017-10-06T12:34:47Z

sys/net/gnrc/netif2/gnrc_netif2.c

+            break;
+        case NETOPT_IPV6_GROUP:
+            assert(opt->data_len == sizeof(ipv6_addr_t));
+            /* acquire locks a recursive mutex so we are save calling this


bergzand · 2017-10-06T12:34:58Z

sys/net/gnrc/netif2/gnrc_netif2.c

+            break;
+        case NETOPT_IPV6_GROUP_LEAVE:
+            assert(opt->data_len == sizeof(ipv6_addr_t));
+            /* acquire locks a recursive mutex so we are save calling this


bergzand · 2017-10-06T12:50:21Z

sys/net/gnrc/netif2/gnrc_netif2_ethernet.c

+        }
+
+        if (nread < bytes_expected) {
+            /* we've got less then the expected packet size,


Should be "less than".

miri64 · 2017-10-06T14:42:19Z

Thanks for reviewing, @bergzand. I addressed your comments.

… into RIOT-OS#7404

bergzand · 2017-10-06T22:52:44Z

tests/gnrc_netif2/main.c

+static inline int _mock_netif_send(gnrc_netif2_t *netif, gnrc_pktsnip_t *pkt);
+static inline gnrc_pktsnip_t *_mock_netif_recv(gnrc_netif2_t * netif);
+static int _get_netdev_address(netdev_t *dev, void *value, size_t max_len);
+static int _set_netdev_address(netdev_t *dev, void *value, size_t value_len);


My compiler is complaining hard that value should be of type const void *.

bergzand · 2017-10-06T22:54:56Z

Nothing more to add except for my remark above. gnrc_netif2.c looks good to me.

miri64 · 2017-10-07T01:16:44Z

Addressed latest comments and rebased (otherwise the test wouldn't build locally on my machine ;-))

cgundogan · 2017-10-08T13:59:02Z

sys/include/net/gnrc/netif2/internal.h

+ */
+static inline void gnrc_netif2_acquire(gnrc_netif2_t *netif)
+{
+    if (netif && (netif->ops)) {


Strange to check for netif->ops here. It's not even used for acquiring a mutex. Good you leave a hint in the doc that netif->ops != NULL is necessary to acquire the mutex?

netif->ops is NULL before the interface was initialized. If the interface is not initialized it is not necessary to lock its mutex.

cgundogan · 2017-10-08T14:00:16Z

sys/include/net/gnrc/netif2/internal.h

+ */
+static inline void gnrc_netif2_release(gnrc_netif2_t *netif)
+{
+    if (netif && (netif->ops)) {


cgundogan · 2017-10-08T14:01:12Z

sys/include/net/gnrc/netif2/internal.h

+ * @param[in] flags     initial flags for the address.
+ *                      - Setting the address' state to
+ *                        @ref GNRC_NETIF2_IPV6_ADDRS_FLAGS_STATE_TENTATIVE
+ *                        means thata this address is assumed to be added due to


cgundogan · 2017-10-08T14:01:58Z

sys/include/net/gnrc/netif2/internal.h

+ *                      - Setting the address' state to
+ *                        @ref GNRC_NETIF2_IPV6_ADDRS_FLAGS_STATE_TENTATIVE
+ *                        means thata this address is assumed to be added due to
+ *                        state-less auto-address configuration and actions


state-less => stateless

cgundogan · 2017-10-08T14:10:59Z

sys/net/gnrc/netif2/gnrc_netif2.c

+                                        const ipv6_addr_t *dst,
+                                        uint8_t *candidate_set)
+{
+    /* create temporary set for assigning "points" to candidates wining in the


cgundogan · 2017-10-09T07:26:11Z

sys/include/net/gnrc/netif2/ipv6.h

+     *
+     * @see net_gnrc_netif2_ipv6_addrs_flags
+     *
+     * @note    Only available with module @ref net_gnrc_ipv6 "gnrc_ipv6".


Only available with module @ref net_gnrc_ipv6 "gnrc_ipv6".

shouldn't there be a guard around those struct members if they are only available with gnrc_ipv6?

There is, not just around the single struct members, but around the whole struct ;-).

cgundogan · 2017-10-09T12:47:55Z

I have no major issues besides those minor – mostly editorial – comments above. I think we can merge here as soon as they are addressed.

miri64 · 2017-10-09T13:54:19Z

Addressed comments

cgundogan · 2017-10-09T14:20:50Z

Please squash!

miri64 · 2017-10-10T08:36:39Z

Squashed and rebased.

… into RIOT-OS#7404

… into RIOT-OS#7409

miri64 · 2017-10-10T08:48:29Z

Murdock likes it. @cgundogan or @aabadie, can I have an ACK? :)

miri64 · 2017-10-10T08:49:32Z

(3 people - 4 if I count @haukepetersen's inofficial offline review - looked at the code and I addressed all their comments, so I assume the PR is now ACK-worthy ;-))

… into RIOT-OS#7410

cgundogan

ACK

cgundogan · 2017-10-10T09:01:40Z

@aabadie your comments have been addressed more than a month ago – I will dismiss your change request now to continue with merging (:

@miri64

Comments have been addressed by @miri64

miri64 added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jul 16, 2017

miri64 assigned cgundogan and BytesGalore Jul 16, 2017

miri64 requested a review from cgundogan July 16, 2017 12:03

miri64 force-pushed the gnrc_netif2/feat/new-netif-api-for-gnrc branch from d848738 to e5ffd7e Compare July 17, 2017 15:38

miri64 force-pushed the gnrc_netif2/feat/new-netif-api-for-gnrc branch from e5ffd7e to 732bf5b Compare July 17, 2017 17:01

miri64 mentioned this pull request Jul 17, 2017

drivers/sx127x, sys: configure auto_init and improve netif adaption #7356

Closed

miri64 force-pushed the gnrc_netif2/feat/new-netif-api-for-gnrc branch 2 times, most recently from db3cc0a to 0ac7db2 Compare July 24, 2017 14:48

This was referenced Jul 25, 2017

shell_commands: port ifconfig command for gnrc_netif2 #7404

Merged

gnrc_netif2: add IEEE 802.15.4 support #7409

Merged

auto_init: upgrade bootstrap of Ethernet devices for gnrc_netif2 #7410

Merged

miri64 changed the title ~~gnrc_netif2: Introduction of a new GNRC network interface API [WIP/RFC]~~ gnrc_netif2: Introduction of a new GNRC network interface API [RFC] Jul 25, 2017

miri64 added GNRC Area: network Area: Networking and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jul 25, 2017

This was referenced Jul 26, 2017

cc110x: port to gnrc_netif2 #7414

Merged

nrfmin: port for gnrc_netif2 #7417

Merged

pkg: port nordic_softdevice_ble to gnrc_netif2 #7419

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 8, 2017

Merge branch 'gnrc_netif2/feat/new-netif-api-for-gnrc' (RIOT-OS#7370)…

4f6d4b5

… into HEAD

miri64 mentioned this pull request Aug 8, 2017

gnrc_netif2: provide raw adaption layer (for e.g. SLIP) #7462

Merged

aabadie previously requested changes Aug 17, 2017

View reviewed changes

miri64 force-pushed the gnrc_netif2/feat/new-netif-api-for-gnrc branch from c40338f to 943aab0 Compare August 17, 2017 17:35

miri64 force-pushed the gnrc_netif2/feat/new-netif-api-for-gnrc branch from f2d599b to d53ffd3 Compare August 18, 2017 15:35

bergzand reviewed Oct 6, 2017

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 6, 2017

Merge branch 'gnrc_netif2/feat/new-netif-api-for-gnrc' (RIOT-OS#7370)…

0c224e3

… into RIOT-OS#7404

bergzand reviewed Oct 6, 2017

View reviewed changes

miri64 force-pushed the gnrc_netif2/feat/new-netif-api-for-gnrc branch from bfa8731 to 173756a Compare October 7, 2017 01:16

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 7, 2017

cgundogan reviewed Oct 8, 2017

View reviewed changes

cgundogan reviewed Oct 9, 2017

View reviewed changes

miri64 added 2 commits October 10, 2017 10:36

gnrc_netif2: Introduction of a new GNRC network interface API

0b80c7e

tests: add tests for gnrc_netif2

77d9770

miri64 force-pushed the gnrc_netif2/feat/new-netif-api-for-gnrc branch from ae88270 to 77d9770 Compare October 10, 2017 08:36

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2017

Merge branch 'gnrc_netif2/feat/new-netif-api-for-gnrc' (RIOT-OS#7370)…

6b9dbe9

… into RIOT-OS#7404

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2017

Merge branch 'gnrc_netif2/feat/new-netif-api-for-gnrc' (RIOT-OS#7370)…

8cc8c0a

… into RIOT-OS#7409

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2017

Merge branch 'gnrc_netif2/feat/new-netif-api-for-gnrc' (RIOT-OS#7370)…

7d897c4

… into RIOT-OS#7410

cgundogan approved these changes Oct 10, 2017

View reviewed changes

cgundogan merged commit d51c058 into RIOT-OS:master Oct 10, 2017

miri64 deleted the gnrc_netif2/feat/new-netif-api-for-gnrc branch October 10, 2017 09:03

miri64 mentioned this pull request Oct 9, 2019

gnrc/netif: don't treat link local differently in prefix matching #12404

Closed

Conversation

miri64 commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Jul 16, 2017

Uh oh!

miri64 commented Jul 17, 2017

Uh oh!

miri64 commented Jul 25, 2017

Uh oh!

aabadie left a comment

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!

miri64 Aug 17, 2017 • 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!

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!

miri64 commented Aug 17, 2017

Uh oh!

miri64 commented Aug 17, 2017

Uh oh!

bergzand left a comment

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!

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!

miri64 commented Jul 16, 2017 •

edited

Loading

miri64 Aug 17, 2017 •

edited

Loading