-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Extensebility of netdev_driver_t API #12469
Description
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?
netdev_driver_t::set(dev, NETOPT_STATE, NETOPT_STATE_IDLE, sizeof(NETOPT_STATE_IDLE))netdev_driver_t::set(dev, NETOPT_STATE, NETOPT_STATE_RX, sizeof(NETOPT_STATE_RX))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 thedrop()andget_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 explicitenumvalue 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()andrecv()functions with a more generic function
- The