- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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;
}
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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 ?
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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 )
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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?
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
How do you know the way ble stack use ring buffer?
I don't find the source code for that part.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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.
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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
- Mark as New
- Bookmark
- Subscribe
- Mute
- Subscribe to RSS Feed
- Permalink
- Report Inappropriate Content
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