cmsis_os.c osPoolAlloc error?

[wlad]

Posted on July 19, 2016 at 14:05

lines in osPoolAlloc function:

index = pool_id->currentIndex + i;if (index >= pool_id->pool_sz) {  index = 0;}

must be replaced with:

index = (pool_id->currentIndex + i) % pool_id->pool_sz;

[wlad]

Posted on July 20, 2016 at 10:42

I've decided to rewrite the whole function :)

void* osPoolAlloc( osPoolId pool_id )

{

  if( pool_id == NULL ) return NULL;

  uint32_t  pool_sz      = pool_id->pool_sz     ;

  uint32_t  currentIndex = pool_id->currentIndex;

  uint8_t * markers      = pool_id->markers     ;

  void    * result       = NULL                 ;

  int       dummy        = 0                    ;

  if (inHandlerMode()) {

    dummy = portSET_INTERRUPT_MASK_FROM_ISR();

  }

  else {

    vPortEnterCritical();

  }

  for( uint32_t i = 0; i < pool_sz; i++ ) {

    if( markers[currentIndex] == 0 ) {

      markers[currentIndex] = 1;

      result = (void *)((uint32_t)(pool_id->pool) + (currentIndex * pool_id->item_sz));

      currentIndex = (currentIndex + 1) % pool_sz;

      for( ++i; i < pool_sz; i++ ) { 

        if( markers[ currentIndex ] == 0 ) break;

        currentIndex = (currentIndex + 1) % pool_sz;

      }

      pool_id->currentIndex = currentIndex;

      break;

    }

    currentIndex = (currentIndex + 1) % pool_sz;

  }

  if (inHandlerMode()) {

    portCLEAR_INTERRUPT_MASK_FROM_ISR(dummy);

  }

  else {

    vPortExitCritical();

  }

  return result;

}

[Richard I]

Posted on February 07, 2018 at 11:58

Sorry about reviving an old topic, but the original poster is 100% correct. There is definitely a major bug in osPoolAlloc() in CMSIS_RTOS/cmsis_os.c . The bug is that when the block at index[0] is not marked as free, and the pool was previously full, the pool will fail to allocate other, already freed blocks. Steps to reproduce:

• Create a memory pool of 10 items
• Allocate all 10 items
• Free the last 5 items
• Try to allocate another item - the allocation will fail
• Free the first item, then retry allocation, and it will work

Faulty, original code - note that in the event of index > pool_sz, the index will be stuck at zero for the rest of the loop, if the marker[0] is not free:

for (i = 0; i < pool_id->pool_sz; i++) {

   index = pool_id->currentIndex + i;   if (index >= pool_id->pool_sz) {       index = 0;   }   if (pool_id->markers[index] == 0) {       pool_id->markers[index] = 1;       p = (void *)((uint32_t)(pool_id->pool) + (index * pool_id->item_sz));       pool_id->currentIndex = index;       break;    }}

Fixed code:

index = pool_id->currentIndex; // ADDED

for (i = 0; i < pool_id->pool_sz; i++) {      // FAULTY LOGIC REMOVED      index %= pool_id->pool_sz; // ADDED, will wrap index around the bounds       if (pool_id->markers[index] == 0) {          pool_id->markers[index] = 1;          p = (void *)((uint32_t)(pool_id->pool) + (index * pool_id->item_sz));          pool_id->currentIndex = index;          break;       }       index++; // ADDED }

On another note, it is a bit wasteful to use full bytes to mark occupied blocks. A bitfield would have sufficed and would have required up to 8x less heap.