Forum Rule: Always post complete source code & details to reproduce any issue!
Results 1 to 7 of 7

Thread: Possible race Condition on Teensy 3.2 Serial when using Transmit Enable pin

  1. #1

    Possible race Condition on Teensy 3.2 Serial when using Transmit Enable pin

    Hi,

    We are using a Dual Tristate buffer to pull the Teensy Serial Output to 5V and to simultaneously create a one Wire communication line (Nexperie 74HC2G125).
    This relies on the Tx Enable pin, much like the RS485 transceivers; the only difference is we stay on 0~5V levels with reference to GND.

    What we're seeing on random occasions is that TX Enable is brought low, before the last character is transmitted. I attach a picture from our Logic Analyser.
    Click image for larger version. 

Name:	Byte nao trabsmitido pelo Teensy.jpg 
Views:	2 
Size:	65.0 KB 
ID:	17090

    - Channel 2 is Teensy Serial2 TX pin (that goes to the tri state buffer)
    - Channel 3 is the Actual output after the Tri State buffer
    - Channel 4 is the pin configured as Tx Enable using Serial2.transmitterEnable(...)
    - Channel 5 can be ignored for the purpose of this post.

    We've seen this happen frequently at 1Mbps, always with the last character; at lower speed (57k) this has not happened at all.

    You can see the last character does not go through the Tri State buffer, which is consistent with the TX Enable being brought LOW too early, while the last byte is being transmitted by Teensy.
    This should not be happening; as I mentioned, TX Enable is being managed by the Teensyduino library (set via the transmitterEnable pin) and as such was expected it to be held high during transmission.


    After several hours of debugging and finally looking at the code in serial2.c we came across a "suspect" for a Race condition as explained:

    - The code relies on checking the following, in the ISR, to determine of it's time to turn off the TX Enable pin.
    Code:
    if ((c & UART_C2_TCIE) && (UART1_S1 & UART_S1_TC)) {
    UART_C2_TCIE interrupt is only enabled after we determined that we have no more characters to put on the TX buffer and thus we change UART1_C2 = C2_TX_COMPLETING (which enables TCIE)


    In the following s:
    - Assume that in last ISR call, we had nothing more to transmit and we enter TX_COMPLETING enabling the TCIE interrupt

    1- If we then call serial2_putchar(..), to add a new character for transmission, before the next ISR call,
    the serial2_putchar function will

    a) re-assert the TX Enable,
    b) check if our software buffer is full and if it is
    b.1) wait on UART1_S1 & UART_S1_TDRE to transmit data so that it can free up space in the buffer
    b.2) get the next character from the software buffer to be transmitted
    b.3) assign the character to UART1_D, to effectively transmit.
    c) save the new character we're passing as a parameter in the software buffer
    d) Finally, it will reset the UART1_C2 to UART1_C2 = C2_TX_ACTIVE, which effectively disables the TCIE interrupt and enables the TDRE interrupt instead.

    If we were in TX_COMPLETING state, with TCIE enabled, and the TC flag is raised while serial2_putchar is being executed, the TC flag appears to remain asserted in the S1 register, because we've done nothing clear it, despite the fact that we're adding more data to be sent.
    (See datasheet page 1214: depending on whether the software buffer is full we may or may not read UART1_S1 but even if we do, there is still time between reading the UART1_S1 and writing to D; this is not atomic and as such won't implicitly reset TC.)

    3- In the next ISR call, we have UART1_C2 = C2_TX_ACTIVE and it treats the TDRE flag. Because there are no other characters to transmit, it will change to UART1_C2 = C2_TX_COMPLETING, enabling TCIE again.


    Because the ISR does not Return on each case it checks, after setting UART1_C2 = C2_TX_COMPLETING it will fall through to the final check of
    Code:
    if ((c & UART_C2_TCIE) && (UART1_S1 & UART_S1_TC)) {
    The problem here is that we may have a leftover TC flag in UART1_S1 as expained above because S1 was not cleared when we added the new character.

    It appears this condition may be evaluated as true which will deassert TxEnable too early, causing the last character to be not be transmitted by the Tri State buffer.
    It is important to understand that TDRE firing means the buffer needs more data; however data may still be in the shift register. That's why we change to wait for TCIE after TDRE and wait for TCIE before disabling TXEnable.


    There appear to be 2 ways to go about this:
    - Either UART1_C2 needs to get set much earlier in serial2_putchar which disables the TCIE interrupt and prevents a the TC bit from being set. (maybe we also need to actively make sure we clear the TC flag anyway?)
    - Or we don't fall through on the ISR. We actively RETURN from the function after treating each case, but it's not clear if this would work; there would still be a leftover (wrong) TC flag; also I'm not sure if this is a good policy because I'm not familiar enough with the way ISRs work in this architecture and we may miss something.

    We're completely in the dark on this one, but even if this is not the cause of our problem, it is definitely worth looking into, because it does seem to be a potential race conditions.

    Your thoughts would be greatly appreciated.

    Thank you
    Last edited by pramilo; 07-30-2019 at 08:54 PM. Reason: added Nexperia chip model; spelling

  2. #2
    Upon thinking about this for a while, this is definetely a potential race condition between the TCIE interrupt firing and de-asserting TXEnable while we add a new character to be transmitted:

    Here is the code from serial2_putchar (similar in other Serial interfaces). Lines numbered for convenience.

    Code:
    1. void serial2_putchar(uint32_t c)
    2. {
    3. 	uint32_t head, n;
    4. 
    5. 	if (!(SIM_SCGC4 & SIM_SCGC4_UART1)) return;
    6. 	if (transmit_pin) transmit_assert();
    7. 
    8. 	head = tx_buffer_head;
    9. 	if (++head >= SERIAL2_TX_BUFFER_SIZE) head = 0;
    10. 	while (tx_buffer_tail == head) {
    11. 		int priority = nvic_execution_priority();
    12. 		if (priority <= IRQ_PRIORITY) {
    13. 			if ((UART1_S1 & UART_S1_TDRE)) {
    14. 				uint32_t tail = tx_buffer_tail;
    15. 				if (++tail >= SERIAL2_TX_BUFFER_SIZE) tail = 0;
    16. 				n = tx_buffer[tail];
    17. 				if (use9Bits) UART1_C3 = (UART1_C3 & ~0x40) | ((n & 0x100) >> 2);
    18. 				UART1_D = n;
    19. 				tx_buffer_tail = tail;
    20. 			}
    21. 		} else if (priority >= 256) {
    22. 			yield(); // wait
    23. 		}
    24. 	}
    25. 	tx_buffer[head] = c;
    26. 	transmitting = 1;
    27. 	tx_buffer_head = head;
    28. 	UART1_C2 = C2_TX_ACTIVE;
    29. }
    Assume we enter putchar when UART1_C2 = C2_TX_COMPLETING.

    On line 6 we assert the TXEnable line, but we haven't changed UART1_C2, so the TCIE interrupt could be set and could fire at any point in time between line 6 and line 28. If the TCIE event fires ,the ISR will de-assert the TXEnable line.

    When we get to line 28., where we change UART2_C1 to a mask that disables TCIE, but we don't re-assert TXEnable and hence there was plenty of time for a race condition to have de asserted TXEnable.
    TXEnable will only get reasserted when we send another chracter. Thus a character can be lost in transmission.

    I could not find a way to edit my original post as I believe the original explanation is a bit complex and not totally accurate in some aspects.

    Another thing I don't get is: why are we messing with the tx_buffer_tail in this function? If the buffer was full should't we wait on the ISR's to fire and do their job transmitting the characters instead of hard coding the transmission here?
    If the ISR fires amidst us handling tx_buffer_tail in this user function, the ISR may actually change the value of tx_buffer_tail but bc we've buffered tail in the function, we may be re transmitting the same character twice: another possible race condition but that's whole separate matter.

    I'm only interested in TXEnable for now.

    Thank you.
    Last edited by pramilo; 07-31-2019 at 05:42 PM.

  3. #3
    Here is sample code that will provoke the malfunction described, on a Teensy 3.2:

    Code:
    1. #define USB_SERIAL (&Serial)
    2. #define RS485_SERIAL (&Serial1)
    3. #define TTL_SERIAL (&Serial2)
    4. 
    5. void setup()
    6. {
    7. 	RS485_SERIAL->transmitterEnable(2);
    8. 	TTL_SERIAL->transmitterEnable(5);
    9. 
    10. 	RS485_SERIAL->begin(1000000);
    11. 	TTL_SERIAL->begin(1000000);
    12. }
    13. 
    14. void loop()
    15. {
    16. 	while (1) {
    17. 
    18. 		// send a burst of bytes
    19. 		for (byte i = 50; i < 60; i++) {
    20. 			TTL_SERIAL->write(i);
    21. 		}
    22. 
    23.		// add a small interval so that we get next byte in, when the UART is already finsishing
    24.		// the transmission of the block above (i.e. UART1_C2 is in state TX_COMPLETING)
    25.		// On a Teensy 3.2 at 96Mhz 75 at 1Mbps did it for me.
    26.		// this is not deterministic so you need to look at several samples in the logic analyser
    27.		// some will be OK, others will exhibit the behaviour described
    28. 		delayMicroseconds(75);
    29. 		TTL_SERIAL->write(60);
    30. 
    31. 		delay(10); // wait a bit to separate samples in the Logic analyser
    32. 	}
    33. }
    Using a Logic analyser connect Pin 10 (TX2) to one channel and connect pin 5 (which in the code is set as TX Enable) to another channel.
    Run the code. You may need to tweak the MicroSeconds as described in the code.


    You will see the following:
    (again, as I said, you need to tweak the interval, and not all samples will show this failure; look around and you'll find some assuming once you get the timing right; this is a race condition after all)
    Click image for larger version. 

Name:	Capturar.jpg 
Views:	1 
Size:	59.9 KB 
ID:	17097
    Channel 2 is TX2 output, Channel 4 is the TXEnable pin (pin 5 as set in the code)

    As you can see TXEnable is deasserted when the last character is being transmitted,

    If you increase the Microseconds delay, you get two separate assertions of the TXEnable line and if you lower it, it will give you the whole transmission in one single assertion of the line.
    The trick to reproduce the issue is tweaking so that the line gets deasserted before the last character is transmited.
    The why's and how's this happens are described in my previous posts.

    Here is my bugfix, which I have tested briefly and has been working for me. In serial2.c:

    Code:
    void serial2_write(const void *buf, unsigned int count)
    {
    	const uint8_t *p = (const uint8_t *)buf;
    	const uint8_t *end = p + count;
    	uint32_t head, n;
    
    	if (!(SIM_SCGC4 & SIM_SCGC4_UART1)) return;
    
    /***************** BUG FIX BEGINS */
    	// Bugfix: TXEnable Race Condition
    	// TCIE (TX COMPLETING) state active; change it so that we don't inadvertently de-assert TXEnable
    	// It's ok to set to INACTIVE bc we will enable it again at the end of the function
    	// Don't enable yet bc we haven't put the new character into the buffer
    	if (UART1_C2 & (C2_TX_COMPLETING)) UART1_C2 = C2_TX_INACTIVE;
    /******************** BUGFIX ENDS */
    
    	if (transmit_pin) transmit_assert();
    Code:
    void serial2_putchar(uint32_t c)
    {
    	uint32_t head, n;
    
    	if (!(SIM_SCGC4 & SIM_SCGC4_UART1)) return;
    
    /***************** BUG FIX BEGINS */
    	// Bugfix: TXEnable Race Condition
    	// TCIE (TX COMPLETING) state active; change it so that we don't inadvertently de-assert TXEnable
    	// It's ok to set to INACTIVE bc we will enable it again at the end of the function
    	// Don't enable yet bc we haven't put the new character into the buffer
    	if (UART1_C2 & (C2_TX_COMPLETING)) UART1_C2 = C2_TX_INACTIVE;
    /******************** BUGFIX ENDS */
    
    	if (transmit_pin) transmit_assert();
    I'd be nice to get some fix for this into the next release of Teensyduino. Note that to needs to be done for all Serials; in this example I'm only showing it for Serial2 but the code is fairly similar for others.

    I hope the lack of replies means the topic is too complex/not interesting as opposed to be found too ignorant...

    Best,
    Last edited by pramilo; 07-31-2019 at 06:48 PM. Reason: improved the code of the bugfix proposed (parenthesis in macro)

  4. #4
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    9,019
    just a glance at the code these lines are waiting for room to queue a char: and checks that the code executing might have higher IRQ_PRIRORITY (i.e. lower #) which assures that 'nobody' else will be sending chars to FIFO as long as in this loop waiting.

    So if this char is not to be lost - or the system deadlocked? - then this code lines #10-24 must feed the FIFO a char to make room to enqueue this char.

    Code:
    10. 	while (tx_buffer_tail == head) {
    11. 		int priority = nvic_execution_priority();
    12. 		if (priority <= IRQ_PRIORITY) {
    perhaps it is true that a race could exist where the FIFO sends the last char and deasserts xmit after line 6 - but before an empty queue gets one character inserted at line 25 - when lines 8,9,10 jump to 25. But I don't know the effect of lines 25,26,27,28 on the waking the push to FIFO code and what happens then.

  5. #5
    Your take on lines #10-24 makes sense. Thank you for clarifying. I'm not enterily familiar with the NVIC in Arm. (I come from Atmel)

    I'm still at a loss regarding the TxEnable pin. The bugfixes I proposed above don't seem to completely resolve the issue. I tried editing the post but the site says I'm not allowed to edit after 120mins of posting.

    This is really a major issue for us. I am able to show code that shows the issue happening but can't figure out a fix.
    Being that TxEnable is handled by Teensyduino, I suppose the solution involves some fixing in there.

  6. #6
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    5,034
    I can maybe see the condition you are mentioning... If you look at the write method, like you mentioned.
    Code:
    void serial2_write(const void *buf, unsigned int count)
    {
    	const uint8_t *p = (const uint8_t *)buf;
    	const uint8_t *end = p + count;
    	uint32_t head, n;
    
    	if (!(SIM_SCGC4 & SIM_SCGC4_UART1)) return;
    	if (transmit_pin) transmit_assert();
    	while (p < end) {
    		head = tx_buffer_head;
    		if (++head >= SERIAL2_TX_BUFFER_SIZE) head = 0;
    		if (tx_buffer_tail == head) {
    			if (transmit_pin) transmit_assert();
    			UART1_C2 = C2_TX_ACTIVE;
    			do {
    				int priority = nvic_execution_priority();
    				if (priority <= IRQ_PRIORITY) {
    					if ((UART1_S1 & UART_S1_TDRE)) {
    						uint32_t tail = tx_buffer_tail;
    						if (++tail >= SERIAL2_TX_BUFFER_SIZE) tail = 0;
    						n = tx_buffer[tail];
    						if (use9Bits) UART1_C3 = (UART1_C3 & ~0x40) | ((n & 0x100) >> 2);
    						UART1_D = n;
    						tx_buffer_tail = tail;
    					}
    				} else if (priority >= 256) {
    					yield();
    				}
    			} while (tx_buffer_tail == head);
    		}
    		tx_buffer[head] = *p++;
    		transmitting = 1;
    		tx_buffer_head = head;
    	}
    	if (transmit_pin) transmit_assert();
    	UART1_C2 = C2_TX_ACTIVE;
    }
    So what happens is you call this with a buffer (likewise for putchar), and the Green line is called, which Asserts the transmit enable pin, but RIGHT after this and before we put a new character on the queue, the ISR fires, and finds the queue empty, and deasserts the transmit, but we go ahead and add the new characters, and for these new characters, the transmit assert is not asserted...

    So there are a variety of fixes that can be done... I think duplicating the enable call in the two places in RED might take care of it.
    In fact not sure the Green one is needed.

    The rational, if we are calling this function and the writes are active, then transmit enable pin should be enabled. If we get into this function and we are not transmitting, then nothing will happen to call the ISR, until we set the start of UART1_C2, which will then try to go get something... First one is if we started off empty and then filled the queue, we need to make sure the TX is active in order for it to send out data. So in these cases we need to make sure that the TX enable is enabled...

  7. #7
    Hi Kurt,

    Good point spotting the place in Red. Hadn't nailed that one down.

    I've been RTFM'ing quite a bit about this; not only the Teensy processor datasheet but especially alternative sources.

    There's a nice Application note from ST about the Transmit Enable pin here https://www.st.com/content/ccc/resou...CD00249778.pdf
    It's a high level explanation so it applies to Kinetis as well.

    The implementation in Teensyduino, at the high level, is pretty much in line with the application note.

    However these corner cases are quite tricky to handle, probably in the context of the actual code implementation.
    I've actually moved my attention to some other tasks for a brief period now as I was getting nowhere with this.

    I really appreciate your input and feedback on this.

    Thank you,
    Pedro

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •