[riot-notifications] [RIOT-OS/RIOT] net/gnrc/netif: Added CC1xxx adaption layer (#10638)

Martine Lenders notifications at github.com
Wed Jan 9 17:56:40 CET 2019


miri64 requested changes on this pull request.

Some preliminary review.

> +#include "net/gnrc.h"
+#include "net/gnrc/netif/cc1xxx.h"
+
+#define ENABLE_DEBUG    (0)
+#include "debug.h"
+
+#define BCAST  (GNRC_NETIF_HDR_FLAGS_BROADCAST | GNRC_NETIF_HDR_FLAGS_MULTICAST)
+
+static gnrc_pktsnip_t *cc1xxx_adpt_recv(gnrc_netif_t *netif)
+{
+    netdev_t *dev = netif->dev;
+    cc1xxx_rx_info_t rx_info;
+    int pktlen;
+    gnrc_pktsnip_t *payload;
+    gnrc_pktsnip_t *netif_snip;
+    gnrc_pktsnip_t *xhdr_snip;

The to snips above can be unified to e.g. `hdr` as `xhdr_snip` is removed and released before `netif_snip` is allocated. This would safe some code size and stack usage

> +        DEBUG("[cc1xxx-gnrc] recv: unable to mark cc1xxx header snip\n");
+        gnrc_pktbuf_release(payload);
+        return NULL;
+    }
+    gnrc_pktbuf_remove_snip(payload, xhdr_snip);
+
+    /* create a netif hdr from the obtained data */
+    netif_snip = gnrc_netif_hdr_build(&l2hdr.src_addr, CC1XXX_ADDR_SIZE,
+                                      &l2hdr.dest_addr, CC1XXX_ADDR_SIZE);
+    if (netif_snip == NULL) {
+        DEBUG("[cc1xxx-gnrc] recv: unable to allocate netif header\n");
+        gnrc_pktbuf_release(payload);
+        return NULL;
+    }
+    netif_hdr = (gnrc_netif_hdr_t *)netif_snip->data;
+    netif_hdr->if_pid = netif->pid;

Not merged yet, but before we forget, I leave #10661 as a reminder here ;-)

> +    /* get the payload size and the dst address details */
+    size = gnrc_pkt_len(pkt->next);
+    DEBUG("[cc1xxx-gnrc] send: payload of packet is %i\n", (int)size);
+    netif_hdr = (gnrc_netif_hdr_t *)pkt->data;
+
+
+    l2hdr.src_addr = cc1xxx_dev->addr;
+    if (netif_hdr->flags & BCAST) {
+        l2hdr.dest_addr = 0x00;
+        DEBUG("[cc1xxx-gnrc] send: preparing to send broadcast\n");
+    }
+    else {
+        /* check that destination address is valid */
+        assert(netif_hdr->dst_l2addr_len > 0);
+        uint8_t *addr = gnrc_netif_hdr_get_dst_addr(netif_hdr);
+        l2hdr.dest_addr = addr[netif_hdr->dst_l2addr_len - 1];

Any reason, why the least and not the most significant byte (which I would expect) is used?

> + * @ingroup     drivers_netdev
+ * @ingroup     net_gnrc_netif
+ * @{
+ *
+ * @file
+ * @brief   CC110x/CC1200 adaption for @ref net_gnrc_netif
+ *
+ * @author  Martine Lenders <m.lenders at fu-berlin.de>
+ * @author  Hauke Petersen <hauke.petersen at fu-berlin.de>
+ * @author  Marian Buschsieweke <marian.buschsieweke at ovgu.de>
+ *
+ * Supported Transceivers
+ * ======================
+ *
+ * This adaption layer is written to be used by CC110x and CC1200 transceivers,
+ * but any transceiver using that should transfer 6LoWPAN and uses layer 2

Why restrict to 6LoWPAN? Any payload should work, right?

> + *
+ * @author  Martine Lenders <m.lenders at fu-berlin.de>
+ * @author  Hauke Petersen <hauke.petersen at fu-berlin.de>
+ * @author  Marian Buschsieweke <marian.buschsieweke at ovgu.de>
+ *
+ * Supported Transceivers
+ * ======================
+ *
+ * This adaption layer is written to be used by CC110x and CC1200 transceivers,
+ * but any transceiver using that should transfer 6LoWPAN and uses layer 2
+ * addresses which are 1 byte in size would likely be able to use it.
+ *
+ * Frame Format
+ * ============
+ *
+ * ```

<code>```</code> fencing does not exist in doxygen, please use `~~~~~~~~~` (or just indent by 4 more spaces since you don't require any syntax highlighting)

> + *
+ * | Field       | Description                     |
+ * |-------------|---------------------------------|
+ * | Destination | The layer 2 destination address |
+ * | Source      | The layer 2 source address      |
+ * | Payload     | The payload (variable size)     |
+ *
+ * Please note that the payload needs to be able to carry 6LoWPAN frames.
+ *
+ * Layer 2 Broadcast
+ * =================
+ *
+ * This adaption layer assumes that the layer 2 address `0x00` is reserved for
+ * layer 2 broadcast, which is true for CC110x and CC1200 transceivers (provided
+ * they are configured accordingly). If more users of this adaption layers are
+ * added, this behaviour might needs to be more generalized.

Why don't we do it now by providing an `#ifndef` configurable broadcast address in this header. This also would prevent the usage of the "magic number" (yes it's `0x00` I know) in https://github.com/RIOT-OS/RIOT/pull/10638/files#diff-dc0ba564bc6c2ba0e0d59f2517bbc8d6R124.

> +/**
+ * @brief Default protocol for data that is coming in
+ */
+#ifdef MODULE_GNRC_SIXLOWPAN
+#define CC1XXX_DEFAULT_PROTOCOL         (GNRC_NETTYPE_SIXLOWPAN)
+#else
+#define CC1XXX_DEFAULT_PROTOCOL         (GNRC_NETTYPE_UNDEF)
+#endif
+
+/**
+ * @brief Layer 2 header size used
+ *
+ * This includes only the Destination and Source address, as only those have
+ * impact on the Length Filed as expected/set by the transceiver
+ */
+#define CC1XXX_HEADER_SIZE              2

parens

> +#else
+#define CC1XXX_DEFAULT_PROTOCOL         (GNRC_NETTYPE_UNDEF)
+#endif
+
+/**
+ * @brief Layer 2 header size used
+ *
+ * This includes only the Destination and Source address, as only those have
+ * impact on the Length Filed as expected/set by the transceiver
+ */
+#define CC1XXX_HEADER_SIZE              2
+
+/**
+ * @brief Size of a layer 2 address on CC110x/CC1200 transceivers
+ */
+#define CC1XXX_ADDR_SIZE                1

parens

> +/**
+ * @brief Default protocol for data that is coming in
+ */
+#ifdef MODULE_GNRC_SIXLOWPAN
+#define CC1XXX_DEFAULT_PROTOCOL         (GNRC_NETTYPE_SIXLOWPAN)
+#else
+#define CC1XXX_DEFAULT_PROTOCOL         (GNRC_NETTYPE_UNDEF)
+#endif
+
+/**
+ * @brief Layer 2 header size used
+ *
+ * This includes only the Destination and Source address, as only those have
+ * impact on the Length Filed as expected/set by the transceiver
+ */
+#define CC1XXX_HEADER_SIZE              2

Also, why not just use `sizeof(cc1xxx_l2hdr_t)` instead of this macro?

> + * impact on the Length Filed as expected/set by the transceiver
+ */
+#define CC1XXX_HEADER_SIZE              2
+
+/**
+ * @brief Size of a layer 2 address on CC110x/CC1200 transceivers
+ */
+#define CC1XXX_ADDR_SIZE                1
+
+/**
+ * @brief Layer 2 header used in CC1xxx frames
+ *
+ * This structure has the same memory layout as the data send in the frames.
+ */
+typedef struct __attribute__((packed)) {
+    uint8_t dest_addr;      /**< Destination Layer-2 address */

Either use `Layer-2` or `Layer 2`, not both (I prefer `Layer 2`)

-- 
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/10638#pullrequestreview-190805666
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190109/135a74f6/attachment.html>


More information about the notifications mailing list