Extensebility of `netdev_driver_t` API

[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

[maribu]

@RIOT-OS/maintainers: Please have a look at this and discuss :-)

Please feel invited to directly add alternative ideas as subsections to the "Collecting ideas" section in the description above.

[jia200x]

hi @maribu

This is aligned with the PHY/MAC/netif rework. There was a presentation for the RIOT summit that unfortunately couldn't present because of lack of time. We discussed some insights with @PeterKietzmann, @miri64, @bergzand, @haukepetersen and @kaspar030, but the idea was to have a wide discussion about this topic.

I will try to record the original session asap so we are all in sync.

[jia200x]

Hi @maribu

After opening #12741, I think it's easier to explain my point of view here.

I think considering netdev is an interface that encapsulates several actions, the proposed approach can make sense.

However, I would like to point the problem the netif rework is trying to resolve (in the mid-long term, third phase of #12688 )

(sorry for the Wall Of Text)

The problem

Netdev is documented as a network interface:

This interface provides a uniform API for network stacks to interact with network device drivers. This interface is designed in a way, that it is completely agnostic to the used network stack. This way, device drivers for network devices (e.g. IEEE802.15.4 radios, Ethernet devices, ...) have to implemented once and can be used with any supported network stack in RIOT.

But in practice is closer to a PHY layer (the MAC layer is on top, e.g it's called by the IEEE802.15.4 MAC in gnrc_netif_ieee802154):

But also abstracts some transceiver operations (read packet, set hardware acceleration params, etc).

So it would be ideal to have different layers to have a MAC, PHY and whatever layer is needed in the middle (proposed in #7736).
But in practice, the APIs of each MAC and PHY layer has unique API (an ethernet PHY has no idea how to set an IEEE802.15.4 channel...) . So we could benefit if we define specific APIs for each layer. Something like:

(this is partially shown in #12741).

E.g for the proposed actions:

• NETDEV_ACTION_SEND: This can be replaced by the PHY layer send function (e.g ieee802154_phy_send).
• NETDEV_ACTION_TX_PRELOAD: In theory the transceiver always pre-loads a packet. Then someone triggers send (TX_START flag). This option was introduced because of MAC layers with duty cycling (TSCH), where it's needed to preload a packet before sending in the exact time. However, this is not needed if there's a dedicated PHY layer (or if the MAC layer has direct access to the transceiver, e.g OpenWSN).
• NETDEV_ACTION_RX_SIZE: This is a PHY layer attribute. It can be implemented in the corresponding PHY (e.g indication message in IEEE802.15.4 packet).
• NETDEV_ACTION_DROP: The transceiver usually doesn't know how to drop a packet. We need to add this command because we call netdev->driver->recv function twice (one to get the packet size, another one to put the packet in a buffer). If we don't implement "drop", there's no way to tell the radio "please receive more packets". But same as before, this is PHY/transceiver HAL dependent.
• NETDEV_ACTION_RECV: This is a transceiver indication. It can be implemented in the transceiver HAL (or in a component of the PHY layer).
• NETDEV_ACTION_SLEEP: This is a transceiver operation. It can be implemented in the transceiver HAL.
• NETDEV_ACTION_WAKE_UP: Ditto.
• NETDEV_ACTION_PEEK: This can also be modeled as a transceiver HAL operation.

In fact, send, recv, get, set are "network interface" operations. Check how similar is the API of gnrc_netif_ops and netdev_driver_t.

Note that having MAC/PHY specific APIs doesn't interfere with users sending data directly using the low layers:

• A IEEE802.15.4 PHY user (probably someone designing an IEEE802.15.4 MAC layer or an enthusiast) will use the IEEE802.15.4 PHY layer, no abstraction needed there.
• Someone that wants to send messages between IEEE802.15.4 radios, will need to use the IEEE802.15.4 MAC layer.
• Someone that wants to send data regardless of the MAC layer below, needs a network interface (netif).

However, I'm fine to continue with this proposed approach, since the netif rework will take some time and it doesn't collides with these principles.
If we all agree to have dedicated PHY and MAC layers (#11483) then we would only need to move code.

I hope everything is clear! Please let me know if I miss something

[maribu]

I fail to fully understand this. Could you sketch the API of e.g. an IEEE 802.15.4 Radio HAL? How would it differ (except for names) from the proposed netdev API? (I get that you want to provide HALs specific to the network technology used. But isn't this already the case? The netdev_driver_t::get() and netdev_driver_t::set() APIs already only provided what is relevant to the technology. E.g. setting TX power on a netdev_driver_t for an Ethernet device will simply return -ENOTSUP.)

NETDEV_ACTION_SEND: This can be replaced by the PHY layer send function (e.g ieee802154_phy_send).

Hardly. The PHY layer send function will need some API to implement the send feature. Something like NETDEV_ACTION_SEND. But I like the idea very much to split PHY and bare device driver APIs. This would likely allow some significant code-deduplication

[maribu]

Let's say the different HALs are implemented with the three basic get(), set(), action() (and some stuff for handling ISRs, and initialization). And every HAL for the different technology only differs in the enums used to specify the options to set() or get(), or the action to perform. Would that already be something that would match the concept you proposed?

That would certainly make the compilers job easier to generate efficient machine code for a switch() of the enums.

[jia200x]

I fail to fully understand this. Could you sketch the API of e.g. an IEEE 802.15.4 Radio HAL? How would it differ (except for names) from the proposed netdev API?

Without any cross layer optimizations, a pure theoretical IEEE802.15.4 HAL would need:

• An interface to the FIFO of the transceiver
• A function to trigger send
• A function to set the transceiver state
• Functions to access hardware acceleration features (L2 filters, auto-ACK, retransmissions, CSMA params, etc).
• A function to set the carrier frequency and modulation (QPSK, FSK, ASK, etc)

A pure PHY layer on top would:

• Calculate the carrier frequency and modulation from the channel number + page and set it via the HAL interface
• Function to set PHY states (e.g note "sleep" is not a PHY state)
• Provide a send function using the FIFO interface, set transceiver state and trigger send functions
• Etc

(I'm not assuming it's the smartest idea to have pure theoretical PHY and HAL because most radios provide some components of PHY, but it's just to show the differences)

Indeed it's possible to encapsulate everything under get and set. I would just try to see if it makes sense to encapsulate under these when there's a set of function that must be implemented in a given technology.
E.g, ALL IEEE802.15.4 radios MUST implement set_channel functions.

We could benefit from type checks. E.g

int channel = 16;
ieee802154_set_channel(dev, channel);

instead of

uint8_t channel = 16; /* This MUST be uint8_here */
netdev->driver->set(NETOPT_CHANNEL, &channel, sizeof(channel));

But yes, this is just an implementation decision :)

Hardly. The PHY layer send function will need some API to implement the send feature. Something like NETDEV_ACTION_SEND

It depends on the scope of the transceiver HAL. If we add a FIFO interface, then it's possible to implement the "send" function on top.

Let's say the different HALs are implemented with the three basic get(), set(), action() (and some stuff for handling ISRs, and initialization). And every HAL for the different technology only differs in the enums used to specify the options to set() or get(), or the action to perform. Would that already be something that would match the concept you proposed?

As said before, for sure it would match. The only question that comes to my mind is, if it makes sense to encapsulate under these three basics when the actions are well known for each technology. We could think about vtables. But again, this is just an implementation detail and it shouldn't interfere with the architecture.

That would certainly make the compilers job easier to generate efficient machine code for a switch() of the enums.

That's for sure!

[jia200x]

I just found this: https://github.com/RIOT-OS/RIOT/blob/master/drivers/at86rf2xx/at86rf2xx_netdev.c#L489

This check is not device dependent (the PHY channels are PHY dependent. IEEE802.15.4 2.4GHz always uses page 0).
With a transceiver HAL, we can move the validation to the PHY layer and assume we would never set a wrong channel

[maribu]

Reducing the number of "actual" functions in the HAL has proven to reduce the ROM requirements significantly. The benefit of proper type checks and range changes could be provided upon a slim API via static inline functions that encapsulate e.g. the proposed action(), get() and set() functions. I'd even say it would be nice to have the action(), get() and set() interface as "private" interface, and let users only use static inline functions to access them. That would yield an API with easy to use and more descriptive functions that provide both type safety and range check; but without the increased ROM requirements.

From the perspective of an implementer of a device driver it is actually quite nice to have few functions like get(), set() and action(): The boilerplate code needed for every communication with the device (e.g. spi_acquire() and spi_release()) could be shared, which can reduce the lines of code needed to implement it a bit.

[jia200x]

Reducing the number of "actual" functions in the HAL has proven to reduce the ROM requirements significantly. The benefit of proper type checks and range changes could be provided upon a slim API via static inline functions that encapsulate e.g. the proposed action(), get() and set() functions. I'd even say it would be nice to have the action(), get() and set() interface as "private" interface, and let users only use static inline functions to access them. That would yield an API with easy to use and more descriptive functions that provide both type safety and range check; but without the increased ROM requirements.

I like the idea of having static inline functions. In fact, it would be nice to have them beforehand so we can measure performance differences between different implementations of HAL.

From the perspective of an implementer of a device driver it is actually quite nice to have few functions like get(), set() and action(): The boilerplate code needed for every communication with the device (e.g. spi_acquire() and spi_release()) could be shared, which can reduce the lines of code needed to implement it a bit.

Agreed. That's the point of separating these sub-layers