Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/lwip: Fix dualstack build when only using 6lowpan #17174

Merged
merged 2 commits into from
Nov 10, 2021

Conversation

yarrick
Copy link
Contributor

@yarrick yarrick commented Nov 9, 2021

Contribution description

  • Disable ARP if Ethernet is not used
  • Use macro converting netif v6 address to ip6_addr_t (which is noop in v6-only setup)

Testing procedure

BOARD=iotlab-m3 LWIP_IPV4=1 LWIP_IPV6=1 make -C tests/lwip -j now works

BOARD=iotlab-m3 LWIP_IPV4=1 make -C tests/lwip -j is still broken (since it disables IPv6): linking fails with undefined reference to 'ipv6_addr_link_local_prefix'

Issues/PRs references

Part of #17162

@yarrick yarrick requested a review from miri64 as a code owner November 9, 2021 22:23
@github-actions github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports labels Nov 9, 2021
@yarrick yarrick added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 9, 2021
@miri64
Copy link
Member

miri64 commented Nov 10, 2021

Part of #17162

Why only "Part of" and not "Fixes"?

@miri64
Copy link
Member

miri64 commented Nov 10, 2021

BOARD=iotlab-m3 LWIP_IPV4=1 make -C tests/lwip -j is still broken (since it disables IPv6): linking fails with undefined reference to 'ipv6_addr_link_local_prefix'

This can be fixed when applying

diff --git a/pkg/lwip/contrib/netdev/lwip_netdev.c b/pkg/lwip/contrib/netdev/lwip_netdev.c
index e70e4eacee..34398470db 100644
--- a/pkg/lwip/contrib/netdev/lwip_netdev.c
+++ b/pkg/lwip/contrib/netdev/lwip_netdev.c
@@ -159,17 +159,19 @@ err_t lwip_netdev_init(struct netif *netif)
             if (netdev->driver->set(netdev, NETOPT_SRC_LEN, &val, sizeof(val)) < 0) {
                 return ERR_IF;
             }
-            /* netif_create_ip6_linklocal_address() does weird byte-swapping
-             * with full IIDs, so let's do it ourselves */
-            addr = ip_2_ip6(&(netif->ip6_addr[0]));
-            /* addr->addr is a uint32_t array */
-            if (l2util_ipv6_iid_from_addr(dev_type,
-                                          netif->hwaddr, netif->hwaddr_len,
-                                          (eui64_t *)&addr->addr[2]) < 0) {
-                return ERR_IF;
+            if (IS_USED(MODULE_IPV6_ADDR)) {
+                /* netif_create_ip6_linklocal_address() does weird byte-swapping
+                 * with full IIDs, so let's do it ourselves */
+                addr = ip_2_ip6(&(netif->ip6_addr[0]));
+                /* addr->addr is a uint32_t array */
+                if (l2util_ipv6_iid_from_addr(dev_type,
+                                              netif->hwaddr, netif->hwaddr_len,
+                                              (eui64_t *)&addr->addr[2]) < 0) {
+                    return ERR_IF;
+                }
+                ipv6_addr_set_link_local_prefix((ipv6_addr_t *)&addr->addr[0]);
+                ip6_addr_assign_zone(addr, IP6_UNICAST, netif);
             }
-            ipv6_addr_set_link_local_prefix((ipv6_addr_t *)&addr->addr[0]);
-            ip6_addr_assign_zone(addr, IP6_UNICAST, netif);
             /* Set address state. */
 #if LWIP_IPV6_DUP_DETECT_ATTEMPTS
             /* Will perform duplicate address detection (DAD). */

However, then another error is revealed:

tests/lwip/bin/iotlab-m3/application_tests_lwip/ip.o:tests/lwip/ip.c:70: undefined reference to `ipv6_addr_to_str'

Which is weird, as this part of the code is #if IS_USED(MODULE_LWIP_IPV6)'d... make info-modules reveals that the module is indeed selected. However, LWIP_IPV6 is 0 as can be shown with. Will have a look where this is coming from...

diff --git a/tests/lwip/Makefile b/tests/lwip/Makefile
index c7b2472561..42dde78c7c 100644
--- a/tests/lwip/Makefile
+++ b/tests/lwip/Makefile
@@ -16,0 +17,2 @@ endif
+$(warning $(LWIP_IPV6))
+

@miri64
Copy link
Member

miri64 commented Nov 10, 2021

Ah...

RIOT/sys/Makefile.dep

Lines 175 to 179 in ff8983c

ifneq (,$(filter ieee802154 nrfmin esp_now cc110x,$(USEMODULE)))
ifneq (,$(filter lwip%, $(USEMODULE)))
USEMODULE += lwip_sixlowpan
endif
endif

selects lwip_sixlowpan (weird that this is in sys/Makefile.dep, this should be in pkg/lwip/Makefile.dep IMHO). That leads to

ifneq (,$(filter lwip_sixlowpan,$(USEMODULE)))
USEMODULE += lwip_ipv6_autoconfig
USEMODULE += l2util
endif

which leads to

ifneq (,$(filter lwip_ipv6_autoconfig lwip_ipv6_mld,$(USEMODULE)))
USEMODULE += lwip_ipv6
endif

IMHO the very first dependency listed should depend on lwip_ipv6% rather than lwip.

@yarrick
Copy link
Contributor Author

yarrick commented Nov 10, 2021

Only part of because it only fixes the dualstack issue, and not when IPv6 is totally missing.

I don't think we should guard on ipv6_addr, it could be useful to still format such addresses without having protocol support for them. The issue might be that lwip_sixlowpan is enabled from sys/, but that does not depend on actual ipv6 support inside lwIP. I think this could be a separate PR however (either skip those interfaces, include ipv6 anyway, or fail to even try to compile anything)

@miri64
Copy link
Member

miri64 commented Nov 10, 2021

I don't think we should guard on ipv6_addr, it could be useful to still format such addresses without having protocol support for them.

Then ipv6_addr and ipv4_addr should be included in tests/lwip/Makefile in all cases.

@miri64
Copy link
Member

miri64 commented Nov 10, 2021

But yes, this can be done as a follow-up. Likewise the dependency fix.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Fixes the issue described in #17162.

@yarrick yarrick merged commit 1a73fee into RIOT-OS:master Nov 10, 2021
@yarrick yarrick deleted the arp_build branch November 10, 2021 09:16
@miri64
Copy link
Member

miri64 commented Nov 10, 2021

🎉 Congrats on your first merge commit!

@miri64 miri64 mentioned this pull request Nov 10, 2021
@fjmolinas fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants