net/gnrc_netif: add bus for interface events, gnrc_netif_up()/gnrc_netif_down() functions #17902

benpicco · 2022-04-04T15:31:22Z

Contribution description

It can be useful to know when an interface is up / down, e.g. to (re)start or stop the DHCP client.

This adds a bus to gnrc netif where users interested in interface events (up/down) can subscripe.

It also adds convenience functions gnrc_netif_up(), gnrc_netif_down() that block until the interface event is generated (or timeout).

Testing procedure

Issues/PRs references

includes #17893

jia200x · 2022-04-05T09:56:39Z

The approach makes sense to me and can be easily extended to other netif link layers (e.g 802.15.4).
Let's get #17893 first and then proceed with this one

fabian18

first round of comments :)

sys/net/gnrc/netif/gnrc_netif.c

fabian18 · 2022-09-21T09:16:09Z

sys/net/gnrc/netif/gnrc_netif.c

+    msg_bus_subscribe(&sub, GNRC_IPV6_EVENT_ADDR_VALID);
+#endif
+
+    while (gnrc_netapi_set(netif->pid, NETOPT_STATE, 0, &state, sizeof(state)) == -EBUSY) {}


Must it be NETOPT_STATE or can it be NETOPT_LINK? Because our ifconfig <i> {up | down} command generates NETOPT_LINK.

Uh interesting.
My use case was setting STATE_SLEEP which should also generate a link down event…

NETOPT_LINK is read-only on all 3 drivers that implement it.

sys/net/gnrc/netif/gnrc_netif.c

benpicco · 2022-10-18T15:04:50Z

With 8de8d05 we can now also make use of the bus mechanism in gnrc_ipv6_nib.

This makes the gnrc_netif_bus module mandatory and drops the GNRC_IPV6_NIB_IFACE_UP/DOWN events as they are no longer needed. This unfortunately adds ~400 bytes to the ROM.

To register with the interface events, we have to wait for the interfaces to be initialized. Especially on esp32 this can take surprisingly long.

If you don't agree with this, I can drop the commit.

riot-ci · 2022-10-20T20:09:14Z

Murdock results

❌ FAILED

8de8d05 gnrc_ipv6_nib: make use of gnrc_netif_bus for interface events

Success	Failures	Total	Runtime
371	0	2006	01m:37s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

maribu

lgtm. Some comments inline.

I think the opportunistic approach to wait for interfaces to become ready is a dragon that at least should be documented.

maribu · 2023-01-10T09:39:04Z

sys/net/gnrc/netif/gnrc_netif.c

+    if ((state == NETOPT_STATE_SLEEP) || (state == NETOPT_STATE_OFF) ||
+        (state == NETOPT_STATE_STANDBY)) {
+        event = GNRC_NETIF_EVENT_LINK_STATE_CHANGED_DOWN;
+    } else {


Suggested change

} else {

}

else {

maribu · 2023-01-10T10:18:46Z

sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

+    DEBUG_PUTS("ipv6: wait for interfaces to be ready");
+    while (gnrc_netif_iter(NULL) == NULL) {
+        ztimer_sleep(ZTIMER_MSEC, 250);
+    }


This looks fragile. But I also do not know any way to reliably wait for interface registration to complete off the top of my head or even if such feature exists :/

The interfaces could send a message when they are initialized, then we can handle the registration here.

maribu · 2023-01-10T10:19:18Z

sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

                gnrc_ipv6_nib_iface_up(msg.content.ptr);
                break;
-            case GNRC_IPV6_NIB_IFACE_DOWN:
+            case GNRC_NETIF_EVENT_LINK_STATE_CHANGED_DOWN:


Care to fix the indent here, since you touch the code anyway?

fabian18 · 2023-03-18T23:42:22Z

sys/include/net/gnrc/netif.h

+typedef enum {
+    /**
+     * @brief   Link state changed to UP
+     *
+     * The event is generated when the link state on the interface became online.
+     */
+    GNRC_NETIF_EVENT_LINK_STATE_CHANGED_DOWN,
+
+    /**
+     * @brief   Link state changed to DOWN
+     *
+     * The event is generated when the link state on the interface become offline.
+     */
+    GNRC_NETIF_EVENT_LINK_STATE_CHANGED_UP,
+} gnrc_netif_event_t;


Documentation seems to be swapped

fabian18 · 2023-03-18T23:54:41Z

sys/net/gnrc/netif/gnrc_netif.c

+    int res = 0;
+
+    netopt_state_t state_prev;
+    while (gnrc_netapi_get(netif->pid, NETOPT_STATE, 0, &state_prev, sizeof(state_prev)) == -EBUSY) {}


This line and the one below are a bit too long (warning of > 100 characters)

benpicco requested review from miri64, haukepetersen, PeterKietzmann and cgundogan as code owners April 4, 2022 15:31

benpicco added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 4, 2022

github-actions bot added Area: network Area: Networking Area: sys Area: System labels Apr 4, 2022

benpicco requested a review from jia200x April 4, 2022 15:32

benpicco mentioned this pull request Aug 29, 2022

gnrc_netif: handle NETDEV_EVENT_LINK_UP/DOWN events #17893

Merged

benpicco force-pushed the gnrc_netif_updown-event branch from f8afd7c to b9ff0ca Compare September 18, 2022 14:03

benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 18, 2022

benpicco force-pushed the gnrc_netif_updown-event branch from 86a9be4 to c17dc4c Compare September 18, 2022 15:48

fabian18 reviewed Sep 21, 2022

View reviewed changes

benpicco force-pushed the gnrc_netif_updown-event branch from 5a25d0c to b54778a Compare September 21, 2022 16:49

benpicco added 3 commits October 11, 2022 23:26

net/gnrc_netif: add bus for interface events

4c012cb

net/gnrc_netif: add default value for NETOPT_LINK

ab47098

net/gnrc_netif: add gnrc_netif_up()/gnrc_netif_down()

bb2697b

benpicco force-pushed the gnrc_netif_updown-event branch from b54778a to bb2697b Compare October 11, 2022 21:27

gnrc_ipv6_nib: make use of gnrc_netif_bus for interface events

8de8d05

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 20, 2022

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 13, 2022

maribu reviewed Jan 10, 2023

View reviewed changes

benpicco added the State: waiting for author State: Action by the author of the PR is required label Feb 2, 2023

benpicco mentioned this pull request Mar 15, 2023

drivers/atwinc15x0: support dynamic scanning and connection to AP #19387

Merged

fabian18 reviewed Mar 19, 2023

View reviewed changes

benpicco requested a review from HendrikVE December 18, 2023 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/gnrc_netif: add bus for interface events, gnrc_netif_up()/gnrc_netif_down() functions #17902

net/gnrc_netif: add bus for interface events, gnrc_netif_up()/gnrc_netif_down() functions #17902

benpicco commented Apr 4, 2022 •

edited

Loading

jia200x commented Apr 5, 2022

fabian18 left a comment

fabian18 Sep 21, 2022

benpicco Sep 21, 2022

benpicco commented Oct 18, 2022 •

edited

Loading

riot-ci commented Oct 20, 2022 •

edited

Loading

maribu left a comment

maribu Jan 10, 2023

maribu Jan 10, 2023

benpicco Jan 31, 2023

maribu Jan 10, 2023

fabian18 Mar 18, 2023

fabian18 Mar 18, 2023

net/gnrc_netif: add bus for interface events, gnrc_netif_up()/gnrc_netif_down() functions #17902

Are you sure you want to change the base?

net/gnrc_netif: add bus for interface events, gnrc_netif_up()/gnrc_netif_down() functions #17902

Conversation

benpicco commented Apr 4, 2022 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

jia200x commented Apr 5, 2022

fabian18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco commented Oct 18, 2022 • edited Loading

riot-ci commented Oct 20, 2022 • edited Loading

Murdock results

Artifacts

maribu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco commented Apr 4, 2022 •

edited

Loading

benpicco commented Oct 18, 2022 •

edited

Loading

riot-ci commented Oct 20, 2022 •

edited

Loading