[riot-notifications] [RIOT-OS/RIOT] pkg/nimble: add IP-over-BLE support via netif/GNRC (#11578)

Martine Lenders notifications at github.com
Mon May 27 14:51:10 CEST 2019


miri64 requested changes on this pull request.

Now some more in-depth review. As stated at first-glance, for the most part the glue code is ok, but the error codes are not mapped properly for the upper layer.

> +#include "nimble_riot.h"
+#include "host/ble_gap.h"
+#include "host/util/util.h"
+
+#define ENABLE_DEBUG            (0)
+#include "debug.h"
+
+#ifdef MODULE_GNRC_SIXLOWPAN
+#define NETTYPE                 GNRC_NETTYPE_SIXLOWPAN
+#else
+#define NETTYPE                 GNRC_NETTYPE_UNDEF
+#endif
+
+/* maximum packet size for IPv6 packets */
+#ifndef NIMBLE_NETIF_IPV6_MTU
+#define NIMBLE_NETIF_IPV6_MTU    (1280U)    /* as specified in RFC7668 */

Mh... Does this really need to be defined so tight nimble? Seems more like a BLE-over-IPv6-specific thing than a nimble specific thing to me.

> +
+static int _netif_send_iter(nimble_netif_conn_t *conn,
+                            int handle, void *arg)
+{
+    (void)handle;
+    _send_pkt(conn, (gnrc_pktsnip_t *)arg);
+    return 0;
+}
+
+static int _netif_send(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt)
+{
+    (void)netif;
+    int res;
+
+    if (pkt->type != GNRC_NETTYPE_NETIF) {
+        return NIMBLE_NETIF_DEVERR;

This should be an `assert()`.

> +    if (hdr->flags &
+        (GNRC_NETIF_HDR_FLAGS_BROADCAST | GNRC_NETIF_HDR_FLAGS_MULTICAST)) {
+        nimble_netif_conn_foreach(NIMBLE_NETIF_L2CAP_CONNECTED,
+                                  _netif_send_iter, pkt->next);
+        res = (int)gnrc_pkt_len(pkt->next);
+    }
+    /* send unicast */
+    else {
+        int handle = nimble_netif_conn_get_by_addr(
+            gnrc_netif_hdr_get_dst_addr(hdr));
+        nimble_netif_conn_t *conn = nimble_netif_conn_get(handle);
+        if (conn) {
+            res = _send_pkt(conn, pkt->next);
+        }
+        else {
+            res = NIMBLE_NETIF_NOTCONN;

This should be or map to an `errno` (e.g. `ENOTCONN`) as per documentation of [the `send` operation](https://doc.riot-os.org/structgnrc__netif__ops.html#a86afb0a1108b6bbacf281c4aba13824c).

> +
+static void _netif_init(gnrc_netif_t *netif)
+{
+    (void)netif;
+
+    /* save the threads context pointer, so we can set its flags */
+    _netif_thread = (thread_t *)thread_get(thread_getpid());
+
+#ifdef MODULE_GNRC_SIXLOWPAN
+    /* we disable fragmentation for this device, as the L2CAP layer takes care
+     * of this */
+    _nimble_netif->sixlo.max_frag_size = 0;
+#endif
+}
+
+static int _send_pkt(nimble_netif_conn_t *conn, gnrc_pktsnip_t *pkt)

Error codes returned by this function should be converted or mapped to the error codes expected by the upper layers (`errno`) as its return value is used by the `send` method [below](https://github.com/RIOT-OS/RIOT/pull/11578/files#diff-0fddc9bbf97a3532b485226bb4642e85R181).

> +#include "net/gnrc/pktbuf.h"
+#include "net/gnrc/nettype.h"
+
+#include "nimble_netif.h"
+#include "nimble_netif_conn.h"
+#include "nimble_riot.h"
+#include "host/ble_gap.h"
+#include "host/util/util.h"
+
+#define ENABLE_DEBUG            (0)
+#include "debug.h"
+
+#ifdef MODULE_GNRC_SIXLOWPAN
+#define NETTYPE                 GNRC_NETTYPE_SIXLOWPAN
+#else
+#define NETTYPE                 GNRC_NETTYPE_UNDEF

As you use this to dispatch to the upper layer directly, shouldn't there also a fall-back to IPv6 if it is available but 6Lo is not?

> +    if (sdu == NULL) {
+        return NIMBLE_NETIF_NOMEM;
+    }
+    while (pkt) {
+        res = os_mbuf_append(sdu, pkt->data, pkt->size);
+        if (res != 0) {
+            os_mbuf_free_chain(sdu);
+            return NIMBLE_NETIF_NOMEM;
+        }
+        num_bytes += (int)pkt->size;
+        pkt = pkt->next;
+    }
+
+    /* send packet via the given L2CAP COC */
+    do {
+        res = ble_l2cap_send(conn->coc, sdu);

It doesn't look like this function returns negative `errno`. Please map.

> +    /* finally dispatch the receive packet to GNRC */
+    if (!gnrc_netapi_dispatch_receive(payload->type, GNRC_NETREG_DEMUX_CTX_ALL,
+                                      payload)) {
+        gnrc_pktbuf_release(payload);
+    }
+
+end:
+    /* copy the receive data and free the mbuf */
+    os_mbuf_free_chain(rxb);
+    /* free the mbuf and allocate a new one for receiving new data */
+    rxb = os_mbuf_get_pkthdr(&_mbuf_pool, 0);
+    /* due to buffer provisioning, there should always be enough space */
+    assert(rxb != NULL);
+    ble_l2cap_recv_ready(event->receive.chan, rxb);
+
+    return ret;

If I see correctly the return value is never used. So is it needed?

-- 
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/11578#pullrequestreview-242234720
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190527/6ebc0f6c/attachment.html>


More information about the notifications mailing list