losing characters in hal_usart_os function usart_os_read

jhenges wrote on Monday, July 15, 2019:

There seems to be a situation where the hal_usart_os code will drop the first characters of a transmission. It is not because of speed, but of order of execution.

If you perform something like (after proper initialization, etc):

	io_read(&USART_0.io,&buf1,len1) (call A)

and wait for characters. Everthing will probably be fine.

But supposing you do the following next:

	io_read(&USART_0.io,&buf2,len2) (call B)

and in between those two calls you performed a function which caused the characters to come into USART_0, what you will find is that the first character(s) of the transmission actually go into the previous buffer, buf1 rather than buf2.

What is supposed to happen is that the internal ring buffer is supposed to get the characters from the interrupt service routine (which it does). It is then supposed to empty the ring buffer into the desired buffer.

What actually happens is that the previous call (call A) does an initialzation on the ring buffer structure which causes the future call (call B) to incorrectly load those character(s) saved in the ring buffer to the wrong place (buf1 rather than buf2).

This would not be a problem if the destination buffer did not change between calls.

In the usart_os_read function, the middle of the code looks like this:

if (ringbuffer_num(&descr->rx) < length) {	// ***** (a)
	CRITICAL_SECTION_ENTER()
	descr->rx_size   = 0;
	descr->rx_length = length;
	descr->rx_buffer = buf;

	while (ringbuffer_num(&descr->rx) > 0) {
		ringbuffer_get(&descr->rx, &descr->rx_buffer[descr->rx_size++]);
	}

	CRITICAL_SECTION_LEAVE()

	if (sem_down(&descr->rx_sem, timeout) != 0) {	// ***** (b)
		return ERR_TIMEOUT;
	}
} else {
	while (was_read < length) {
		ringbuffer_get(&descr->rx, &buf[was_read++]);
	}
}

In the two places marked by ****, the following changes should be made.
In the (a) case, the ringbuffer could have just 1 character in in which requires attention. As such, the original code would be off by 1. The test should be <= rather than just <.

In the (b) case, the (descr->rx_buffer = NULL) state is used as a flag to initialize some transfer activities in the overall code flow. (look at the interrupt callback, usart_os_fill_rx_buffer) In this example, this pointer is set to the previous buffer. So when the call come in to start a new read, the ring buffer gets emptied into the wrong target buffer because the (descr->rx_buffer) has not been set to NULL. The reason for adding this initialization code at (b) is for the case when you try to flush the buffer before going onto a following serial transaction.

if (ringbuffer_num(&descr->rx) <= length) {		// ***** (a)
	CRITICAL_SECTION_ENTER()
	descr->rx_size   = 0;
	descr->rx_length = length;
	descr->rx_buffer = buf;

	while (ringbuffer_num(&descr->rx) > 0) {
		ringbuffer_get(&descr->rx, &descr->rx_buffer[descr->rx_size++]);
	}

	CRITICAL_SECTION_LEAVE()

	if (sem_down(&descr->rx_sem, timeout) != 0) {
		descr->rx_buffer = NULL;					// ***** (b)
		descr->rx_size = 0;							// ***** (b)
		descr->rx_length = 0;						// ***** (b)
		return ERR_TIMEOUT;
	}
} else {
	while (was_read < length) {
		ringbuffer_get(&descr->rx, &buf[was_read++]);
	}
}

I have attached an updated version of hal_usart_os.c call ed hal_usart_lost_character_fix.c

richard_damon wrote on Monday, July 15, 2019:

One important thing to point out, FreeRTOS doesn’t have any of these functions in its base distribution, but these look more like a vendor layer around FreeRTOS (and likely designed to use other OSes also)

Such code doesn’t belong in the FreeRTOS kernel, as it is very device specific.

jhenges wrote on Monday, July 15, 2019:

Richard,
Thanks for the note. I am new to this. After your comment, I realized that this IS actually at the port level for the ATSAMD51 Cortex M4 processor.

John Hengesbach