Skip to content

drivers/slipdev: Refactor parser state machine for readability #21953

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.

Sign up for GitHub

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

Merged
Teufelchen1 merged 2 commits into from
Dec 17, 2025
+222 −121
Merged

drivers/slipdev: Refactor parser state machine for readability #21953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5220bff
drivers/slipdev: Refactor parser state machine for readability
Teufelchen1 Dec 11, 2025
6d109ba
drivers/slipdev: Uncrustify
Teufelchen1 Dec 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions drivers/include/slipdev.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ extern "C" {
* sized packets.
*/
#ifdef CONFIG_SLIPDEV_BUFSIZE_EXP
#define CONFIG_SLIPDEV_BUFSIZE (1<<CONFIG_SLIPDEV_BUFSIZE_EXP)
#define CONFIG_SLIPDEV_BUFSIZE (1 << CONFIG_SLIPDEV_BUFSIZE_EXP)
#endif

#ifndef CONFIG_SLIPDEV_BUFSIZE
Expand All @@ -94,15 +94,26 @@ extern "C" {
/** @} */

/**
* @name Device state definitions
* @anchor drivers_slipdev_states
* @{
* @brief States for the slipdev and its parser
*/
enum {
typedef enum {
/**
* @brief Device is in no mode (currently did not receiving any data frame)
* @brief Device is in no mode (currently not receiving any frame), this is the idle state.
*
* Waits for any byte, if the byte is a valid frame start byte (diagnostic, configuration,
* IP packet), it starts a new frame of the respective type and switches to the corresponding
* state. If the byte is a frame end byte, it is a no-op (we received an empty frame).
* If the byte has any other value, an unknown frame type is assumed. The state switches
* to UNKNOWN state.
*/
SLIPDEV_STATE_NONE = 0,
/**
* @brief Device discards incoming data.
*
* It switches back to the NONE (idle) state once the unknown frame is ended via a frame
* end byte.
*/
SLIPDEV_STATE_UNKNOWN,
Copy link
Contributor

@benpicco benpicco Dec 11, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Teufelchen1 Teufelchen1 Dec 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@benpicco benpicco Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Teufelchen1 Teufelchen1 Dec 12, 2025

Choose a reason for hiding this comment

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

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.

/**
* @brief Device writes handles data as network device
*/
Expand Down Expand Up @@ -135,8 +146,7 @@ enum {
* @brief Device is in sleep mode
*/
SLIPDEV_STATE_SLEEP,
};
/** @} */
} slipdev_state_t;

/**
* @brief Configuration parameters for a slipdev
Expand All @@ -163,15 +173,15 @@ typedef struct {
uint8_t rxmem[CONFIG_SLIPDEV_BUFSIZE]; /**< memory used by RX buffer */

#if IS_USED(MODULE_SLIPDEV_CONFIG)
chunk_ringbuf_t rb_config; /**< Ringbuffer stores received configuration frames */
uint8_t rxmem_config[CONFIG_SLIPDEV_BUFSIZE]; /**< memory used by RX buffer */
kernel_pid_t coap_server_pid; /**< The PID of the CoAP server */
chunk_ringbuf_t rb_config; /**< Ringbuffer stores received configuration frames */
uint8_t rxmem_config[CONFIG_SLIPDEV_BUFSIZE]; /**< memory used by RX buffer */
kernel_pid_t coap_server_pid; /**< The PID of the CoAP server */
#endif
/**
* @brief Device state
* @see [Device state definitions](@ref drivers_slipdev_states)
* @see [Device state definitions](@ref slipdev_state_t)
*/
uint8_t state;
slipdev_state_t state;
} slipdev_t;

/**
Expand Down
14 changes: 12 additions & 2 deletions drivers/slipdev/include/slipdev_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,24 @@ extern "C" {
* @see taken from diagnostic transfer from
* [SLIPMUX](https://tools.ietf.org/html/draft-bormann-t2trg-slipmux-03#section-4)
*/
#define SLIPDEV_STDIO_START (0x0aU)
#define SLIPDEV_START_STDIO (0x0aU)

/**
* @brief Marker byte for beginning of configuration/CoAP
* @see taken from configuration from
* [SLIPMUX](https://tools.ietf.org/html/draft-bormann-t2trg-slipmux-03#section-5)
*/
#define SLIPDEV_CONFIG_START (0xa9U)
#define SLIPDEV_START_COAP (0xa9U)

/**
* @brief Starts an IP packet frame
*/
#define SLIPDEV_START_NET(byte) ( \
/* is it an IPv4 packet? */ \
(byte >= 0x45 && byte <= 0x4f) || \
/* or is it an IPv6 packet? */ \
(byte >= 0x60 && byte <= 0x6f) \
)
/** @} */

/**
Expand Down
Loading