1 2 Previous Next 15 Replies Latest reply on Jun 1, 2017 1:32 PM by mifo

    WICED ring_buffer data corruption

    DaveStude

      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;

      }

        • 1. Re: WICED ring_buffer data corruption
          axel.lin_1746341

          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.

          1 of 1 people found this helpful
          • 2. Re: WICED ring_buffer data corruption
            xavier@candyhouse

            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 ?

            1 of 1 people found this helpful
            • 3. Re: WICED ring_buffer data corruption
              DaveStude

              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 )

              2 of 2 people found this helpful
              • 4. Re: WICED ring_buffer data corruption
                axel.lin_1746341

                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.

                • 5. Re: WICED ring_buffer data corruption
                  xavier@candyhouse

                  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?

                  • 6. Re: WICED ring_buffer data corruption
                    mifo

                    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.

                    • 7. Re: WICED ring_buffer data corruption
                      sqlsql_2244756

                      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

                      • 8. Re: WICED ring_buffer data corruption
                        DaveStude

                        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.

                        • 9. Re: WICED ring_buffer data corruption
                          sqlsql_2244756

                          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

                          • 10. Re: WICED ring_buffer data corruption
                            axel.lin_1746341

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

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

                            • 11. Re: WICED ring_buffer data corruption
                              DaveStude

                              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.

                              • 12. Re: WICED ring_buffer data corruption
                                sqlsql_2244756

                                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

                                • 13. Re: WICED ring_buffer data corruption
                                  axel.lin_1746341

                                  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.

                                  • 14. Re: WICED ring_buffer data corruption
                                    DaveStude

                                    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.

                                    1 2 Previous Next