[riot-notifications] [RIOT-OS/RIOT] sys/net/gnrc/tx_sync: new module (#15694)

Martine Lenders notifications at github.com
Fri Jan 8 17:02:17 CET 2021


@miri64 commented on this pull request.

Some initial thoughts.

> @@ -148,7 +148,18 @@ typedef enum {
      * @}
      */
 
-    GNRC_NETTYPE_NUMOF,         /**< maximum number of available protocols */
+    /**
+     * @brief   maximum number of available protocols
+     *
+     * @details The special type GNRC_NETTYPE_TX_SYNC is not counted, as it
+     *          is a non-network protocol type
+     */
+    GNRC_NETTYPE_NUMOF,

This might cause problems with `gnrc_netreg`, as it uses `GNRC_NETTYPE_NUMOF` for its internal registry array. Can you ensure that this won't be a problem? Alternatively you can go the (IMHO more scalable) route we already went for `GNRC_NETTYPE_NETIF` and make it a negative number.

> +/**
+ * @brief   Signal TX completion via the given tx sync packet snip
+ *
+ * @pre     Module gnrc_netttype_tx_sync is sued
+ * @pre     `pkt->type == GNRC_NETTYPE_TX_SYNC`
+ *
+ * @param   The tx sync packet snip of the packet that was transmitted
+ */
+static inline void gnrc_tx_complete(gnrc_pktsnip_t *pkt)
+{
+    assert(IS_USED(MODULE_GNRC_TX_SYNC) && (pkt->type == GNRC_NETTYPE_TX_SYNC));
+    /* Allow for multiple waiters by just unlocking the mutex until all
+     * blocked threads have resumed */
+    gnrc_tx_sync_t *sync = pkt->data;
+    do {
+        mutex_unlock(&sync->signal);

Unlocking multiple mutexes makes me think if `signal` shouldn't rather be a semaphore or a condition variable.

> +    gnrc_pktsnip_t *tx_sync;
+    tx_sync = IS_USED(MODULE_GNRC_TX_SYNC) ? gnrc_tx_sync_split(pkt) : NULL;

Why not just one line?

```suggestion
    gnrc_pktsnip_t *tx_sync = IS_USED(MODULE_GNRC_TX_SYNC)
                            ? gnrc_tx_sync_split(pkt) : NULL;
```

> -            _free(pkt->data);
+            if (!IS_USED(MODULE_GNRC_TX_SYNC)
+                || (pkt->type != GNRC_NETTYPE_TX_SYNC)) {
+                _free(pkt->data);
+            }
+            else {
+                gnrc_tx_complete(pkt);
+            }

It worries me a bit, that this needs to be packet so deep into the packet buffer :-/

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/15694#pullrequestreview-564373006
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210108/98e18232/attachment.htm>


More information about the notifications mailing list