netdev_new_api: allow .send() to return > 0 to signal synchronos send

[benpicco]

Contribution description

The new netdev API makes implementing drivers where .send is inherently synchronous (slip, dose, ethos) kind of awkward. They need to memorize the frame size, generate an event, and return the frame size in .confirm_send to signal completion.

However, the new netdev API does not use .send return values that are > 0. We can use those to signal that send is complete already and no further action is needed.

This makes writing and understanding synchronos drivers much easier, the code changes to support this in GNRC and LWIP are minimal.

Testing procedure

You can test with SLIP with is now trivial to convert:

slip.patch

--- a/drivers/slipdev/Makefile.dep
+++ b/drivers/slipdev/Makefile.dep
@@ -1,6 +1,6 @@
 USEMODULE += chunked_ringbuffer
 USEMODULE += eui_provider
-USEMODULE += netdev_legacy_api
+USEMODULE += netdev_new_api
 USEMODULE += netdev_register
 FEATURES_REQUIRED += periph_uart
 
diff --git a/drivers/slipdev/slipdev.c b/drivers/slipdev/slipdev.c
index ff6880a5b6..b0c0c827ff 100644
--- a/drivers/slipdev/slipdev.c
+++ b/drivers/slipdev/slipdev.c
@@ -234,10 +234,6 @@ static int _send(netdev_t *netdev, const iolist_t *iolist)
     slipdev_write_byte(dev->config.uart, SLIPDEV_END);
     slipdev_unlock();
 
-    if (netdev->event_callback) {
-        netdev->event_callback(netdev, NETDEV_EVENT_TX_COMPLETE);
-    }
-
     return bytes;
 }
 
@@ -327,9 +323,6 @@ static int _get(netdev_t *netdev, netopt_t opt, void *value, size_t max_len)
             assert(max_len == sizeof(uint16_t));
             *((uint16_t *)value) = NETDEV_TYPE_SLIP;
             return sizeof(uint16_t);
-        case NETOPT_TX_END_IRQ:
-            *((netopt_enable_t *)value) = NETOPT_ENABLE;
-            return sizeof(netopt_enable_t);
 #if IS_USED(MODULE_SLIPDEV_L2ADDR)
         case NETOPT_ADDRESS_LONG:
             assert(max_len == sizeof(eui64_t));
@@ -341,12 +334,20 @@ static int _get(netdev_t *netdev, netopt_t opt, void *value, size_t max_len)
     }
 }
 
+static int _confirm_send(netdev_t *netdev, void *info)
+{
+    (void)netdev;
+    (void)info;
+    return 0;
+}
+
 static const netdev_driver_t slip_driver = {
     .send = _send,
     .recv = _recv,
     .init = _init,
     .isr = _isr,
     .get = _get,
+    .confirm_send = _confirm_send,
 #if IS_USED(MODULE_SLIPDEV_STDIO)
     .set = netdev_set_notsup,
 #else

Issues/PRs references

[riot-ci]

Murdock results

✔️ PASSED

4eb1c35 cpu/sam0_eth: fix return values of sam0_eth_send()

Success	Failures	Total	Runtime
10249	0	10249	17m:28s

Artifacts

• Documentation preview

[maribu]

where .send is inherently synchronous (slip, dose, ethos) kind of awkward.

It is not actually inherently synchronous, e.g. with DMA it is possible and in fact quite sensible to have UART asynchronous. Even if only an TX done IRQ is available, providing an async UART interface would be possible with the ISR feeding the data into the UART while allowing the CPU to do other things in between the ISR.

I wonder if we are here digging in deeper into the whole of the synchronous periph APIs.

[maribu]

Let's rather revert the slip netdev to the legacy API. No need to add a third case, as this matches behaviour of the legacy API already.

[benpicco]

If the driver becomes async, it can easily be converted to return 0 on send - but e.g. dose will likely always be synchronous as it does handle collision detection and retransmissions.

Let's rather revert the slip netdev to the legacy API. No need to add a third case, as this matches behaviour of the legacy API already.

The nice thing about the new API is the change in memory ownership and that we can be a lot more strict about how it behaves. e.g. in the synchronous send case we don't need to generate an event, the legacy API would just generate events 'just in case' - or not, depending on what the driver author felt like.

The goal would be to get rid of all the netdev_legacy_api cases eventually.

[maribu]

OK, makes sense