Tool/software: TI-RTOS
Hi,
I'm porting some code from an MSP432P401R to an MSP432P401M so my SRAM has reduced from 64KB to 32KB. I avoid dynamic memory allocation in my code, but it's used extensively throughout your drivers and the bluetooth stack, previously I allocated 16KB for the heap, but no longer have room for that so have had to reduce it to 7 KB. This isn't super relevant, but the point is that simplelink_msp432_sdk_bluetooth_plugin_1_25_00_42 has inadequate error handling and will leak memory if initialisation fails.
Relevant config: I'm using UART transport with sapParams.port.remote.stackSize = 2048.
There's a few different cases that I will go through, but the relevant function that's called from my code is always SAP_open.
Going through its code, it initially does default parameter, callback configuration. None of this requires cleanup if it fails, so that's fine. Then SNP_open is called which doesn't allocate memory, but does require that SNP_close is called to destroy its semaphores. So if the passed in portType is invalid, we return an error without calling SNP_close. This doesn't leak memory or infinitely loop, but is still not ideal. Eventually we get into NPITask_open which is where most of the issues happen (npi_task_posix.c).
We have the same sem_init call that'll never be destroyed, but the main issues are that npiTxQueue, npiRxQueue, npiSyncRxQueue, npiSyncTxQueue and npiPacketQueue are all opened (which does an allocation) without checking the result. So some of these could have been allocated, while others may not have. A little further down, if npiTaskStack fails to be allocated, then we've just leaked several kilobytes of RAM that we'll never get back. We could potentially call SAP_close even if SAP_open fails, but that will be very error prone because it assumes that SAP_open was successful, and if for example one of the queues failed to allocate, then we'll try to call mq_free on -1 which then tries to call Memory_free on it, which will cause all sorts of issues. So, basically, NPITask_open desperately needs error handling and to free any allocated resources if it fails. Also, it seems that npiTaskStack is completely redundant and never even used? pthread_attr_setstacksize will cause pthread_create to allocate the stack anyway, so we've just wasted some more RAM for no reason at all.
Moving onto NPITL_openTL we get both memory leaks (npiRxBuf, npiTxBuf), potential memory corruption if one of the buffers fails to allocate (in reality it'll be trying to overwrite 0x00000000 which will be a no-op because it's flash, but still not good practice) and infinite loops within transportOpen. transportOpen is aliased to NPITLUART_openTransport for me, so off to there I go.
There's two more queues that are potentially leaked (uartQueueRec, uartQueueSend) but more annoyingly, the "error handling" for pthread calls is to simply infinitely loop. I'm writing firmware for a battery operated device where bluetooth is mainly a convenience for configuration. If it fails, that's a shame and I guess I could reset if worst came to worst. However, if the initialisation never actually returns and prevents low power modes, I'm going to run out of battery fairly quickly which makes the entire device useless.
Sorry for the long winded post, but can we please get some error handling within this supposedly release quality code? I haven't listed all of the leaks, etc. but I feel like I've illustrated my point. I'm already compiling it myself, so I'm going to go through and fix issues that affect me, but I can't be the only person bothered by this. At a minimum can we get:
- Return value for NPITLUART_openTransport that indicates error (after cleaning up any allocated resources). Presumably there's the same issues with SPI.
- Resource cleanup in NPITask_open and SAP_open.
- Guarantees when SAP_open doesn't return SNP_SUCCESS that we at least haven't leaked any resources.
- Ability to configure queue depths so we can adjust the memory requirements for more resource constraint devices. I wonder if it makes sense to just put _npiFrame_t on the queues in npi_task_posix.c instead of pointers, to save 4x16x4 = 256 bytes.
