USB NCP data corruption

[andrewleech]

Hello,
I'm working on integrating NCP support into micropython in micropython/micropython#16459

I initially followed the lib/tinyusb/examples/device/net_lwip_webserver/src/main.c example to bind the NCM class support to the existing LWIP integration in micropython and had it working relatively easily, thanks!

However I found that if I started send / recv more than ~230 bytes at a time the received data was corrupted at the start of reception, eg the following log should be all a

I was able to fix this by removing the use of

/* shared between tud_network_recv_cb() and service_traffic() */
static struct pbuf *received_frame;

and essentially replacing the example code:

bool tud_network_recv_cb(const uint8_t *src, uint16_t size) {
  /* this shouldn't happen, but if we get another packet before
  parsing the previous, we must signal our inability to accept it */
  if (received_frame) return false;

  if (size) {
    struct pbuf *p = pbuf_alloc(PBUF_RAW, size, PBUF_POOL);

    if (p) {
      /* pbuf_alloc() has already initialized struct; all we need to do is copy the data */
      memcpy(p->payload, src, size);

      /* store away the pointer for service_traffic() to later handle */
      received_frame = p;
    }
  }

  return true;
}

static void service_traffic(void) {
  /* handle any packet received by tud_network_recv_cb() */
  if (received_frame) {
    ethernet_input(received_frame, &netif_data);
    pbuf_free(received_frame);
    received_frame = NULL;
    tud_network_recv_renew();
  }

  sys_check_timeouts();
}

with

bool tud_network_recv_cb(const uint8_t *src, uint16_t size) {
    struct netif *netif = &(usbnet_obj.netif);
    bool ret = true;
    if (size) {
        struct pbuf *p = pbuf_alloc(PBUF_RAW, size, PBUF_POOL);
        if (p == NULL) {
            error_printf("tud_network_recv_cb() failed to alloc pbuf %d\n", size);
            return false;
        }

        // Copy buf to pbuf
        pbuf_take(p, src, size);

        if (netif->input(p, netif) != ERR_OK) {   // netif->input is configured as ethernet_input
            error_printf("tud_network_recv_cb() netif input failed\n");
            pbuf_free(p);
            ret = false;
        }
        // We need to signal to TinyUSB to continue processing after this current context is complete
        mp_sched_schedule_node(&network_usbd_ncm_sched_node, _scheduled_tud_network_recv_renew);
    }
    return ret;
}

static void _scheduled_tud_network_recv_renew(mp_sched_node_t *node) {
    // This will unblock the usb network to continue reception.
    tud_network_recv_renew();
}

I think the key changes were replacing memcpy with pbuf_take() and not calling pbuf_free(received_frame); after ethernet_input(received_frame, &netif_data); unless there's an error. I copied this pattern from other working network drivers in micropython and don't seem to have run out of lwip buffers; I gather ethernet_input() frees the buffer itself on success?

I don't have hardware / build env set up to try making similar changes to the actual example code here sorry but wanted to share my findings in case they're relevant and helpful.

[andrewleech]

Oh, my mistake, it seems I was working with an older pinned submodule that didn't include #2828
The root issue here has already been fixed in #2829