Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

USB serial stops delivering data after large segment sent to Teensy #401

Closed
blackketter opened this issue Nov 1, 2019 · 24 comments
Closed

Comments

@blackketter
Copy link

I think I'm seeing a bug in usb_serial on Teensy 4.0. When I try sending a block of text larger than 2413 bytes via the serial port, it locks up. Here's the sketch:
#include <Arduino.h>

void setup() {

}

void loop() {
  if (Serial.available()) {
    int x = Serial.read();
    Serial.write(x);
  }
}

I put 2414 bytes of text into foo.txt and from the macOS terminal:

cat foo.txt > /dev/cu.usbmodem62442801

This command get stuck as the teensy is no longer reading bytes. Putting 2413 bytes in the file, it works fine. This and much larger text files work fine on Teensy 3.6.

Teensy 1.147 built via platformio. macOS 10.14.6

Discussion here: https://forum.pjrc.com/threads/57995-Teensy-4-0-USB-serial-lockup

@akumpf
Copy link

akumpf commented Nov 6, 2019

I can confirm this bug, and it's a pretty major blocker for anyone trying to send even moderate amounts of serial data to a Teensy 4 over USB.

After testing a range of inputs, I'm able to reproduce the bug with any input chunk greater than 63 bytes.

Example sketch (just a Serial echo):

// As simple as it gets. Just echo back what's seen over USB serial.
void setup() {
  Serial.begin(115200); // baud doesn't matter (native USB).
}
void loop() {
  while(Serial.available()){
    Serial.print((char)Serial.read());
  }
}

For inputs less than or equal to 63 bytes, it will echo back appropriately. Anything over that and it will show the first 64 of what was sent, and then never show anything more. For example, just send 111111111111111111111111111111111111111111111111111111111111123 and it will work every time... add one more digit (1111111111111111111111111111111111111111111111111111111111111234) and it will work once and never see any more data.

The behavior leads me to believe that the bug occurs when a USB RX buffer (always 64 bytes) is full or runs over; either (1) the next buffer is not being filled correctly, or (2) the counter/index pointing to the received data is not being updated correctly.

@PaulStoffregen Any help would be greatly appreciated! I've been poking around in usb_serial.h/c files but not familiar enough with the codebase to make much headway. I've also tried the handful of suggestions over in the forum (setTimeout(0), forcing a Serial.read() etc. but no change in behavior).

@akumpf
Copy link

akumpf commented Nov 6, 2019

It also seems worth noting that you can put in a delay (so the code only periodically reads from serial) and as long as each chunk that is sent is 63 bytes or less, there can be a bunch of chunks queued up just fine. Each iteration will then read one buffer's worth (the 63 bytes), report 0 available, and then the next time around see the next 63 bytes.

The momentary delay between chunks seems to be due to the usb_prepare_transfer() and usb_receive() calls under the hood when reading a character, used to push data from 1 buffer to the next which appears to happen asynchronously. A hack here is to add delayMicroseconds(100); after the prepare/receive calls to allow time for the transfer to happen.

Teensy 4 seems hackable to get things talking, but ooooffff... limiting data input to 63 bytes at a time and needing to throttle reads to see things consistently feels like a major step back from Teensy 3.

@akumpf
Copy link

akumpf commented Nov 6, 2019

Just pulled in the latest Teensy4 core (73ea157) and the receive behavior is much better. Handles much larger incoming data chunks (384 bytes at a time). Same delayMicroseconds(100); hack in usb_serial.c allows for continued looping as buffers swap.

Thanks @PaulStoffregen -- Keep up the great work on Teensy!

@blackketter
Copy link
Author

Thanks, @akumpf! I'd like to try the delayMicroseconds(100) hack, can you share exactly where you added that call?

@akumpf
Copy link

akumpf commented Nov 6, 2019

Sure @blackketter. In the usb_Serial.c file within Teensy4 avr core (embedded in Arduino.app on Mac it should be somewhere like: /Applications/Arduino.app/Contents/Java/hardware/teensy/avr/cores/teensy4/usb_serial.c), I added the delayMicroseconds(100); line after the usb_receive(...) call:

// get the next character, or -1 if nothing received
int usb_serial_getchar(void)
{
	if (rx_index[0] < rx_count[0]) {
		int c = rx_buffer[rx_index[0]++];
		if (rx_index[0] >= rx_count[0]) {
			// reschedule transfer
			usb_prepare_transfer(rx_transfer + 0, rx_buffer + 0, rx_packet_size, 0);
			usb_receive(CDC_RX_ENDPOINT, rx_transfer + 0);
			delayMicroseconds(100);
		}
		return c;
	}
	return -1;
}

@PaulStoffregen
Copy link
Owner

Just to confirm, I have a similar report (via the forum) on my high-priority list of bug to fix for 1.49.

https://forum.pjrc.com/threads/57946-Teensy4-0-does-not-receive-data-longer-than-62-characters

Even though this is high priority, at this moment I'm fully occupied by an even higher priority PJRC business matter. I have travel planned soon too, so odds are slim I'll be able to do much before I return on Nov 25th.

Really wish I had a better answer right now, but I'm only one guy. I absolutely will be working on this problem later this month. If you haven't heard anything by the Thanksgiving holiday, please ping me. This really does need to get fixed.

@blackketter
Copy link
Author

Thanks for the update, @PaulStoffregen

@PaulStoffregen
Copy link
Owner

I've committed a fix for the issue reported on the forum.

24e0248

However, I suspect this issue may also be triggering a bug within the new high speed serial monitor code. I will publish a 1.49-beta1 installer soon, which rolls up this change and the other USB improvements. Could really use some help with testing when that's published.

@blackketter
Copy link
Author

That seemed to fix the issue for me, @PaulStoffregen. Thanks!

@PaulStoffregen
Copy link
Owner

@akumpf - Any chance you could give this a try and confirm whether it solves the problem you found? (without adding any delays)

@PaulStoffregen
Copy link
Owner

This issue was also discussed on this forum thread.

https://forum.pjrc.com/threads/57995-Teensy-4-0-USB-serial-lockup

@akumpf
Copy link

akumpf commented Dec 1, 2019

@PaulStoffregen I can confirm that the latest code now handles USB serial input upto 4097 bytes as expected (with any arbitrary read interval in code).

At 4098 or above, the extra data that is not read in time seems to be lost (as expected without some other form of explicit rate limiting or flow control) -- the Teensy continues to run and see subsequent data (after reading what's in the buffer) normally without any freezing or lock ups.

Nice work, Paul!

@PaulStoffregen
Copy link
Owner

It's supposed to have flow control, so anything beyond the 4K buffer is supposed to wait and get properly received after you read some of the buffered data.

Can you tell me how you're testing, so I can try to reproduce the data loss after 2097 bytes?

@akumpf
Copy link

akumpf commented Dec 1, 2019

I was just dumping ascii text into the arduino Serial Monitor with the previously mentioned echo sketch (#401 (comment)). If the read loop is reasonably tight, it'll read as much as you throw at it.

However, if you add a significant delay between each read loop (like a full second, which is ridiculous for normal use, but just for testing), it'll echo back the first 4097 characters, but lose the data beyond that. Perhaps I was being too hard with it (if there's some built in flow control timeout). Hope that helps.

@luni64
Copy link
Contributor

luni64 commented Dec 1, 2019

Tried it with a real life application which sends 480kB in 0.8s to the Teensy (600kB/s). It sends data in 4kB chunks.

The transfer speed didn't change compared to the version before the latest changes. Neither the PC software nor Teensy firmware are speed optimized. Teensy does some processing after each received byte which is probably speed limiting.

Anyway, it runs out of the box without problems which is very good :-)

@PaulStoffregen
Copy link
Owner

PaulStoffregen commented Dec 6, 2019

However, if you add a significant delay between each read loop (like a full second, which is ridiculous for normal use, but just for testing), it'll echo back the first 4097 characters, but lose the data beyond that.

I'm running a test now, using this program which is the closest I can imagine to what you've described.

void setup() {
  Serial.begin(115200);
}
void loop() {
  while(Serial.available()){
    Serial.print((char)Serial.read());
    delay(1000);
  }
}

I creates a file with all the comments (excluding code) from this issue. It's 5882 bytes. I opened the file and copied it all to the clipboard. The I opened the serial monitor and copied it all into the line on the top and clicked "Send".

The program is slowly echoing it all back, 1 character at a time. It should take 98 minutes and should end with "Anyway, it runs out of the box without problems which is very good :-)". If only 4097 bytes echo back, then it should finish is 69 minutes and end somewhere in one of the earlier messages.

Will post again when it stops...

@akumpf
Copy link

akumpf commented Dec 6, 2019

@PaulStoffregen Curious to hear how it turns out. My approach was a bit faster to see the fault. I put the delay after the while(Serial.available()) loop.

void setup() {
  Serial.begin(115200);
}
void loop() {
  while(Serial.available()){
    Serial.print((char)Serial.read());
  }
  delay(1000); // I also played with this upto 3 seconds.
}

@PaulStoffregen
Copy link
Owner

Looks like it worked.

screen

@PaulStoffregen
Copy link
Owner

PaulStoffregen commented Dec 6, 2019

Tried it also on Windows 10, but only your faster version. Seems to echo all 5852 characters.

capture

@PaulStoffregen
Copy link
Owner

PaulStoffregen commented Dec 6, 2019

I tried one more test, copying the 5852 bytes 9 times, with "11", "22", "33", etc typed after each. So that ought to be approx 52600 bytes all sent at once from the serial monitor.

Again, it seems to work fine. Here's the final result.

capture1

As a sanity check, I did a long horizontal scroll through the text. It all seems to be there. Here's the place when you can see the first output all on the first line ends and the horizontal scroll bar is indeed about 10% of the way.

capture2

@PaulStoffregen
Copy link
Owner

@akumpf - Any other suggestions how I should try to test?

If there really is a problem here, I'd sure like to reproduce it and fix it before replicating this USB serial code into USB MIDI and other protocols.

Any chance I can talk you into retesting with 1.49-beta1?

https://forum.pjrc.com/threads/58567-Teensyduino-1-49-Beta-1

If you do find a way to cause Serial to not work, please take a moment to share the exact code you upload to Teensy 4.0 and a screenshot or other specific details of exactly what you did. I can almost always get to the bottom of a problem I'm able to reproduce. But as you can see from these recent tests, I don't have to way to reproduce the problems you've seen. Will you please help me to reproduce it here?

@akumpf
Copy link

akumpf commented Dec 7, 2019

@PaulStoffregen Trying now. I'll report back shortly.

@akumpf
Copy link

akumpf commented Dec 8, 2019

@PaulStoffregen 1.49-beta1 seems solid with anything I throw at it for the USBSerial echo test (#401 (comment), with various delays) to a Teensy 4.0. Nice work! 😄

No idea what the issue was before, but you seemed to have ironed it out.

For reference, I'm testing on OSX (v10.14.6) via Arduino (v1.8.9) Serial Monitor to send ASCII.

@PaulStoffregen
Copy link
Owner

Great news. Thanks for testing. I'm going to close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants