WICED ring_buffer data corruption

Tip / Sign in to post questions, reply, level up, and achieve exciting badges. Know more

cross mob
dast_1961951
Level 4
Level 4
10 likes received First like received

Working in WICED 4.1.0.43 and have noted that, when using a ring buffer (ring_buffer.c), every nth byte is corrupt where n = ring_buffer.size. 

Looks like a long-standing issue that's masked often because the corrupt byte is most often NULL and the buffers are typically used in fault-tolerant protocols.

Exposed as follows:

int cmd_rb_test(int argc, char argv[])

{

    static wiced_ring_buffer_t test_buf;

    unsigned char test_buf_data[999];

    uint8_t in;

    uint8_t out;

    int bytes_read;

    int i;

    memset(test_buf_data, 0, sizeof(test_buf_data));

    ring_buffer_init((wiced_ring_buffer_t*) &test_buf, (uint8_t*) test_buf_data, sizeof(test_buf_data));

    for (i = 0; i < (sizeof(test_buf_data) * 10); ++i)

    {

        in = i & 0xFF;

        out = 0;

        ring_buffer_write(&test_buf, &in, 1);

        ring_buffer_read(&test_buf, &out, 1, &bytes_read);

        if (out != (i & 0xFF))

        {

            printf("ring buffer failure at %d head= %d tail= %d!\n", (unsigned int) out, test_buf.head, test_buf.tail);

            return -1;

        }

    }

    printf("ring buffer passed\n");

    return 0;

}

15 Replies
AxLi_1746341
Level 7
Level 7
10 comments on KBA 5 comments on KBA First comment on KBA

DaveStude wrote:

Working in WICED 4.1.0.43 and have noted that, when using a ring buffer (ring_buffer.c), every nth byte is corrupt where n = ring_buffer.size. 

It's buggy in all sdk versions.

It's actually an important issue as all applications/libraries using ring_buffer can hit the bug.

Let me share my fix :

uint32_t ring_buffer_write( wiced_ring_buffer_t* ring_buffer, const uint8_t* data, uint32_t data_length )

{

#ifdef SDK_MOD_RING_BUFFER_FIX_WRITE

  uint32_t write_length;

  uint32_t tail_write_length_max;

  write_length = (ring_buffer->size + ring_buffer->head - 1 - ring_buffer->tail) % ring_buffer->size;

  if (write_length > data_length)

  {

  write_length = data_length;

  }

  tail_write_length_max = ring_buffer->size - ring_buffer->tail;

  if (ring_buffer->head <= ring_buffer->tail && tail_write_length_max < write_length)

  {

  memcpy(ring_buffer->buffer+ring_buffer->tail, data, tail_write_length_max);

  memcpy(ring_buffer->buffer, data+tail_write_length_max, write_length-tail_write_length_max);

  }

  else

  {

  memcpy(ring_buffer->buffer+ring_buffer->tail, data, write_length);

  }

  ring_buffer->tail = (ring_buffer->tail + write_length) % ring_buffer->size;

  return write_length;

#else

    uint32_t tail_to_end = ring_buffer->size-1 - ring_buffer->tail;

    /* Calculate the maximum amount we can copy */

    uint32_t amount_to_copy = MIN(data_length, (tail_to_end + ring_buffer->head) % ring_buffer->size);

    /* Copy as much as we can until we fall off the end of the buffer */

    memcpy(&ring_buffer->buffer[ring_buffer->tail], data, MIN(amount_to_copy, tail_to_end));

    /* Check if we have more to copy to the front of the buffer */

    if ( tail_to_end < amount_to_copy )

    {

        memcpy( &ring_buffer->buffer[ 0 ], data + tail_to_end, amount_to_copy - tail_to_end );

    }

    /* Update the tail */

    ring_buffer->tail = (ring_buffer->tail + amount_to_copy) % ring_buffer->size;

    return amount_to_copy;

#endif

}

Please, Cypress team, please help look at this issue (and my fix if possible).

DaveStude​, axel.lin_1746341​:

What do you think about this fix ?

here's what worked for me.  haven't tested it exhaustively:

-----diff---------------

@@ -97,10 +97,10 @@ wiced_result_t ring_buffer_deinit( wiced_ring_buffer_t* ring_buffer )

uint32_t ring_buffer_write( wiced_ring_buffer_t* ring_buffer, const uint8_t* data, uint32_t data_length )

{

-    uint32_t tail_to_end = ring_buffer->size-1 - ring_buffer->tail;

+    uint32_t tail_to_end = ring_buffer->size - ring_buffer->tail;

     /* Calculate the maximum amount we can copy */

-    uint32_t amount_to_copy = MIN(data_length, (tail_to_end + ring_buffer->head) % ring_buffer->size);

+    uint32_t amount_to_copy = MIN(data_length, ring_buffer_free_space(ring_buffer));^M

     /* Copy as much as we can until we fall off the end of the buffer */

     memcpy(&ring_buffer->buffer[ring_buffer->tail], data, MIN(amount_to_copy, tail_to_end));

@@ -161,8 +161,8 @@ wiced_result_t ring_buffer_read( wiced_ring_buffer_t* ring_buffer, uint8_t* data

uint32_t ring_buffer_free_space( wiced_ring_buffer_t* ring_buffer )

{

-    uint32_t tail_to_end = ring_buffer->size-1 - ring_buffer->tail;

-    return ((tail_to_end + ring_buffer->head) % ring_buffer->size);

+    uint32_t tail_to_end = ring_buffer->size - ring_buffer->tail;^M

+    return ((tail_to_end - 1 + ring_buffer->head) % ring_buffer->size);

}

uint32_t ring_buffer_used_space( wiced_ring_buffer_t* ring_buffer )

DaveStude wrote:

here's what worked for me.  haven't tested it exhaustively:

The fix is wrong.

The change in ring_buffer_free_space is pointless.

0 Likes

Hi axel.lin_1746341
I believe DaveStude​'s fix and mine are basically the same.

(Considering how they are different from original, I think his version is better)

These fixes does pass my tests (of course not exhaustive either).

My understanding about those changes in ring_buffer_free_space is for consistency of variable names.

The same variable name "tail_to_end" in closely related APIs should better have exactly the same meaning.

I personally appreciate those changes.

Hi mifo

My understanding is that mifo​ or any other Cypress staff​'s "Like" to replies DOES NOT MEAN APPROVAL OR ANYTHING.

It is just a like and no more than a like. Is this correct?

0 Likes

Likes are essentially a means to extend our appreciation to users that take the time to try to help others within the community.

They do not indicate that Cypress has validated and tested the user's response internally.

General Preferences:

Likes are essentially a means to extend our appreciation to users that take the time to try to help others within the community.

They do not indicate that Cypress has validated and tested the user's response internally.

The behaviour of your "Like" is misleading.

Think about your position, as a employee of cypress,

It's easy to make people think your "like" is a confirm that the reply is correct.

Moreover, people may think your team will take care of the fix.

I would suggest you don't press "Like" this way.

Unless your really read the code and think it's useful, don't blindly press like.

mifo​ Any comment on Cypress' ability to fully test this fix?

axel.lin_1746341​ Advice to you:  I think we should stay on-topic.   Generally, we need to remain respectful of each other and Cypress employees.  It's important to remain focused on the subject matter & issues and harsh words & tangential discussions don't work in our favor of working toward an improved WICED platform.  Please think before you post.

Adding the level 1 applications team.

grsrlsrirashmadygsnsankh

Yes xavier@candyhouse​ my attempt was to minimize difference & get a working buffer.  I got lucky that "tail_to_end" calculation was clearly not right so the fix started there.

I found the issue using the buffer as a way to exchange a very large rolling set of data (no integrity checks) between two threads. 

Indeed the ring buffer primarily supports BLE stack.  In addition, I've used command_console for a while and noted sporadic incorrect character output and missing characters in printfs.  Let's see if it fixes this stuff too.

0 Likes

I am not sure, that wiced ring buffer is thread safe.

In ble stack not used read write ring buffer functions.

Her used direct dma write into ring buffer.

When it filled with uart data begun from start.

But her I see possible thread safe problems.

Darius

0 Likes

How do you know the way ble stack use ring buffer?

I don't find the source code for that part.

0 Likes

axel.lin_1746341  Have to add some stub/debug code to ring_buffer.c to determine which thread is calling and then determine buffer size.

0 Likes

Ble stack using uart functions.

Uart functions using ring buffer.

I used sam4s implementation.

For test I enable breakpoints on  ring buffer functions and program stop from ble read_thread.

BR

Darius

0 Likes
DaBa_2244756
Level 5
Level 5
25 likes received 10 likes received 10 likes given

Hi DaveStude,

Thanks for your report.

Bad news:

ring buffer used in ble stack in read_thread.

Her it in size 3000. So after each 3000 read/write we have corrupted 1byte data in BLE communication?

Its very critical error.

BR.

Darius

0 Likes