Skip to main content
rmawatson
Associate II
November 11, 2015
Question

STM32Cube FreeRTOS cmsis_os.c osPoolAlloc bug

  • November 11, 2015
  • 1 reply
  • 659 views
Posted on November 11, 2015 at 18:58

Hi,

This code generated by STM32Cube with FreeRTOS looks like it has a bug. 

To reproduce, create a small pool. Allocate a single item that is not free'd then, allocate and deallocate sucessive items. The pool will stop allocating, even though there are free slots.

The code at fault is,

    index = pool_id->currentIndex + i;

    if (index >= pool_id->pool_sz) {

      index = 0;

    }

currentIndex is never reset, and so for a queue size of say 8, if currentIndex = 7, then for all i (1,2,3..) in the loop, currentindex + i will be greater than pool_sz. and index will be set to 0, so only markers[0]  gets checked continiously. If index 0 is not free, then the pool will not allocate, and will break in this condition

The above section should be something like 

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

FULL FUNCTION:

void *osPoolAlloc (osPoolId pool_id)

{

  int dummy = 0;

  void *p = NULL;

  uint32_t i;

  uint32_t index;

  if (inHandlerMode()) {

    dummy = portSET_INTERRUPT_MASK_FROM_ISR();

  }

  else {

    vPortEnterCritical();

  }

  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;

    }

  }

  if (inHandlerMode()) {

    portCLEAR_INTERRUPT_MASK_FROM_ISR(dummy);

  }

  else {

    vPortExitCritical();

  }

  return p;

}
    This topic has been closed for replies.

    1 reply

    thomfischer
    Senior
    November 12, 2015
    Posted on November 12, 2015 at 10:44

    Hello,

    maybe there is another error in function

    osPoolCAlloc

    void

    *

    osPoolCAlloc

    (

    osPoolId

    pool_id)

    {

    void

    *p = osPoolAlloc(pool_id);

    if

    (p != NULL)

    {

    //

    memset

    (p, 0,

    sizeof

    (pool_id->pool_sz));

    ERROR this is size of uint32_t = 4

    memset

    (p, 0, pool_id->

    item_sz

    ); //

    should be

    }

    return

    p;

    }

    my proposel for

    osPoolAlloc()

    void

    *

    osPoolAlloc

    (

    osPoolId

    pool_id)

    {

    int

    dummy = 0;

    void

    *p = NULL;

    uint32_t

    i;

    uint32_t

    index;

    if

    (inHandlerMode()) {

    dummy = portSET_INTERRUPT_MASK_FROM_ISR();

    }

    else

    {

    vPortEnterCritical();

    }

    index=pool_id->

    currentIndex

    ;

    // +++ new

    for

    (i = 0; i < pool_id->

    pool_sz

    ; i++) {

    // index = pool_id->currentIndex + i; // --- old

    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

    ;

    }

    index++;

    // +++ new

    }

    if

    (inHandlerMode()) {

    portCLEAR_INTERRUPT_MASK_FROM_ISR(dummy);

    }

    else

    {

    vPortExitCritical();

    }

    return

    p;

    }

    maybe in

    osPoolFree

    currentIndex

    could be set to the last freed block, so

    alloc does not need to search for a free block.

    osStatus

    osPoolFree

    (

    osPoolId

    pool_id,

    void

    *block)

    {

    uint32_t

    index;

    if

    (pool_id == NULL) {

    return

    osErrorParameter

    ;

    }

    if

    (block == NULL) {

    return

    osErrorParameter

    ;

    }

    if

    (block < pool_id->

    pool

    ) {

    return

    osErrorParameter

    ;

    }

    index = (

    uint32_t

    )block - (

    uint32_t

    )(pool_id->

    pool

    );

    if

    (index % pool_id->

    item_sz

    ) {

    return

    osErrorParameter

    ;

    }

    index = index / pool_id->

    item_sz

    ;

    if

    (index >= pool_id->

    pool_sz

    ) {

    return

    osErrorParameter

    ;

    }

    pool_id->

    markers

    [index] = 0;

    pool_id->currentIndex=index; // +++ next

    alloc

    will get this free block

    return

    osOK

    ;

    }

    A better solution would be to use a linked list with free memory packets.

    This would avoid searching for a free packet on allocation.

    best regards