drivers/slipdev: Refactor parser state machine for readability

[Teufelchen1]

Contribution description

Hej 🦝

because the other PR (#21907) around this driver is kinda stuck, I figured it would be best to pull out the non-critical code changes and merge them separately.

I know this is an extra review burden, I apologize - but I think it's best for RIOT (and me, in this case) if useful changes get merged sooner rather than later.

What changes are included here:

• Make a typedef for the state machine states.
• Add a filter for actual IP packets to match on the NET state (see next point)
• Add SLIPDEV_STATE_UNKNOWN to no confused garbage/not-yet-supported frametypes for IP packets. This state is also used for "garbage collection" when one of the other states encounters an issue like no space left in their buffer.
• Renamed the start bytes so that _START_{type} is first (was _{type}_START), to ensure good order in the documentation generation.
• Move preprocessor defines #if IS_USED(MODULE_FOO) out of the parser callback function into sub-functions for readability.
• Add missing length check to CoAP handling

Testing procedure

It should build and pass the CI. Otherwise choose what ever slip/slipmux application you like and see if it still works.
For example:
cd RIOT/examples/networking/gnrc/border_router
BOARD=nrf52840dk UPLINK=slip make all -j9 flash

Issues/PRs references

Prequel to this PR: #21907

[riot-ci]

Murdock results

✔️ PASSED

7056e55 fixup! drivers/slipdev: Uncrustify

Success	Failures	Total	Runtime
10950	0	10950	08m:49s

Artifacts

• Documentation preview

[benpicco]

I would suggest adding a define for the thread flags.
But since you only use a single flag, couldn't the thread_flags_wait_any(1) be replace by a simple mutex_lock()?

[Teufelchen1]

There are other options yes. I don't remember why I choose thread_flags back then. Probably, I was already thinking about unicoap? I don't know.

Since this code was only moved and not changed, can we keep it like this? It's scheduled for removal anyway, with the Unicoap-preparation-pr....and messing with locks means another round of extensive testing for me 🙈

[benpicco]

The name is a bit confusing, we don't have a stdio frame, how about just

Suggested change

	static inline void _slipdev_stdio_add_to_frame(slipdev_t * dev, uint8_t byte) {
	static inline void _slipdev_stdin_add(slipdev_t * dev, uint8_t byte)
	{

[Teufelchen1]

It's confusing either way, loose-loose situation. If you come from slipmux, then all "diagnostic messages" (which we already "degraded" name-wise to stdio (which at least fits its usage in RIOT)) are come in frames. They have a start byte, they do escaping, the have an end byte, just like all other frames. So we are still adding bytes to a frame. The only difference here, to coap/net , is that we don't buffer the frame. From this perspective _add_to_frame is perfectly valid and descriptive. In addition, because we conceptually do the same thing as with the other frame types, it is nice to share the commonality in the function name.

But I can also see why one would look at it from the perspective of an stdio-module. In that case, sure, stdin_add is better suited.

Naturally I prefer to look at it from the slipmux point of view.

But one last argument for my naming: You can run slipmux/slipdev on multiple uarts simultaneously. But RIOT has only one stdin. Calling it stdin_add limits it to that. Should someone ever decide "I like getting stdin on uart0 but I would like to work with diagnostic messages from uart1 in some extra logic as well", in that case add_to_frame will have been the better name! (Okay this is getting ludicrous 🤪🤪)

[benpicco]

What's the difference in behavior to SLIPDEV_STATE_NONE?
In both cases you just wait for a valid start byte, right?

[Teufelchen1]

For someone who spotted the tiniest white space issue, you surely missed an entire gigantic path in the state machine 😄

State NONE signals: There is no frame started, we are idle.

• In SLIP this means the next byte is either:
  • SLIPDEV_END (0xc0), which means we received a zero byte long, empty frame. This is valid behaviour in SLIP.
  • any byte that is not SLIPDEV_END: Starts an IP packet frame.
  • That is why the driver did not had the additional state UNKNOWN, there was no need for it.
• In SLIPMUX this means the next byte is either:
  • SLIPDEV_END (0xc0), which means we received a zero byte long, empty frame. This is valid behaviour in SLIPMUX.
  • SLIPDEV_START_STDIO (0x0a), which means we start a new frame of the type Diagnostic message.
  • SLIPDEV_START_COAP (0xa9), which means we start a new frame of the type Configuration message.
  • SLIPDEV_START_NET (0x45..=0x4f, 0x60..=0x6f), which means we start a new frame of the type IP Packet
  • any byte that is not one of the above is invalid.
  • Since we are in idle and expect to start a new frame, this invalid byte can be considered the initial byte of a new frame, whose type we do not know. The draft calls this Frames with unknown initial bytes should be silently ignored..
  • So, we start a new frame of type UNKNOWN, do the necessary parsing (waiting for the SLIPDEV_END byte to finish the frame) and discard all data received.

[benpicco]

Ok so do I get that right:

• in SLIPDEV_STATE_NONE you wait for a valid frame start byte. Any other byte is discarded.
• in SLIPDEV_STATE_UNKNOWN you wait for a SLIPDEV_END byte. Any other byte (e.g. a valid frame start byte) is discarded. Receiving the END byte gets you to SLIPDEV_STATE_NONE

So you lose the next frame if you lose the END byte, but you also don't get garbage frames if the START bytes is inside another frame where you missed the start sequence.

[Teufelchen1]

in SLIPDEV_STATE_NONE you wait for a valid frame start byte. Any other byte is discarded.

Almost, right. "Any other byte starts an UNKNOWN frame and the followup bytes are discarded". The NONE -> unknown byte -> UNKNOWN transition will be the main / typical way to end up in the UNKNOWN state. The alternative is via exhausted receive buffer, in which case we also want to discard any further incoming bytes for the current frame.

in SLIPDEV_STATE_UNKNOWN you wait for a SLIPDEV_END byte.

Yes. One could also do un-escaping here (SLIPDEV_ESC, SLIPDEV_END_ESC and SLIPDEV_ESC_ESC) but whats the point if one discards the bytes anyway.

Any other byte (e.g. a valid frame start byte) is discarded.

Yes. Which is roughly equal to the regular frames. You don't react on a SLIPDEV_START_COAP byte inside your UDP payload when currently parsing an IP packet (in SLIPDEV_STATE_NET)

Receiving the END byte gets you to SLIPDEV_STATE_NONE

Yes. Equal behaviour to the regular frames.

So you lose the next frame if you lose the END byte,

Yes. True for all frames. And has also been true before my changes. If you missed the END byte, your parsing is out of sync and will produce a garbage frame until it re-syncs with the next END byte.

but you also don't get garbage frames if the START bytes is inside another frame where you missed the start sequence.

Yes. This is among the biggest reasons to have this extra UNKNOWN frame type. Serial connections do not sync, right? They just stream. So it is actually common / likely to be in the middle of a frame when physically connecting the uarts. In the past, where an unknown start byte always started an IP packet, this resulted in gnrc being sad about unparsable packets.

Another reason is future proofing. Should slipmux evolve and include a new frame type like DNS or IPv7 is invented and send via slip: This will result in sane behaviour, which is silent discarding.

[benpicco]

Looks like Doxygen also wants to have a @brief block for this

[Teufelchen1]

Does my change make any sense? I had to wrestle with doxygen here and I am not competent with doxygen...