paho: use GNRC instead of lwip #18982

OlegHahm · 2022-11-27T00:43:34Z

Contribution description

The example application for Paho MQTT is using lwip instead of GNRC because at the time of writing no TCP support for GNRC was available. However, in the meantime the situation has changed and with a tiny tweak of the adapter code Paho works like charm on GNRC TCP sockets.

Testing procedure

Start a MQTT broker, e.g., mosquitto.

On native:

Start the example application and add the tab bridge as default route via nib route add 7 :: <link-local address of the bridge>

On iotlab-m3 (or similar):

Setup a border router (for instance in IoT-Lab following https://www.iot-lab.info/learn/tutorials/riot/riot-public-ipv6-m3/)
Start the example application on a second node

Finally, call con <Broker IPv6 address> followed by sub <TOPIC> and `pub .

benpicco · 2022-11-27T15:09:18Z

examples/paho-mqtt/Makefile

+USEMODULE += auto_init_gnrc_netif
+# Activate ICMPv6 error messages
+USEMODULE += gnrc_icmpv6_error
+# Specify the mandatory networking module for a IPv6 routing node
+USEMODULE += gnrc_ipv6_router_default
+# Add a routing protocol
+USEMODULE += gnrc_rpl
+USEMODULE += auto_init_gnrc_rpl
+# Additional networking modules that can be dropped if not needed
+USEMODULE += gnrc_icmpv6_echo


Why not support both?
e.g. examples/gcoap and examples/telnet_server can be compiled with either GNRC or LWIP.

Sure it would, but what would be the advantage? In my understanding the example applications should be easy to use (and as simple as possible). I think having one example that demonstrates how to build a network application for different stacks is sufficient.

benpicco · 2022-11-27T15:10:54Z

examples/paho-mqtt/Makefile

+# Activate ICMPv6 error messages
+USEMODULE += gnrc_icmpv6_error
+# Specify the mandatory networking module for a IPv6 routing node
+USEMODULE += gnrc_ipv6_router_default


Suggested change

USEMODULE += gnrc_ipv6_router_default

USEMODULE += gnrc_ipv6_default

should be enough here

I actually thought about which GNRC modules are required and which are optional and came to the conclusion that for the ease-of-use requirement adding routing support is beneficial. Same goes for ICMP. If someone wants to the test the application in the IoT-Lab, for instance, having RPL and routing active might be helpful to actually get a route towards the 6lbr - and having ICMP enabled is beneficial to check the connectivity first.

Of course, we could also remove the modules and try to keep the example application as slim as possible.

Both options have their pros and cons IMO.

The thing is that this will actually make it harder to use. When the node is a router, it will not send router solicitations. That means you’ll have to wait for the next unsolicited router advertisement to get a global address.

Valid point. So, how about keeping RPL, but switching to host, i.e., RPL leaf node?

tbh I'm not sure if RPL actually is used much at all, at appears to be rather abandoned.

Are you talking about usage or maintenance? As far as I can tell, it is working well.

If @benpicco isn't using it, it is abandoned ;) I use it regularly in my setups.

(and it is used with gnrc_networking, so most people using that example will use it)

@miri64 are you only using it with static topologies? It appears that repairing the DODAG when a node vanishes just doesn't happen, which is kinda crucial in real-world scenarios. (#17327) Or maybe I'm just under-estimating the time this takes?
When I asked about it more than a year ago I couldn't find anyone who used it outside simple / short lived test scenarios and nobody who maintains the module.

If this bug is still present basically the same arguments from below (whether to enable LWIP as an alternative or not) apply. If RPL works in some scenarios it is nice to have for this application, but it may get frustrating for users using it an dynamically changing topology. I used RPL on real hardware with bigger topologies only in the IoT-LAB so far, so I've observed the bug myself.

benpicco · 2022-11-27T15:11:14Z

examples/paho-mqtt/Makefile

+# Add a routing protocol
+USEMODULE += gnrc_rpl
+USEMODULE += auto_init_gnrc_rpl


Suggested change

# Add a routing protocol

USEMODULE += gnrc_rpl

USEMODULE += auto_init_gnrc_rpl

Not really needed here

aabadie · 2022-11-27T15:45:45Z

examples/paho-mqtt/Makefile

@@ -33,25 +33,20 @@ USEMODULE += ps
 USEMODULE += netdev_default
 USEPKG += paho-mqtt

-# paho-mqtt depends on TCP support, choose which stacks you want
-LWIP_IPV4 ?= 0


Switching to GNRC will drop support for IPv4 from this example. I think the use of LwIP stack was not only motivated by the missing TCP support in GNRC but also by the missing IPv4 support.

To be honest, the main motivation for this PR was that the LWIP version in the IoT-Lab on IPv6 doesn't work for some reason. (The TCP connection is not established.) Since I didn't want to debug LWIP, I tried with GNRC instead.

In the end, the question remains: what is the main use case we want to support here. My idea is that all networking examples use GNRC whenever possible. Of course, that limits the use case to IPv6 (and 6LoWPAN) only. The other option would be to use LWIP for all networking examples wherever possible to allow for both, IPv4 and IPv6.

But with the sock API we can support both stacks without any changes to the code.
So the Makefile only needs to select different modules, depending on the desired stack.

Well, first of all that would require that this application actually works with both stacks which I couldn't verify (LWIP with 6lbr doesn't seem to work). Second, if we go with this reasoning we should update all networking applications which use sock.

You can use LWIP without 6lbr, it also supports non-6lo interfaces.

It's not unreasonable to try to build with both stacks to ensure they work the same. The changes to the Makefile are really small.

You can use LWIP without 6lbr, it also supports non-6lo interfaces.

I would prefer an example that works in all scenarios. Having an example application that is known to be broken in a certain configuration may get frustrating for a user.

It's not unreasonable to try to build with both stacks to ensure they work the same. The changes to the Makefile are really small.

I'm not per se against this, but what would be the rationale to enable both stacks for this application, but, for instance, not for the MQTT-SN examples?

OlegHahm · 2022-11-29T12:06:27Z

Okay, I removed RPL, configure RIOT as a host (instead of router) and let the user select a network stack.

benpicco

Looks good to me, please squash

examples/paho-mqtt/main.c

riot-ci · 2022-11-29T23:59:23Z

Murdock results

✔️ PASSED

ef2b581 paho: use GNRC instead of lwip

Success	Failures	Total	Runtime
117858	0	117858	01h:54m:32s

Artifacts

Documentation preview

benpicco · 2022-11-30T13:55:17Z

If you change the default, Makefile.ci will need an update since GNRC is generally bigger than LWIP.

OlegHahm added Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: pkg Area: External package ports Area: examples Area: Example Applications labels Nov 27, 2022

OlegHahm requested a review from miri64 November 27, 2022 00:43

benpicco reviewed Nov 27, 2022

View reviewed changes

aabadie reviewed Nov 27, 2022

View reviewed changes

benpicco approved these changes Nov 29, 2022

View reviewed changes

benpicco reviewed Nov 29, 2022

View reviewed changes

examples/paho-mqtt/main.c Outdated Show resolved Hide resolved

OlegHahm force-pushed the pr/paho_on_gnrc branch from ee87b26 to e4f8f10 Compare November 29, 2022 15:10

OlegHahm requested review from kYc0o and maribu as code owners November 29, 2022 15:10

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Nov 29, 2022

OlegHahm force-pushed the pr/paho_on_gnrc branch from e4f8f10 to 5455c9f Compare November 29, 2022 15:15

github-actions bot removed Area: cpu Area: CPU/MCU ports Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Nov 29, 2022

OlegHahm added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 29, 2022

OlegHahm mentioned this pull request Nov 30, 2022

pkg: paho-mqtt: added missing requirement #18997

Merged

paho: use GNRC instead of lwip

ef2b581

OlegHahm force-pushed the pr/paho_on_gnrc branch from 5f0174e to ef2b581 Compare November 30, 2022 19:43

OlegHahm merged commit df0c04e into RIOT-OS:master Dec 1, 2022

OlegHahm deleted the pr/paho_on_gnrc branch December 1, 2022 07:47

kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

paho: use GNRC instead of lwip #18982

paho: use GNRC instead of lwip #18982

OlegHahm commented Nov 27, 2022

benpicco Nov 27, 2022

OlegHahm Nov 27, 2022

benpicco Nov 27, 2022

OlegHahm Nov 27, 2022

benpicco Nov 27, 2022

OlegHahm Nov 27, 2022

benpicco Nov 28, 2022

OlegHahm Nov 28, 2022

miri64 Nov 28, 2022

miri64 Nov 28, 2022

benpicco Nov 28, 2022 •

edited

Loading

OlegHahm Nov 28, 2022

benpicco Nov 27, 2022

aabadie Nov 27, 2022 •

edited

Loading

OlegHahm Nov 27, 2022

benpicco Nov 28, 2022

OlegHahm Nov 28, 2022

benpicco Nov 28, 2022

OlegHahm Nov 28, 2022

OlegHahm commented Nov 29, 2022

benpicco left a comment

riot-ci commented Nov 29, 2022 •

edited

Loading

benpicco commented Nov 30, 2022

	USEMODULE += gnrc_ipv6_router_default
	USEMODULE += gnrc_ipv6_default

	# Add a routing protocol
	USEMODULE += gnrc_rpl
	USEMODULE += auto_init_gnrc_rpl

paho: use GNRC instead of lwip #18982

paho: use GNRC instead of lwip #18982

Conversation

OlegHahm commented Nov 27, 2022

Contribution description

Testing procedure

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie Nov 27, 2022 • edited Loading

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

OlegHahm commented Nov 29, 2022

benpicco left a comment

Choose a reason for hiding this comment

riot-ci commented Nov 29, 2022 • edited Loading

Murdock results

Artifacts

benpicco commented Nov 30, 2022

benpicco Nov 28, 2022 •

edited

Loading

aabadie Nov 27, 2022 •

edited

Loading

riot-ci commented Nov 29, 2022 •

edited

Loading