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.