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

[pramilo]

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.


- 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.

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

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

[pramilo]

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.

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.

[pramilo]

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

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)

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:

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();

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,

[defragster]

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.

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.

[pramilo]

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.

[KurtE]

I can maybe see the condition you are mentioning... If you look at the write method, like you mentioned.

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...

[pramilo]

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

[pramilo]

Quick update: Haven't had much luck getting this to work.

It appears someone else has reported similar issues before https://forum.pjrc.com/threads/56281...tween-2-teensy

We're now looking at alternative processors, namely the ATSAMD51 and the new STM32G series both of which handle the TX Enable pin in hardware. You actually configure the TX Enable pin in the registers (including a guard time) and it's the processor that handles the Enable signal.

It's a pity that this hasn't received much attention from the manufacturer, considering we posted code to clearly reproduce the issue.

[KurtE]

@pramilo - Did you try out the changes I mentioned? Did it work?

I never saw any clear answer on if this worked for you or not? If so, then one of us could create a Pull Request for the changes and get them into the next release.

As for not receiving much attention from manufacturer, Which one? If Teensy? @PaulStoffregen - is the one and only developer, the rest comes from members like ourselves who try to fix as many things as possible. Obviously most of us will mostly fix the things that impact our own use and/or grabs our interest.

Again if if the above mentioned changes fixes your issue, I will probably be inclined to put together a PR as I do use this functionality, although I have never run into this issue.

Kurt

[pramilo]

Hi Kurt

I tried those changes and I also tried some other, more aggressive ways to make the operation atomic.

None of them resolved the issue.

Hence my post from earlier today.

Coincidentally I found a post which I actually wrote myself, back in 2016, and apparently, there seems to be a way to get Hardware support for enabling/disabling RS485. Not as flexible as you get on the ATSAMD51 but good nonetheless: https://forum.pjrc.com/threads/36314...sor-handles-it

The trick resides in configuring RTS with the TXRTSE flag (as opposed to the usual RXRTSE). This makes the RTS pin behave as a TX Enable, thus dispensing the need to do this in software (with all the issues encountered).
The only thing is that it only waits 1 bit time before beginning to transmit and 1 bit time afterwards and this seems to be fixed (and obviously you're also limited on which pins you can use)

I am currently investigating this route.

As for the a software fix in the library, as mentioned, I tried yours and several other more aggressive approaches without success.

Thanks

[KurtE]

@paramilo - Again it is hard to know what to suggest again as I don't really know what all you tested and what did not work.

For example in the modifications I suggested, only fixed certain cases of this, in particular when you call something like:
Serial2.write(buf, cnt);

It does NOT fix the case if you call Serial2.write(60);

That is because Serial2.write(60) does not call serial2_write, but instead calls serial2_putchar, so you would need to put in the same level of changes there.

I put in some instrumentation into a local copy of Serial2, and I believe it took care of those issues.

So for example with your test program, try, changing serial2_putchar function to be:

void serial2_putchar(uint32_t c)
{
	uint32_t head, n;

	if (!(SIM_SCGC4 & SIM_SCGC4_UART1)) return;
	head = tx_buffer_head;
	if (++head >= SERIAL2_TX_BUFFER_SIZE) head = 0;
	while (tx_buffer_tail == head) {
		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(); // wait
		}
	}
	tx_buffer[head] = c;
	transmitting = 1;
	tx_buffer_head = head;
	if (transmit_pin) transmit_assert();
	UART1_C2 = C2_TX_ACTIVE;
}

Again I moved the transmit_assert to be just before we setup to do output...

Edit: Looks like I still get a few. I have gotten about 4 in maybe an hour...

Edit2: I disabled interrupts around the trnasmit_assert and UART1_C2 setting like:

	__disable_irq();

	if (transmit_pin) transmit_assert();
	debug_in_serial2_write = 1;		// BUGBUG DEBUG
	UART1_C2 = C2_TX_ACTIVE;
	__enable_irq();
	debug_in_serial2_write = 0;		// BUGBUG DEBUG

Disregard the debug_in ... I am using that in test app, where I put an ISR on the falling edge of the Enable pin to then check to see if I am in that window...

#define TTL_SERIAL (&Serial2)
uint32_t error_count = 0;
uint32_t error_count_prev = 0;
void setup()
{
  Serial.begin(115200);
  while (!Serial && millis() < 4000) ;
  Serial.println("Start test");
  Serial.flush();
  delay(3000);
  pinMode(4, OUTPUT);
  digitalWrite(4, LOW);
  pinMode(13, OUTPUT);
  Serial2.begin(1000000);
  Serial2.transmitterEnable(5);
  attachInterrupt(5, &tepin_interrupt, FALLING);
}
extern "C" {
  extern volatile uint8_t debug_in_serial2_write;
}

void tepin_interrupt() {
  if (debug_in_serial2_write) {
    digitalWriteFast(4, !digitalRead(4));
    error_count++;
  }
  digitalWrite(13, !digitalRead(13));
}

void loop()
{
  while (1) {

    for (uint16_t delay_us = 60; delay_us < 90; delay_us++) {
      // send a burst of bytes
      for (byte i = 50; i < 60; i++) {
        Serial2.write(i);
      }

      // add a small interval so that we get next byte in, when the UART is already finsishing
      // the transmission of the block above (i.e. UART1_C2 is in state TX_COMPLETING)
      // On a Teensy 3.2 at 96Mhz 75 at 1Mbps did it for me.
      // this is not deterministic so you need to look at several samples in the logic analyser
      // some will be OK, others will exhibit the behaviour described
      delayMicroseconds(delay_us);
      Serial2.write(60);
      if (error_count != error_count_prev) {
        Serial.printf("Error %d: delay:%d\n", error_count, delay_us);
        error_count_prev = error_count;
      }

      delay(2); // wait a bit to separate samples in the Logic analyser
    }
  }
}

It helped locate others before I made the chagnes

[pramilo]

Tomorrow I will be conducting tests using both the software TransmitEnable and the hardware implemented TransmitEnable that I mentioned, and compare results using a Logic Analyser

I will also revisit the code bugfixes I tried before, while attempting to get the software implementation of Tx Enabe working and hope to post back all that data

[KurtE]

Another follow on. If that does not work sufficiently than another option might be to have the ISR assert the transmit..

Note Two cases in the ISR dependong on if has FIFO or not...
But Serial2 has FIFO on these chips so:

That is in the section of code:

	if ((c & UART_C2_TIE) && (UART1_S1 & UART_S1_TDRE)) {
		head = tx_buffer_head;
		tail = tx_buffer_tail;
		if ((head != tail) && transmit_pin) transmit_assert();
		do {
			if (tail == head) break;
			if (++tail >= SERIAL2_TX_BUFFER_SIZE) tail = 0;
			avail = UART1_S1;
			n = tx_buffer[tail];
			if (use9Bits) UART1_C3 = (UART1_C3 & ~0x40) | ((n & 0x100) >> 2);
			UART1_D = n;
		} while (UART1_TCFIFO < 8);
		tx_buffer_tail = tail;
		if (UART1_S1 & UART_S1_TDRE) UART1_C2 = C2_TX_COMPLETING;
	}

What happens if we add the line in RED? Alternatively could remove the head != tail check and move the code in the do { part of the code after the test tail == head...

Again I have not tried it, but that would put all of the control under the ISR...

[pramilo]

Hi Kurt,

What I found so far is that the problem does not seem to be solved by simply hacking serial_putchar or serial_write.

What I've tried to far was far more comprehensive, because I suspect the issue occurs if you add a character to the Queue while the TCIE interrupt is enabled, or at a specific time while the TIE interrupt is enabled but the character is not yet in the queue.
This causes the program to follow a strict flowchart that leads to de-asserting tx enable.

I have quickly put together a flowchart showing my understating of how the code works. (I used some trial software, so bear with me regarding all the watermark in the pic)



Even if you move around the transmit_assert in the serial putchar and write functions, if UART_C2 is in - or transitions to - C2_TX_COMPLETING, due to interrupts firing, you're always doomed.

1) You need an additional flag that indicates if we're in the process of "adding to the queue" and acts as guard for the code block wjile you add the data to the software buffer.

2) That flag needs to be checked in the handlers of:
a) TIE interrupt (enabled when C2_TX_ACTIVE). If we are in the process of "adding to the queue" it should NOT transition to C2_TX_COMPLETING and instead to C2_TX_INACTIVE
b) and TCIE interrupt as well. (enabled when C2_TX_COMPLETING). If we are in the process of "adding to the queue" it should NOT transmit_deassert but still move to C2_TX_INACTIVE

I will try to piece together how far I've gotten and I'll post back.

I hope the above clearly shows where the issue is.

This obviously not an issue limited to Teensy. There's an Application Note for the STM32F series which essentially advises users to follow the same implementation although it has the problems we uncovered.

[KurtE]

Did you try the solution I mentioned:

__disable_irq();
	if (transmit_pin) transmit_assert();
	UART1_C2 = C2_TX_ACTIVE;
	__enable_irq();

I think it should work, but could be wrong:

In all of the cases you are covering the assumption is when you entered this portion of code the TXFIFO is empty and you are just completing the output of your last character...

a) If it completes before the disable_IRQ, then the ISR code will at that point will transmit_deassert(), and then return, we then transmit_assert() and start up for the ISR to fire again which will output the new character(s)...

b) the list bit transfers out just after we disabled interrupts... This will hold off processing of the ISR. We go ahead and again call transmit_assert which does nothing as already asserted... We then set: UART1_C2 = C2_TX_ACTIVE;
And then we enable interrupts again.

So the ISR triggers - It should hit the condition:

	c = UART1_C2;
	if ((c & UART_C2_TIE) && (UART1_S1 & UART_S1_TDRE)) {

That says that the transmitter is not full, so we then grab the new character from the queue and stuff it out on UART1_D
It may (in your case will) find that it does not fill the FIFO again, so it will probably hit this case: if (UART1_S1 & UART_S1_TDRE) UART1_C2 = C2_TX_COMPLETING;

SO now we get down to the code to deassert...

	if ((c & UART_C2_TCIE) && (UART1_S1 & UART_S1_TC)) {
		transmitting = 0;
		if (transmit_pin) transmit_deassert();
		UART1_C2 = C2_TX_INACTIVE;
	}

I don't think this code will do the transmit_deassert for two reasons.
First this test: (c & UART_C2_TCIE) - Remember the code in the area that we disabled interrupts set C2 to:#define C2_TX_ACTIVE C2_ENABLE | UART_C2_TIE
So TCIE is not defined here. Yes the TX code enables it again with:#define C2_TX_COMPLETING C2_ENABLE | UART_C2_TCIE
But we are comparing against variable we read before we updated it again.

Second reason: (UART1_S1 & UART_S1_TC) will return false as I believe this status bit gets cleared when you put something out: UART1_D = n;

But again I could be completely wrong. But again if you have not tested out the above simple fix, you might try it.

Kurt

[pramilo]

Kurt, thanks for the proposed solutions.

I am taking a step by step incremental approach to go through everything I tried and everything you suggested.

I have now been able to confirm that TX is de-asserted prematurely by means of the TCIE interrupt, firing at an undesired time.

I modified Serial2.c to toggle a pin everytime it hits the TCIE routine.

	if ((c & UART_C2_TCIE) && (UART1_S1 & UART_S1_TC)) {  // TX complete
		transmitting = 0;
		if (transmit_pin) transmit_deassert();
		
		if (*debug_pin == 0) { *debug_pin = 1; } else { *debug_pin = 0; }  // Added PedroR
		
		UART1_C2 = C2_TX_INACTIVE;
	}

With this set up, I ran the Logic analiser and got 2 different (interesting) samples:

Channel 0 = TX on Teensy Pin
Channel 1 = TX on the output chip (ie the chip that relies on Tx Enable)
Channel 2 = Tx Enable pin
Channel 3 = unused for this test
Channel 4 = my Debug pin (toggles every time we hit the TCIE interrupt)



Case 1: the Debug pin confirms TCIE interrupt fired and we can see the TxEn was prematurely de asserted resulting in part of the character being transmitted.



Case 2: the Debug pin confirms TCIE fired and de-asserted TxEnable; We can also see Tx Enable it was not re-asserted while transmitting that last character.
Nevertheless (and ironically), the debug pin shows the TCIE interrupt fires again at the end of transmitting that last character (which was not really sent out bc TxEnable was de asserted)


I will now move on to using PIN 22 with the TXRTSE flag to make it work as a "Hardware TX Enable". This should let me compare the behaviour of the software implementation vs the Hardware implementation.

Next I will move on to testing software fixes and post back.

Please note that I will not try any solution involving "disable_irq" because that could lead to opening a can of worms with other functions (USB, etc); I'd rather have a fix without messing with IRQ or otherwise move to using the Hardware Tx Enable (if that works... that's what I'm going to test now).

[pramilo]

NEXT TEST RESULTS:

Test consisted in the same approach as before, but now we add the TxEnable pin via Hardware, by means on the TXRTSE flag.

I added the following code after Serial2.begin(..), to enable TxEn via hardware. For Serial 2 this is performed by the processor on Pin 22.

// configure pin 22 for TX EN
	CORE_PIN22_CONFIG = PORT_PCR_MUX(3);
	UART1_MODEM |= (UART_MODEM_TXRTSE | UART_MODEM_TXRTSPOL);

Therefore, now on the Logic analyser we have:

Channel 0 = TX on Teensy Pin
Channel 1 = TX on the output chip (ie the chip that relies on Tx Enable)
Channel 2 = Tx Enable pin in SOFTWARE (toggled by the code in Serial2.c)
Channel 3 = TX Enable pin managed in HARDWARE (pin 22, via the TXRTSE flag)
Channel 4 = my Debug pin (toggles every time we hit the TCIE interrupt)



You see a lost character as before because it is still the Software implementation of TX Enable that's driving the Enable on the chip (the other signal, we're only observing in the Analyser at this point)

However the interesting bit here is comparing Channel 2 to Channel 3.

The toggle on Channel 3 suggest that the timing was such that the Tx buffer was emptied and the hardware de asserted the TX Enable. ON the software side, it likely entered the TCIE sequence of interrupts and the TCIE de asserted Tx enable as we can confirm in Channel 4.
However the HW managed TxEn managed to recover and re assert the Tx Enable signal whereas the software couldn't.


Therefore I am very convinced this is caused by interaction between the interruptions and the time at which we add a character. I think this and the previous post provide sufficient proof.

I will now move on to test a software fix and will post back.

[pramilo]

Interim update:

There seem to be 2 separate issues, which I coincidentally documented above. (my post #16 https://forum.pjrc.com/threads/57067...l=1#post215396)

Case 2 described there, I think I have been able to overcome with code changes (code guards)

Case 1 though, is what I am still getting, and getting quite frequently.
The interesting thing about case 1 is that the TCIE interrupt fires once, while transmitting the last character, causing the character to be truncated. Furthermore TCIE won't fire again, when the character finishes transmitting, which is quite odd.

It appears the character escapes the state machine and is placed in the shift register but doesn't trigger any of the interrupt sequences of a typical end of TX (TIE and TCIE sequence).

Looking into that right now and will post back either results or code progress so far, shortly.

[KurtE]

Again Personally I would try the simple disable interrupt one I mentioned to see if it solves the issue.

__disable_irq();
if (transmit_pin) transmit_assert();
UART1_C2 = C2_TX_ACTIVE;
__enable_irq();

As all you are doing is disabling interrupts for a simple If statement and two asignments:
As transmit_assert is defined as: #define transmit_assert() *transmit_pin = 1

And in fact the ISR function disables interrupts already... We could also potentially get rid of the if statement, and in the non-transmit pin case assign the variable to some simple memory variable that we write to ... But probably not necessary.

[pramilo]

Hi Kurt

There is an upside to calling transmit_Assert early on, which is to give the chip enough time to switch to transmission mode.
Modern chips are almost instant but still...

I will give up on this for today (and likely for good) but I am posting as far as I've gone. If anyone wants to pick up on this, they will be most welcome as we'd rather get this fixed in software as well.

I appreciate your suggestion for the disable_irq but I won't try it.

And in fact the ISR function disables interrupts already...

AFAIK, when an ISR is running, it doesn't disable interrupts; the NVIC can still steal the attention to higher priority interrupt (and Paul has guards in the code for that, to avoid deadlocking the UART).

For us, we will switch to using the hardware Tx Enable feature, on pin 22, via the TXRTSE which solves the issue.

It'd be good to get support for that in the Teensduino library (ie. choose Hardware TxEn or Software), but it's just 2 lines of code to enable it after Serialx.begin, so it's fine. I might make a separate post with instructions detailing how t enable the Hw Tx En on all 3 Serials.

This is as far as I've gone:

Channel 0 = TX on Teensy Pin
Channel 1 = TX on the output chip (ie the chip that relies on Tx Enable)
Channel 2 = Tx Enable pin in SOFTWARE (toggled by the code in Serial2.c)
Channel 3 = TX Enable pin managed in HARDWARE (pin 22, via the TXRTSE flag)
Channel 4 = my Debug pin (toggles every time we hit the TCIE interrupt)

The case below is fixed as you can see. TCIE fires but manages to recover by re asserting TX Enable in software. It then fires again after the last character and de-asserts TXEnable correctly.
This is good.


The case that I'm getting now and that I can't seem to be able to fix is the one below:



You can see TCIE fires, de asserts TXEn and the character gets truncated.
The odd part is that TCIE does not fire again when the character finishes transmitting (like it did originally, where it fired but TxEn weas de asserted).
This suggests the transmission is put in some state where the character in the shift register is no longer being tracked.
The other interesting clue is that the Hardware managed TXEn signal doesn't de-assert; maybe it merges with the "1 bit time" it respects after ending a character transmission but it may be a clue nonetheless.

It may be caused by my changes in code but it could also be a separate issue in itself. This is because we've seen the same thing it in my post #16 above, at the time using the stock serial2.c with minimal code changes (only the debug pin).


Attached is the serial2.c file with all the changes I made. This is for Teensyduino 1.45.

It may be worth running a Diff against the original Serial2.c as changes are scattered around the file. I made an effort to comment all my code changes with "PedroR".


Best Regards
Pedro.... tapping out.

[KurtE]

Ok, here is maybe another option. That might work... At least the limited things time I looked at the output on LA... did not put in test checks with ISR...

In the serial2.c file, up in the global defines I added a new variable:

static volatile BUFTYPE tx_buffer[SERIAL2_TX_BUFFER_SIZE];
static volatile BUFTYPE rx_buffer[SERIAL2_RX_BUFFER_SIZE];
static volatile uint8_t transmitting = 0;
static volatile uint8_t in_write_or_putchar = 0;	// flag to know we are in process of adding stuff to tx queue

In Serial2_putchar I setup this variable...

void serial2_putchar(uint32_t c)
{
	uint32_t head, n;

	if (!(SIM_SCGC4 & SIM_SCGC4_UART1)) return;
	in_write_or_putchar = 1;	// we are going to add stuff to TX queue
	if (transmit_pin) transmit_assert();
	head = tx_buffer_head;
	if (++head >= SERIAL2_TX_BUFFER_SIZE) head = 0;
	while (tx_buffer_tail == head) {
		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(); // wait
		}
	}
	tx_buffer[head] = c;
	transmitting = 1;
	tx_buffer_head = head;
	UART1_C2 = C2_TX_ACTIVE;
	in_write_or_putchar = 0;	// we are done...
}

likewise in the write function. (I also added test if count==0 and bailed early.

Now in the ISR I slightly modified the cod to deassert.

if ((c & UART_C2_TCIE) && (UART1_S1 & UART_S1_TC)) {
		transmitting = 0;
		if (transmit_pin && (in_write_or_putchar == 0)) transmit_deassert();
		UART1_C2 = C2_TX_INACTIVE;
	}

Idea is, we setup flag when we enter putchar or write, before we turn on the transmit_assert and only clear after we are finished adding stuff.
And then if we get the TCIE and we are still in code adding stuff to queue, don't turn off the transmit assert....

[pramilo]

Hi Kurt,

Using the global variable is the approach I took.

It's mostly as you wrote it, plus some corner cases that need dealing with.

The main issue though is in the ISR. In your code, when you block de-assertion but don't block UART_C2 from going into TX_INACTIVE you may end you with a stale character in the TX buffer.
The other thing is in the TIE ISR also needs some handling code added there, in my opinion.


Anyway, I guess it's best to publish the diff of the files myself. Please see the attached PDF file which has the Diff all color coded so you can see where things were changed.

I could not find a way to transfer the color coding to the forum automatically, so here's a screenshot and the actual PDF file is attached.



[EDIT] It appears the Forum reduces the resolution of uploaded images; therefore, please see the PDF attached if image is unreadable[/EDIT]

In short, despite all these changes, there's still the issue that I listed in my previous post, with all its weirdness.

Best,
Pedro.

[KurtE]

Glad you got it working hardware wise...

Note: I actually thought some about the stale condition you mentioned, but actually more worried the other way around.

That is in the output function:

UART1_C2 = C2_TX_ACTIVE;
	in_write_or_putchar = 0;	// we are done...

The thought was, if the ISR come through TCIE before we did the assignment of UART1_C2 = C2_TX_ACTIVE
Then when we do the assignment it should set us to go back and get that character... If it should trigger after this assignment the ISR should have the bits turned on for TIE and grab the data and put it out on the D register and cancel out the TCIE processing... But again could be wrong...

The actual other case I wondered about is having it leave the TX pin stuck on.

That is, suppose, we get to the assignment: UART1_C2 = C2_TX_ACTIVE;
Which starts up the transfer, and before we can do the next assignment in_write_or_putchar = 0;
Another high priority interrupt comes along that takes some time, it returns, the TCIE ISR is called, who sees the flag is still set does not deassert and returns, we then merrily exit leaving TX asserted...

The real question is, is their a minimal update that fixes up the vast majority of cases like this, that is easy enough to merge in. With T3.x I would need to make the change in something like 7 places, in T4 probably one. All one class so just need to change class code.

Again I still wonder if you can reproduce it with disabling instructions for two assignments?

void serial2_putchar(uint32_t c)
{
	uint32_t head, n;

	if (!(SIM_SCGC4 & SIM_SCGC4_UART1)) return;
	if (transmit_pin) transmit_assert();
	head = tx_buffer_head;
	if (++head >= SERIAL2_TX_BUFFER_SIZE) head = 0;
	while (tx_buffer_tail == head) {
		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(); // wait
		}
	}
	tx_buffer[head] = c;
	transmitting = 1;
	tx_buffer_head = head;

	__disable_irq();
	if (transmit_pin) transmit_assert();
	UART1_C2 = C2_TX_ACTIVE;
	__enable_irq();
}

I left in the transmit assert at the start that hopefully should the vast majority of the time take care of giving you maximum delay before tx starts.
The time that we disable interrupts is REAL small so not much of an issue. Could make it smaller time by

	if (transmit_pin) {
		__disable_irq();
		transmit_assert();
		UART1_C2 = C2_TX_ACTIVE;
		__enable_irq();}
	} else {
		UART1_C2 = C2_TX_ACTIVE;
	}

removes the if out of time that is disabled and only disables on those serial ports that you setup transmit...

But again does this approach reasonably solve your issue, or should we just punt for now?
Kurt

[pramilo]

Attachment 17570

I'm not sure if the above case can be resolved.
In the image:

Channel 0=Output of TX on teensy
Channel 1=Output of the buffer (trucanted)
Channel 2=TX Enable from the teensyduino library
Channel 3=TX Enable in hardware
Channel 4= toggles when we hit the TIE condition (UART1_C2 = C2_TX_COMPLETING
Channel 5= toggles every time we write to the UART_D register (enter a new character to be sent)

What the image is telling me (from my understating) is that a character is put in D (see signal in Channel 5), but the FIFO still won't get full so it transitions to UART1_C2 = C2_TX_COMPLETING

At this point the TC flag is likely asserted because the character is not yet the shift register. There seems to be a delay between adding the character to the FIFO and it actually starting to shift out.

Therefore during this delay and while the character is transitioning to the shift register, TC will remain asserted and it appears executes the de assertion of the TxEn line because all conditions are indeed met (TC is asserted). See Channel 4 transition to TX COMPLETING and later on Channel 2 TXEn is de asserted.

It seems to follow a correct sequence of events.

It appears that by the time the character starts being transmitted by the shift register, the handler for TC (which de asserts the TX Enable) had already fired and ran.

This is a corner case which I'm not sure how to correct; the ideal would be to have some form of guard time/timeout between UART1_C2 = C2_TX_COMPLETING and the assertion of TC.
However this would not be practical.

I have tried adding a verification of UART1_TCFIFO != 0 and also (UART1_SFIFO & UART_SFIFO_TXEMPT) == 0 ) before de asserting TX_En line, but it doesn't help.
My theory is that the processor seems to go into an undefined state where the character is transitioning from the FIFO to the shift register; during that time UART1_TCFIFO is 0, and TC is asserted (therefore nothing is tracking the character), so it de-asserts TX Enable as expected.
If this is the case there's nothing else that can be done about this.

Therefore, going back to my earlier thought, I'm not sure if this particular case can be resolved.


----

Nevertheless, the code changes I made do take care of a lot of situations where a full character was being shifted out with TxEn de asserted. Those cases are solved by the code fixes, which should VASTLY improve the issue at hand already.
We'll keep these fixes in our back pocket just in case, even though we're transitioning to Hw managed TXEn.

I have reduced the code changes in serial2.c to the absolute minimum I was able to.
I attach the final serial2.c file and a PDF with the DIFF.



Kurt, I'm not sure how/if you're affiliated to PJRC and I don't mean to be rude in what I'm about to say: If you're interested in submitting this for a Pull Request, Serial2.c should be mostly ready; this needs porting to the other serials though. I leave that to you/someone else. I think I've already gone way beyond what could be asked from a paying customer.
I've even gone ahead and made a post about how to use HW TxEn on all 3 serials of a T3.2.

Kurt, I appreciate your help and feedback. You've replied to not only this post but others I placed and I'm grateful for that.

Best Regards
Pedro

[KurtE]

Attachment 17570
...
Kurt, I'm not sure how/if you're affiliated to PJRC and I don't mean to be rude in what I'm about to say: If you're interested in submitting this for a Pull Request, Serial2.c should be mostly ready; this needs porting to the other serials though. I leave that to you/someone else. I think I've already gone way beyond what could be asked from a paying customer.
I've even gone ahead and made a post about how to use HW TxEn on all 3 serials of a T3.2.

I too am just a paying customer. Who happens to be a retired software developer and likes the Teensy platform and the user base, so I spend a some time and money helping supporting it.

I will take a look at your changes and try to verify at least most of the cases mentioned...

Note: My main usage for these API's is to support for communicating with Robotis Dynamixel Servos at Half duplex. Sometimes I have done it without needing this and simply setting up TX pin to do both TX and RX, but other times I use a buffer or buffer chips to do the half duplex plus up the voltage to 5v.

I never have run into this issue.

Mainly probably because of usage. I send out a packet with information (so all of it goes out without delays), and then if appropriate wait for a response packet. So never had the issue (that I detected)... But feels like something to work on minimizing.