drivers/slipmux: Rework SLIPDEV driver in preparation for Unicoap

[Teufelchen1]

Contribution description

Hey hey :seal:

This refactors the current slipdev driver heavily and also renames it to slipmux.

The networking aspect (plain slip) is no longer the default but a submodule: slipmux_net. The "configuration" aspect (coap via uart) is no longer called slipdev_config but slipmux_coap. That moves it further from the slipmux draft but (I hope) closer to the average developer and sets the expectations clearer. Same for diagnostic but there it was already called _stdio which was retained.

There is a new "overall" module called slipmux which enables all three modes (slipmux_stdio, _coap and _net) for full slipmux functionality.

The parser behavior got a slight change and differentiates between unknown frame types and IPv4/6 frames now. This should make it more robust when interfacing slipmux instead of slip. (Worry not, our slip tools still work as expected)

In addition, the coap handling got reworked to be ready for plug & play action with unicoap once #21582 is merged.

Testing procedure

Pick one of our examples that offer slip e.g. examples/networking/gnrc/border_router/ and see that it still behaves as advertised in the README. tl;dr:

cd examples/networking/gnrc/border_router/
BOARD=nrf52840dk UPLINK=slip make all flash -j9 term
Building application "gnrc_border_router" for "nrf52840dk" with CPU "nrf52".
[...]
> 
> ifconfig
Iface  2 
          MTU:65535  HL:64  RTR  
          
          Link type: wired
          inet6 addr: fe80::2  scope: link  VAL
          inet6 group: ff02::1:ff00:2
          
Iface  6  HWaddr: 35:3F  Channel: 26  NID: 0x23  PHY: O-QPSK 
         [...]
          inet6 group: ff02::1:ff22:b53f
          
ps
> ps
	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
	  - | isr_stack            | -        - |   - |    512 (  224) (  288) | 0x20000000 | 0x200001c8
	  1 | main                 | running  Q |   7 |   1536 (  904) (  632) | 0x200002f0 | 0x2000062c 
	  2 | slipdev              | bl anyfl _ |   2 |    896 (  284) (  612) | 0x20004af4 | 0x20004db4 
	 [...]
	  7 | uhcp                 | bl mutex _ |   6 |   1536 (  944) (  592) | 0x200040bc | 0x200044d4 
	    | SUM                  |            |     |   7744 ( 3388) ( 4356)

See the slipdev thread running (pid 2) and then try to ping the border router from your linux console:

 ~> ping fe80::2%sl0
PING fe80::2%sl0 (fe80::2%sl0) 56 data bytes
64 bytes from fe80::2%sl0: icmp_seq=1 ttl=64 time=20.1 ms
64 bytes from fe80::2%sl0: icmp_seq=2 ttl=64 time=20.5 ms
^C
--- fe80::2%sl0 ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1001ms
rtt min/avg/max/mdev = 20.108/20.287/20.466/0.179 ms

[riot-ci]

Murdock results

:x: FAILED

376fd64e3367808cd820b86f7fe943157b85874c move module ifdef/is_used into helper functions

Build failures (14)

Application	Target	Toolchain	Runtime (s)	Worker
tests/net/slip	native32	llvm	0.67	alien
tests/net/slip	native32	gnu	1.24	mobi6
tests/net/slip	msba2	gnu	2.30	alien
tests/net/slip	native64	llvm	0.61	alien
tests/net/slip	native64	gnu	1.35	mobi6
tests/net/slip	qn9080dk	gnu	1.17	tatooine
tests/net/slip	msb-430	gnu	3.53	mobi6
tests/net/slip	stm32f429i-disc1	gnu	2.22	mobi7
tests/net/slip	adafruit-itsybitsy-m4	gnu	6.57	mobi6
tests/net/slip	samr21-xpro	gnu	2.02	mobi6
tests/net/slip	frdm-k64f	gnu	1.57	mobi7
tests/net/slip	nrf52840dk	gnu	2.21	mobi6
tests/net/slip	stk3200	gnu	3.54	mobi6
tests/net/slip	samr21-xpro	llvm	6.06	skyleaf

Test failures (1)

Application	Target	Toolchain	Runtime (s)	Worker
tests/sys/event_wait_timeout_ztimer	native64	llvm	0.00	alien

Artifacts

[Teufelchen1]

Just some basic style and nitpick comments for now.

Way too early :p but thanks anyways, I adopted the fixes.

Also the static test has some ideas

Probably. I wasn't done-enough yet, to look at them.

[..] and you sometimes added yourself in the Copyright and sometimes in the Doxygen authorship, but not really consistently yet 🤔

No care was given to that yet.

[Teufelchen1]

I think this is ready for actual review now. I would appreciate feedback specifically on the mutex handling. I am not super proud of it.

[crasbe]

If you want to, you can squash before the reviews start pouring in 🤔

[mguetschow]

Btw, while editing the title I was thinking: does this really belong to drivers and not rather sys/net?

[Teufelchen1]

Great stuff! ❤️

Thanks!

Unfortunately it was a bit hard to review since Github didn't properly display the file renames (separate commit would have helped).

Yes, I realized too late that this approach was shit (sorry!).

does this really belong to drivers and not rather sys/net?

I don't know. :see_no_evil:

[Teufelchen1]

@mguetschow you are going to hate me :see_no_evil:

Unfortunately it was a bit hard to review since Github didn't properly display the file renames (separate commit would have helped).

I did it again! (sort of) I renamed the driver to slipmux_dev. This was necessary to clean up the makefile stuff around pseudomodules and dependency resolution. This allows to have the slipmux pseudomodule (which conflicted with the drivers name before hand) that enables all submodules automatically. It also makes it much much cleaner, when a submodule, say slipmux_net pulls in slipmux_dev (used to be slipmux). The old way was super confusing as having the slipmux module in you make info-modules list did not guarantee that all slipmux submodules were enabled. This is now the case. Bonus points because I can now issue a warning to the user when when selecting slipmux together with either slipmux_ submodule: The user will otherwise be surprised that all submodules are enabled.

[Teufelchen1]

I'm not 100% sure I'm happy with slipmux being an alias for all of the submodules. Is that common practice in RIOT? e.g. gnrc uses gnrc_default for such default configurations. But I won't die on that hill either.

Yes I am also not 100% happy. I could go and rename the driver back to slipmux and have slipmux_default to be inline with gnrc but that would also be wrong'ish? Enabling slipmux raises the expectation that RIOT is now slipmux-draft compatible, which it wouldn't be. Plus, unlike other modules where the "base" module already provides value, the slipmux base module would provide zero value. That is why I reasoned: Let's have the draft name equal a draft compliant driver. But because it is a hard requirement to be able to only use a sub-set of the slipmux frame types (e.g. SLIP / Networking only), we have to have a sub-module that reflects that. I chose slipmux_net but that is arbitrary. Now we have to solve the problem that slipmux_net needs to enable some kind of base module which must not be slipmux as this is already saying "enable everything". For that I chose slipmux_dev.

[benpicco]

Enabling slipmux raises the expectation that RIOT is now slipmux-draft compatible, which it wouldn't be. Plus, unlike other modules where the "base" module already provides value, the slipmux base module would provide zero value.

Our main use case for the slipdev driver is a simple point to point IP connection between two MCUs, so no stdio, no CoAP.

So if slipdev were an alias for slipmux_net, I'd be happy.

(Arg, but then you'd also have to provide SLIPDEV_PARAM_UART -> SLIPMUX_PARAM_UART compat defines - is the slipdev name really causeing so much pain that the rename is worth the hassle?)