Skip to content

Extensebility of netdev_driver_t API #12469

@maribu

Description

@maribu

The Issue

In PR #12294 the feature to let the transceiver sleep and wake up again is developed. I strongly think that there should be a common API for this functionality, so this feature can be used in the same way for all transceiver supported by RIOT.

There seems however not a an easy way to extend the API, except for the netdev_driver_t::get() and netdev_driver_t::set() function - those have been proven to be very well extensible without having compatibility issues.

Using netdev_driver_t::set() for sleeping

One could use netdev_driver_t::set(dev, NETOPT_STATE, NETOPT_STATE_SLEEP, sizeof(NETOPT_STATE_SLEEP)) as a conical way to get the device sleeping. But what should be the way to wake it up again?

One of the following? Any of the following?

  1. netdev_driver_t::set(dev, NETOPT_STATE, NETOPT_STATE_IDLE, sizeof(NETOPT_STATE_IDLE))
  2. netdev_driver_t::set(dev, NETOPT_STATE, NETOPT_STATE_RX, sizeof(NETOPT_STATE_RX))
  3. netdev_driver_t::set(dev, NETOPT_STATE, NETOPT_STATE_RESET, sizeof(NETOPT_STATE_RESET))

TX Preloading

A similar approach is used for TX preloading: When preloading is active, netdev_driver_t::send() will not send but preload the given frame. And netdev_driver_t::set(dev, NETOPT_STATE, NETOPT_STATE_TX, sizeof(NETOPT_STATE_TX))

The issue with netdev_driver_t::set() for actions

To me the fact that netdev_driver_t::set() and netdev_driver_t::get() have been proven to be so versatile proves that this API is well designed. But this doesn't seem to reflect actions well.

E.g. with the state of the driver: Normally, this is and implementation detail. The internal state machine of the driver can either reflect the states in netopt_state_t very well, or could be vastly more complex, or anything in between. Similar, the transitions between the states greatly depend on the properties of both the specific hardware used and the design decisions in the driver made. Providing an API that looks like force-setting the internal state seems very wrong to me.

The road forward

So, what should we do?

Should we simple keep using the get() / set() not only for things like configuration knobs (for which the API - at least to my personal opinion - has served us very well), but also for actions?

Or should we try to add a second API that should ideally as extensible as the get() / set() API while also allowing netdev_driver_ts to only support a subset of the features?

Very likely there are more options that I haven't thought about

Collecting ideas

Adding an action API

How about adding a specific action API, so that netdev_driver_t becomes:

typedef struct netdev_driver {
    int (*action)(netdev_t *dev, netdev_action_t action, void *value, size_t value_len);
    int (*init)(netdev_t *dev); 
    void (*isr)(netdev_t *dev);
    int (*get)(netdev_t *dev, netopt_t opt,  void *value, size_t max_len);
    int (*set)(netdev_t *dev, netopt_t opt, const void *value, size_t value_len);
} netdev_driver_t;

with

typdef enum {
    NETDEV_ACTION_SEND, /**< Sends a frame. Value is of type `const iolist_t *` and `value_len` is `sizeof(iolist_t)`. */
    NETDEV_ACTION_TX_PRELOAD, /**< Preloads a frame for TX. Value is of type `const iolist_t *` and `value_len` is `sizeof(iolist_t)`. A subsequent call with `NETDEV_ACTION_SEND` and `value == NULL` will send it */
    NETDEV_ACTION_RX_SIZE, /**< Get the size of the received frame without dropping it. `value` and `value_len` are ignored, but should be set to zero by the caller */
    NETDEV_ACTION_DROP, /**< Drops the received frame. `value` and `value_len` are ignored, but should be set to zero by the caller */
    NETDEV_ACTION_RECV, /**< Retrieves the received frame */
    NETDEV_ACTION_SLEEP, /**< Power down the netdev to conserve power. No frames are received any more */
    NETDEV_ACTION_WAKE_UP, /**< Powers up the netdev to be able to receive frames */
    NETDEV_ACTION_PEEK, /**< Retrieve the beginning of the received or incoming frame without dropping it, cropping it to the provided buffer size if needed */
} netdev_action_t;

(The details of NETDEV_ACTION_RECV are purposely left out, as e.g. just using value as the destination buffer and value_len as the buffer size would lack the feature of the RX info; so either a struct that can be used to pass the buffer and retrieve the RX info should be used, or a second action or a get() would be needed to get the RX info. But this in-detail discussion is misplaced here until the general idea of this approach should be agreed upon first.)

This proposal tries to learn some lessons from previous discussions and ideas:

  • The get()/set() API has proved to be very flexible and extensible. This proposed API tries to copy this feature
  • Previous tries in extending the API had some flaws:
    • The recv() function has been overloaded to support the drop() and get_size() functionality by providing magic values as arguments. This lead to confusion about the API and a lot of bugs in the implementations (see Misleading API in netdev driver #9805). The explicit enum value provided here should be more obvious
    • Adding additional functions increases the size of the struct and the ROM size of the implementations as well (see numbers provided in Misleading API in netdev driver #9805). This approach does the opposite by replacing the existing send() and recv() functions with a more generic function

Metadata

Metadata

Labels

Discussion: RFCThe issue/PR is used as a discussion starting point about the item of the issue/PRDiscussion: needs consensusMarks a discussion that needs a (not necessarily moderated) consensusState: staleState: The issue / PR has no activity for >185 days

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions