adafruit / circuitpython

CircuitPython - a Python implementation for teaching coding with microcontrollers
https://circuitpython.org
Other
4.09k stars 1.21k forks source link

bulk pasting code extremely flaky on nRF #3152

Open jepler opened 4 years ago

jepler commented 4 years ago

When pasting blocks of code to the REPL there are lost characters very often. I don't see this on samd51, but I see it all the time on nrf52.

How to reproduce:

Expected behavior: The line of code calculates the number 0 Actual behavior: It frequently results in other values because one or more characters were dropped. e.g.,

>>> 123456789123456789123456789123456789123456789123456789123456789123456789%11
0
>>> 123456789123456789123456789123456789123456789123456789123456789123456789%11
0
>>> 1234567891234567891234567891234567891234567891345678923456789123456789%11
2
>>> 123456789123456789123456789123456789123456789123456789123456789123456789%11
0

The size of the USB RX buffer is 1024, same as samd, so this does NOT seem like it could be to the FIFO filling up on account of processing delays.

It's worth noting, I saw under gdb that at least with the tio terminal program the characters were received by tinyusb one at a time. I didn't ever reproduce a lost character when using the debugger to monitor the state of the rx fifo, but of course this ran much more slowly so timing-dependent weirdness could change or go away.

I feel like I filed an issue about this before but I couldn't find it. Please forgive me filing this fresh issue if it's a dup.

tannewt commented 4 years ago

Unless you plan on doing this soon for 6.x please put this under Long Term. I want to be stricter about putting things in milestones. To mark it as a bug, use the label instead. Thanks!

kevinjwalters commented 4 years ago

I'm curious what version of CircuitPython is this and do you have a screen attached to the device showing the console output on TFT LCD? I have a five second play with this on Win8 with two CLUEs running 5.3.0 and can't reproduce but that's a very short test.

kevinjwalters commented 4 years ago

I upped the character count with some text from t'Internet and got a different bug. The console has frozen on me on a CLUE running 5.3.0. Here's one good then one that just stopped outputting

>>> len("What of the future? It is a matter of some concern to Real Programmers that the latest generation of computer programmers are not being brought up with the same outlook on life as their elders. Many of them have never seen a computer with a front panel. Hardly anyone graduating from school these days can do hex arithmetic without a calculator. College graduates these days are soft -- protected from the realities of programming by source level debuggers, text editors that count parentheses, and user friendly operating systems. Worst of all, some of these alleged computer scientists manage to get degrees without ever learning FORTRAN! Are we destined to become an industry of Unix hackers and Pascal programmers? On the contrary. From my experience, I can only report that the future is bright for Real Programmers everywhere. Neither OS/370 nor FORTRAN show any signs of dying out, despite all the efforts of Pascal programmers the world over. Even more subtle tricks, like adding structured coding constructs to FORTRAN have failed. Oh sure, some computer vendors have come out with FORTRAN 77 compilers, but every one of them has a way of converting itself back into a FORTRAN 66 compiler at the drop of an option card -- to compile DO loops like God meant them to be.") - 1277
0
>>> len("What of the future? It is a matter of some concern to Real Programmers that the latest generation of computer programmers are not being brought up with the same outlook on life as their elders. Many of them have never seen a computer with a front panel. Hardly anyone graduating from school these days can do hex arithmetic without a calculator. College graduates these days are soft -- protected from the realities of programming by source level debuggers, text editors that count parentheses, and user friendly operating systems. Worst of all, some of these alleged computer scientists manage to get degrees without ever learning FORTRAN! Are we destined to become an industry of Unix hackers and Pascal programmers? On the contrary. From my experience, I can only report that the future is bright for Real Programmers everywhere. Neither OS/370 nor FORTRAN show any signs of dying out, despite all the efforts of Pascal programmers the world over. Even more subtle tricks, like adding structured coding constructs to FORTRAN have failed. Oh sure, some computer vendors have come out with FORTRAN 77 compilers, but every one of them has a way of

CIRCUITPY is still accessible there. The screen has frozen at the exact same point. I can reconnect to the console but still is dead.

dunkmann00 commented 4 years ago

Expected behavior: The line of code calculates the number 1

I think you meant calculates the number 0?

jepler commented 4 years ago

Thanks @dunkmann00 I updated the initial comment

DavePutz commented 4 years ago

@jepler - what was the host system you were talking to? I was unable to reproduce using a Windows 10 system, but running Ubuntu on the same box I saw the issue. Using Wireshark showed that on Linux the characters were coming in from the USB much faster - 5-6x faster, to be exact (Update: See the following comment for clarification). Nearly all the time I measured between calls to readline_process_char() was being used to run background tasks, so I think perhaps with a fast USB and slower clock speeds we must be missing an occasional interrupt.

DavePutz commented 4 years ago

Further investigation reveals that this issue is related to the terminal emulator being used. Some emulators will send in the entire pasted string in one packet, which always succeeds. Others send in one character at a time, which is prone to the error. Emulators tested so far that work are putty (windows & Linux), TeraTerm(Windows), and screen (Linux). The failures were seen using minicom on Linux. Given the high usb rate (115200 baud) and relatively slow CPU speeds on nrf, I'm not sure that CP could handle a long stream of single-byte packets successfully without some major work reducing the time the USB stack takes handling each packet.

kevinjwalters commented 4 years ago

At what lower (tx) baud rate from the Windows/Linux end does this work (more) reliably with minicom?

DavePutz commented 4 years ago

The baud rate actually does not matter ( at least not on the Circuitplayground Bluefruit I was testing with) For USB the baud rate only is used if the USB is going through a USB->serial converter. If it is 'pure' USB then baud does not matter. I did test with baud rates from 115200 to 9600 just to make sure what I thought I knew was true; and the results did not vary. The only reliable fix I found was to use a terminal emulator that sends the entire pasted string at once.

On Tue, Jul 28, 2020 at 3:19 PM kevinjwalters notifications@github.com wrote:

At what lower (tx) baud rate from the Windows/Linux end does this work (more) reliably?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/adafruit/circuitpython/issues/3152#issuecomment-665260497, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFNJKESLIE4PYPU7L4HQJWDR54XF3ANCNFSM4OZWBQFQ .

kevinjwalters commented 4 years ago

@DavePutz Interesting, I've always wondered if baud rate has any meaning on serial over USB but never looked into it. I'm from the 300 era but I just connected using 110 from PuTTY and it doesn't feel at all like 110 :)

I'm thinking this is more relevant in Arduino-land. I'll check that the next time I'm there.

jepler commented 4 years ago

I use tio on debian linux. At least from how it looked on the receiving side, it does transmit one byte at a time. I'll investigate possible alternatives to tio.

dhalbert commented 4 years ago

I use this alias:

alias repl='picocom -q  -b 115200 /dev/ttyACM0'
jepler commented 4 years ago

Even with tio, 6.0.0.alpha.2 is much better than 5.1.0, the two uf2s I had handy. Now USB tasks are not delayed until the next 1ms tick, making them less likely to get "backed up", I guess.

Modifying tio to do multiple bytes of I/O at once makes the problem go away and also makes it feel faster when pasting lots of code. Interesting. I may PR this with the tio maintainers though it looks like the version I modified (debian stable) is pretty out of date--the changes don't apply directly, but they are still doing I/O character by character. (I like tio because it will always re-open the terminal device when resetting / re-plugging so I don't have to restart it)

jepler commented 4 years ago

I set this to long term since the impact is low and there are workarounds

hathach commented 2 years ago

@jepler https://github.com/hathach/tinyusb/pull/1208 help a lot with this PR. It is worth a re-try. In my test, it does still has missing characters, but much less frequent. I have to hold ctrl+shift+v (constantly pasting with newline) to reproduce that. It could be caused by other buffer overflow or something.

FoamyGuy commented 2 years ago

I do think this is much better after https://github.com/hathach/tinyusb/pull/1208 :tada:

I tested for a bit with a CLUE pasting:

print(123456789123456789123456789123456789123456789123456789123456789123456789%11)

Over 2-3 dozen tests pasting that I had only 3 times where there was a difference in what got pasted in the REPL. With a current stock build of circuitpython the ratio of different pasted values felt much higher (I didn't record stats for it but felt more like 50% or so having different pastes).

I recently learned that Paste Mode exists and did some of my testing with that as well to see if it made it difference I did see 1 out of the 3 different pasted values while using paste mode so it seems to me like using paste mode vs. directly into the REPL does not make a difference with regards to this issue.

Thanks for this update and all of the work you do @hathach

hathach commented 2 years ago

thank @FoamyGuy for trying it out, it does not fix this issue 100%. I guess there is (probably) other factor such as buffer overflow of some kind. I will make an PR to update tinyusb soon.