Memory leak in HCICordioTransportClass

[Wenn0101]

I am working on porting this library to a new target, and I have found what I believe may be a memory leak in the HCICordioTransportClass::write() function.

In file \ArduinoBLE\src\utility\HCICordioTransport.cpp

size_t HCICordioTransportClass::write(const uint8_t* data, size_t length)
{
  if (!_begun) {
    return 0;
  }

  uint8_t packetLength = length - 1;
  uint8_t packetType   = data[0];

#if CORDIO_ZERO_COPY_HCI
  uint8_t* packet = (uint8_t*)WsfMsgAlloc(max(packetLength, MIN_WSF_ALLOC));

  memcpy(packet, &data[1], packetLength);

  return CordioHCIHook::getTransportDriver().write(packetType, packetLength, packet);
#else
  return CordioHCIHook::getTransportDriver().write(packetType, packetLength, (uint8_t*)&data[1]);
#endif
}

If CORDIO_ZERO_COPY_HCI is defined, memory is allocated, and I am never seeing it freed. Where are these messages supposed to be freed?

Locally I have made changes so that the allocated memory is freed after transmitting.This has solved my problem of running out of space and receiving a null pointer from WsfMsgAlloc(). However, I get the feeling that I am missing something about how this is supposed to work.

My code is:

size_t HCICordioTransportClass::write(const uint8_t* data, size_t length)
{
  if (!_begun) {
    return 0;
  }

  uint8_t packetLength = length - 1;
  uint8_t packetType   = data[0];

#if CORDIO_ZERO_COPY_HCI
  uint8_t* packet = (uint8_t*)WsfMsgAlloc(max(packetLength, MIN_WSF_ALLOC));
  if(packet)
  {
    memcpy(packet, &data[1], packetLength);

    uint16_t writtenBytes = CordioHCIHook::getTransportDriver().write(packetType, packetLength, packet);
    WsfMsgFree(packet);
    return writtenBytes;
  }
  else
  {
    return 0;
  }
#else
  return CordioHCIHook::getTransportDriver().write(packetType, packetLength, (uint8_t*)&data[1]);
#endif
}

[polldo]

Hi @Wenn0101 ,
when using CORDIO_ZERO_COPY_HCI, the ownership of the buffer containing the message is passed to the controller.
So you should not worry about freeing the allocated memory because it will be handled by the controller.
https://github.com/ARMmbed/mbed-os/blob/6a244d7adffc0e93872cfc880e539ee11bbc6002/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NORDIC_CORDIO/TARGET_NRF5x/NRFCordioHCITransportDriver.cpp#L49-L50

[Wenn0101]

Hi @polldo Thanks for the response.

Taking a closer look, I am beginning to understand more the purpose of this CORDIO_ZERO_COPY_HCI define, and that it has a matching #define in the mbed core.

I guess my problems come back to defining this when the target I am using does not utilize this in the mbed core. Right now it seems to me that this CORDIO_ZERO_COPY_HCI is used to guard enough in the arduinoBLE library that the library cannot be used without this defined. Is this correct?

Why is the CORDIO_ZERO_COPY_HCI define around the functionality that makes the ble loop function? The only place it makes sense in my mind is within the size_t HCICordioTransportClass::write(const uint8_t* data, size_t length) function.

For example why is CORDIO_ZERO_COPY_HCI used in the functions below

#if CORDIO_ZERO_COPY_HCI
extern uint8_t *SystemHeapStart;
extern uint32_t SystemHeapSize;

void init_wsf(ble::vendor::cordio::buf_pool_desc_t& buf_pool_desc) {
    static bool init = false;

    if (init) {
      return;
    }
    init = true;

    // use the buffer for the WSF heap
    SystemHeapStart = buf_pool_desc.buffer_memory;
    SystemHeapSize = buf_pool_desc.buffer_size;

    // Initialize buffers with the ones provided by the HCI driver
    uint16_t bytes_used = WsfBufInit(
        buf_pool_desc.pool_count,
        (wsfBufPoolDesc_t*)buf_pool_desc.pool_description
    );

    // Raise assert if not enough memory was allocated
    MBED_ASSERT(bytes_used != 0);

    SystemHeapStart += bytes_used;
    SystemHeapSize -= bytes_used;

    WsfTimerInit();
}

extern "C" void wsf_mbed_ble_signal_event(void)
{
    // do nothing
}
#endif //CORDIO_ZERO_COPY_HCI

static void bleLoop()
{
#if CORDIO_ZERO_COPY_HCI
    uint64_t last_update_us = 0;
    mbed::LowPowerTimer timer;

    timer.start();

    while (true) {
        last_update_us += (uint64_t) timer.read_high_resolution_us();
        timer.reset();

        uint64_t last_update_ms = (last_update_us / 1000);
        wsfTimerTicks_t wsf_ticks = (last_update_ms / WSF_MS_PER_TICK);

        if (wsf_ticks > 0) {
            WsfTimerUpdate(wsf_ticks);
            last_update_us -= (last_update_ms * 1000);
        }

        wsfOsDispatcher();

        bool sleep = false;
        {
            /* call needs interrupts disabled */
            mbed::CriticalSectionLock critical_section;
            if (wsfOsReadyToSleep()) {
                sleep = true;
            }
        }

        uint64_t time_spent = (uint64_t) timer.read_high_resolution_us();

        /* don't bother sleeping if we're already past tick */
        if (sleep && (WSF_MS_PER_TICK * 1000 > time_spent)) {
            /* sleep to maintain constant tick rate */
            uint64_t wait_time_us = WSF_MS_PER_TICK * 1000 - time_spent;
            uint64_t wait_time_ms = wait_time_us / 1000;

            wait_time_us = wait_time_us % 1000;

            if (wait_time_ms) {
              rtos::ThisThread::sleep_for(wait_time_ms);
            }

            if (wait_time_us) {
              wait_us(wait_time_us);
            }
        }
    }
#else
    while(true) {
        rtos::ThisThread::sleep_for(osWaitForever);
    }
#endif // CORDIO_ZERO_COPY_HCI
}

Or to put it another way, perhaps I should re-name my issue " Library does not work without CORDIO_ZERO_COPY_HCI defined ".

Sorry to ask the same question in so many ways. I think I may just be missing a piece of how this library is supposed to work, and the mismatch in my mind is that the CORDIO_ZERO_COPY_HCI is used to make a lot more decision than how to send and receive data to the HCI layer, it is used to determine the logic of bleLoop and init_wsf and wsf_mbed_ble_signal_event.

[Wenn0101]

misclick closing issue, I would appreciate feedback on my comment.

[polldo]

@Wenn0101 you are right, at the moment HCICordioTransport is intended to work only with HCI_ZERO_COPY defined.
If you need to work without HCI_ZERO_COPY check this out ARMmbed/mbed-os-cordio-hci-passthrough#2
Basically the packets are inserted in a static buffer rather than being encapsulated in a dynamically allocated wsf_msg.

[polldo]

@Wenn0101 I would like to be more precise. Our HCICordioTransport actually supports two kind of operation:

• The first consists in using the cordio controller for managing the ble radio. In this case ArduinoBLE, as a host library, communicates with the controller through a fake uart channel. Here the HCI_ZERO_COPY plus a thread for dispatching ble events are needed. This method is used for the nano33ble board.
• The second consists in bypassing also the cordio controller. In this case, cordio is used just for managing the uart channel with the ble module and other physical connections. So, here HCI_ZERO_COPY and the thread are not needed, the cordio write and receive functions are used directly to forward packets to/from the uart channel. This method is used for the portenta h7 board.

[Wenn0101]

@polldo Thanks for the clarification.

I think my problem really stems from the fact that I am porting a target that:
-uses the mbed CORDIO stack to manage an onboard radio (the driver uses wsf events)
-does not utilize the CORDIO_LL in mbed (and thus does not use HCI_ZERO_COPY) (the on-chip ble controller accept a HCI-like data interface)

This left me wanting to utilize the bleloop as if I was part of your first group, but the target was not configured to free the HCI messages as if i was in the latter group.
I decided to work around this by simply enabling HCI_ZERO_COPY in mbed, and having my driver free the buffer after it passes the data to the hardware.

This is a suitable workaround, and we can close this issue if you would like.

I think in my mind it is still lingering that the macro HCI_ZERO_COPY is being used to determine whether the host and controller are combined on a single chip solution, but there can exist cases where an single chip solution does not want to utilize HCI_ZERO_COPY.

If you are curious the mbed target I am porting can be found here.

[polldo]

@Wenn0101 your workaround sounds good to me.

there can exist cases where an single chip solution does not want to utilize HCI_ZERO_COPY

I agree with you, supporting this use case could be a future enhancement.
However nice porting!
I'm going to close this issue, but feel free to open another one if you happen to have some question or request.
Thanks!