drivers/slipmux: Add to RIOT #21418

Teufelchen1 · 2025-04-17T09:45:00Z

Contribution description

Hey 🪼,

this adds an (incomplete) implementation of Slipmux to RIOT. It extends the current slip driver and is backwards compatible.

The second commit adds an usage example, where the shell commands are exposed to coap and through it to slipmux. That commit will be dropped prio merging.

It is incomplete as frame abort is missing.

Testing procedure

Grab your favorite slipmux tool and connect RIOTs stdio to it. Make sure to build RIOT with USEMODULE += slipdev_stdio and optionally set CFLAGS += "-DCONFIG_NANOCOAP_BLOCK_SIZE_EXP_MAX=10" to be able to handle coap packets that are longer than 64 bytes. See the changes I made to the default example.

Feedback wanted: coap server

My current approach spawns a new thread with a nanocoap server to handle the incoming and outgoing configuration frames. This might cause issues when another coap server is in use (one that is connected to the network instead of just the uart). For example, the NANOCOAP_RESOURCE(..)s will be shared between them.

I also experimented with sending the coap messages with a fake ip6+udp header into gnrc. One can then only run one single coap server (and it is no longer bound to nanocoap) who listens for such packets. The outgoing way is a bit more annoying, here a new dummy interface must be created, to which gnrc can direct those fake ip6+udp headers. Very similar to how it is already done with the current slip implementation.

What alternative approaches do you see? Any idea is appreciated.

benpicco · 2025-04-17T13:50:23Z

I don't quite understand the purpose if this.
When you already have a SLIP connection, you might as well do CoAP over UDP.

Sure, with this approach you can omit the IP and UDP header, but is that really worth the hassle?

Teufelchen1 · 2025-04-17T14:04:32Z

From the byte-wise overhead, it is not that much of a difference yes. I don't think it is that much of a hassle tho. Beside, as soon as you use IP+UDP you have endpoints in a network and those need to be configured (I need to know the IP I want to talk to). Doing slipmux configuration frames you have a guaranteed point to point channel with no configuration nor hassle ;)

chrysn · 2025-04-17T21:26:39Z

I can't comment on the code right now, but maybe for the open aspects:

"Why?" Teufelchen already mentioned the configuration aspect; without precise knowledge of the concrete application (and a point of using SLIP is that this knowledge is not required) the host may not even know that it reaches the board this way. In particular,
- this connection may be more trusted than a network connection: While I generally advocate no-TOFU / all-links-are-untrusted / cryptographically-secure-or-bust policy, TOFU policies do have their places, and TOFU over a link that is known not to be networked is preferable over a link where one would need to apply heuristics to know whether a peer is local. (Like, sure, might do link-local-only, but maybe there's NDP-proxying involved?).
- This might be a good fit also for devices that don't do network at all, or not on that interface. On those, actual SLIP packets would be blackhole'd, but CoAP provides an interface that is easier to automate than shell interactions, and moreover easily ported to devices where no serial line but network is available.
- Where we do have Ethernet-over-USB, I think that a dedicated CoAP-over-USB protocol would be preferred. However, I'm not aware of any such attempt, and even then, there are relatively new devices around still that still run through the moral equivalent of an FTDI chip.
"Which CoAP server to run?" I think we should run the same single CoAP server on all transports as a matter of simplicity; where a UART connection is more trusted than a network peer, which transport it is should be discoverable by inspecting the socket-ish thing it comes in over. I don't have an opinion yet on which implementation strategy I'd advocate for.

miri64

First round of review.

drivers/include/slipdev.h

miri64 · 2025-05-16T09:06:41Z

drivers/include/slipdev.h

+#if IS_USED(MODULE_SLIPDEV_STDIO)
+    chunk_ringbuf_t rb_config;              /**< Ringbuffer to store received CONFIG frames.*/
+    uint8_t rxmem_config[CONFIG_SLIPDEV_BUFSIZE]; /**< memory used by RX buffer */
+    kernel_pid_t coap_server_pid;


Suggested change

kernel_pid_t coap_server_pid;

kernel_pid_t coap_server_pid; /**< The PID of the CoAP server */

Wrt to future extensions, maybe that can be more generalized so that, e.g., also a gcoap or unicoap queue could be put here, if they exist to not have to provide an extra server then.

Again, mainly a comment for future improvements, this does not have to be addressed in this PR.

(edit: but would also address @chrysn's comment)

drivers/slipdev/include/slipdev_internal.h

examples/basic/default/Makefile

miri64 · 2025-05-16T09:20:05Z

examples/basic/default/Makefile

 # Modules to include:
+USEMODULE += slipdev_stdio


I would prefer this to be its own example.

The second commit adds an usage example, where the shell commands are exposed to coap and through it to slipmux. That commit will be dropped prio merging.

Thanks for the review!

I resolved your other comments without fixing them as they would be dropped too.

sys/include/shell.h

riot-ci · 2025-05-16T09:50:24Z

Murdock results

❌ FAILED

d8dd55e fixup: Minor guard/parsing adjustment

Success	Failures	Total	Runtime
2027	0	9598	03m:32s

Artifacts

drivers/include/slipdev.h

drivers/slipdev/slipdev.c

benpicco · 2025-06-10T10:57:29Z

drivers/slipdev/slipdev.c

 void slipdev_setup(slipdev_t *dev, const slipdev_params_t *params, uint8_t index)
 {
+#if IS_USED(MODULE_SLIPDEV_STDIO)


Please use a separate pseudo-module for this, normal slipdev stdio multiplex should not require a CoAP server.

You are repeating yourself ;)

I am not sure yet if "just a separate pseudo-module" is the correct call. What do you think about making the driver "slipmux" with no functionality (not even slip). One can then add the pseudomodules slipmux_ip, slipmux_configuration and slipmux_stdio which add their respective functions. This way, one could use configuration frames (-> coap) without needing IP/gnrc as a dependency.

If you desire to do so go ahead.
I just want to avoid bloating the slipdev driver for simple device to device links that might have a multiplexed debug interface for convenience.

That will be much more work than I thought. I'll move that task to a followup PR. For this one, I adopted your wish and guarded it with its own pseudomodule.
In addition, I changed the way the parsing is guarded with the #if IS_USED(..). Before: The whole case of stdio was removed if the module was not selected. Now: Only the data processing is removed, the cases for parsing remain. (Same for configuration frames). Reason is simple: If you remove the parsing + states, any byte received will be treated as a IP packet and then dropped by gnrc as "undefined packet type". This no-longer the case, instead, receiving a diagnostic/configuration frame without their pseudomodules enabled, they will be silently ignored.

drivers/slipdev/slipdev.c

Teufelchen1 · 2025-06-17T09:22:53Z

@benpicco I think this is ready for another review. If that is positive, I will drop the shell/example related commit and squash the others.

Teufelchen1 · 2025-06-23T11:58:02Z

@benpicco ping :)

review(miri) fix up review(ben): Guard slipmux config handling with own pseudo module adjustment for review, to be dropped fixup whitespace issues

miri64 · 2025-06-30T11:53:42Z

drivers/include/slipdev.h

@@ -120,13 +129,19 @@ typedef struct {
 typedef struct {
    netdev_t netdev;                        /**< parent class */
    slipdev_params_t config;                /**< configuration parameters */
-    chunk_ringbuf_t rb;                     /**< Ringbuffer to store received frames.       */
+    chunk_ringbuf_t rb;                     /**< Ringbuffer to store received NET frames.   */


Is “NET frames” wording from the draft?

no it is not, it would be "Packet Transfer". I thought I would assume a bit that the reader is more likely to know RIOT than the Draft, so I picked a name that represents "Packet Transfer" for RIOT networking humans. Do you think that was a poor choice? I have no hard feelings on this. ("Packet frames" is rather confusing as one could say "coap packet")

miri64 · 2025-06-30T11:54:07Z

drivers/include/slipdev.h

                                            /* Written to from interrupts (with irq_disable */
                                            /* to prevent any simultaneous writes),         */
                                            /* consumed exclusively in the network stack's  */
                                            /* loop at _isr.                                */

    uint8_t rxmem[CONFIG_SLIPDEV_BUFSIZE];  /**< memory used by RX buffer */
+
+#if IS_USED(MODULE_SLIPDEV_CONFIG)
+    chunk_ringbuf_t rb_config;              /**< Ringbuffer to store received CONFIG frames.*/


Same here for “CONFIG frames”.

mguetschow · 2025-06-30T15:05:23Z

Some high-level comments from my side:

incomplete implementation

this adds an (incomplete) implementation of Slipmux to RIOT. It extends the current slip driver and is backwards compatible. (...) It is incomplete as frame abort is missing.

Isn't it also missing diagnostic transfer? Just had a quick glance at the code, but could only find references to configuration frames.

documentation: before merging this should definitely get documentation, probably on the defined pseudomodule slipdev_config. Not sure if this may even warrant its own doc.md somewhere? It should definitely mention slipmux and clearly list what is currently supported. Also some notes on the implementation (currently spawning an additional thread, sharing NANOCOAP_RESOURCE with any other nanocoap server) should be in the documentation.
coap server with different backends: I would hope unicoap could help with that. I'm pretty sure it supports defining resources only for a subset of driver backends (e.g. just slipmux and not udp), which means that a single unicoap server could be used for several backends. Maybe @carl-tud has some more comments on this?
nanocoap/nanocoap_sock: could you maybe explain in a bit more detail how this actually works right now? I see the "server thread" just receives chunks from the ringbuffer, tries to parse and answer them (using nanocoap). Every chunk is supposed to be a CoAP packet as received within slipdev, right? Where do the NANOCOAP_RESOURCEs (which are part of nanocoap_sock afaict) actually come into play?

mguetschow · 2025-06-30T15:01:08Z

drivers/slipdev/slipdev.c

+            crb_consume_chunk(&dev->rb_config, buf, len);
+
+            /* Is crc correct via Residue test */
+            if (crc16_ccitt_fcs_update(SPECIAL_INIT_FCS, buf, len) != 0xF0B8) {


where does this constant come from? mind using a define or at least add a comment?

That's a residue test, the magic value is defined here, in the crc header. Please don't make me add all crcs residue values as defines 😨

Teufelchen1 · 2025-06-30T15:32:36Z

Isn't it also missing diagnostic transfer? Just had a quick glance at the code, but could only find references to configuration frames.

That's because those were already implemented in RIOT and are just called stdio via slip. Blame @miri64 for that :p If you enable this, every printf() will be send slipmux encoded.

[..needs doc...] Not sure if this may even warrant its own doc.md somewhere?

Eventually, yes. There is still work to be done on the coap-handling/server side. If I can get away with minimal documentation this time, I would appreciate. I fear it will be duplicated work if I re-implement it later (for example with unicoap). Plus, as mentioned in a comment with benpicco, the handling of the pseudomodule is not ideal in the moment. For example, it is not possible to enable configuration transfer without packet transfer. And even if you could, nanocoap depends on udp, which means you would still pull in a lot of wasted code & bytes. Both issues are scheduled for follow-up work.

I see the "server thread" just receives chunks from the ringbuffer, tries to parse and answer them (using nanocoap). Every chunk is supposed to be a CoAP packet as received within slipdev, right?

That is correct.

Where do the NANOCOAP_RESOURCEs (which are part of nanocoap_sock afaict) actually come into play?

That's nanocoap specific, I do not interact with them myself (beside the shell stuff but thats just an example of what one could do and will be dropped before merging). Afaik it is just a XFA defining names and callbacks. The nanocoap server just magically knows that XFA and looks up the endpoints on the fly.

Teufelchen1 requested a review from miri64 as a code owner April 17, 2025 09:45

github-actions bot added Area: drivers Area: Device drivers Area: sys Area: System Area: examples Area: Example Applications labels Apr 17, 2025

miri64 requested a review from benpicco April 17, 2025 10:23

Teufelchen1 changed the title ~~DRAFT: Add slipmux to RIOT~~ Add slipmux to RIOT May 16, 2025

miri64 changed the title ~~Add slipmux to RIOT~~ slipmux: Add to RIOT May 16, 2025

miri64 requested changes May 16, 2025

View reviewed changes

crasbe added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 16, 2025

benpicco reviewed May 16, 2025

View reviewed changes

drivers/include/slipdev.h Outdated Show resolved Hide resolved

crasbe changed the title ~~slipmux: Add to RIOT~~ drivers/slipmux: Add to RIOT May 16, 2025

Teufelchen1 force-pushed the feat/slipmux_c branch from e203f61 to 7af6c9c Compare June 10, 2025 10:51

benpicco reviewed Jun 10, 2025

View reviewed changes

drivers/slipdev/slipdev.c Outdated Show resolved Hide resolved

benpicco reviewed Jun 10, 2025

View reviewed changes

drivers/slipdev/slipdev.c Outdated Show resolved Hide resolved

benpicco reviewed Jun 10, 2025

View reviewed changes

drivers/slipdev/slipdev.c Outdated Show resolved Hide resolved

github-actions bot added the Area: build system Area: Build system label Jun 11, 2025

Teufelchen1 mentioned this pull request Jun 16, 2025

checksum: Add crc16-fcs / IBM-SDLC #21552

Merged

Teufelchen1 force-pushed the feat/slipmux_c branch from 88d8ca1 to 12435ec Compare June 17, 2025 09:20

github-actions bot added the Area: tests Area: tests and testing framework label Jun 17, 2025

driver/slip: Add slipmux frames

9d081cb

Teufelchen1 added 3 commits June 23, 2025 15:50

Make shell commands list in coap

eda800f

review changes, to be squashed

8ea3a81

review(miri) fix up review(ben): Guard slipmux config handling with own pseudo module adjustment for review, to be dropped fixup whitespace issues

refactor: Use RIOTs built-in crc16

99211c0

Teufelchen1 force-pushed the feat/slipmux_c branch from 12435ec to 99211c0 Compare June 23, 2025 13:51

Teufelchen1 added 2 commits June 24, 2025 13:14

fixup: add missing header sched.h

2a2ef6e

fixup: Minor guard/parsing adjustment

d8dd55e

miri64 reviewed Jun 30, 2025

View reviewed changes

mguetschow reviewed Jun 30, 2025

View reviewed changes

	kernel_pid_t coap_server_pid;
	kernel_pid_t coap_server_pid; /*< The PID of the CoAP server /

drivers/slipmux: Add to RIOT #21418

Are you sure you want to change the base?

drivers/slipmux: Add to RIOT #21418

Conversation

Teufelchen1 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Feedback wanted: coap server

Uh oh!

benpicco commented Apr 17, 2025

Uh oh!

Teufelchen1 commented Apr 17, 2025

Uh oh!

chrysn commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miri64 May 16, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

riot-ci commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Teufelchen1 commented Jun 17, 2025

Uh oh!

Teufelchen1 commented Jun 23, 2025

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!

mguetschow commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Teufelchen1 commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Teufelchen1 commented Apr 17, 2025 •

edited

Loading

chrysn commented Apr 17, 2025 •

edited

Loading

miri64 May 16, 2025 •

edited

Loading

riot-ci commented May 16, 2025 •

edited

Loading

Teufelchen1 commented Jun 30, 2025 •

edited

Loading