Fix 128 Character Max Paste Into REPL by sommersoft · Pull Request #698 · adafruit/circuitpython

sommersoft · 2018-03-23T16:15:20Z

See Issue #417.

After all that digging through the USB read functions and ASF4, turns out we were just reading to often, before clearing the local USB buffer. usb_bytes_available now checks if the local USB buffer 1) has data per usb_rx_count, and 2) if we have space for a new USB read (64 bytes max).

I also commented out the recurring calls to start_read in both usb_read and read_complete. I don't think we need them, since usb_bytes_available and usb_cdc_background will always call it. Makes it easier to debug in the future, and keeps from having too many hands in the cookie jar. If anyone has objections, easy enough to uncomment. Likewise, if we want to delete them...just as easy.

jerryneedell · 2018-03-23T17:17:42Z

tried this on a metro_mr_express_revb:

Adafruit CircuitPython 3.0.0-alpha.3-23-gccbe557-dirty on 2018-03-23; Metro M4 Express Rev B (Black) with samd51j19
>>> 
>>> 
paste mode; Ctrl-C to cancel, Ctrl-D to finish
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== #123456789ABCDEF
=== 
>>> 
>>> 
>>> 
>>> 
>>> #123456789ABCDEF
>>> #123456789ABCDEF
>>> #123456789ABCDEF
>>> #123456789ABCDEF
>>> #123456789ABCDEF
>>> #123456789ABCDEF
>>> #123456789ABCDEF
>>> #12345678EF     TRUNCATED
>>> #123456789ABCDEF
>>>

so it seems to work in Paste Mode but still truncates in "normal" REPL -- Is this expected?

sommersoft · 2018-03-23T17:20:11Z

Thanks for testing @jerryneedell!! I'll go sniffing...

jerryneedell · 2018-03-23T17:34:50Z

Just for fun - here is a nice example of it working in "Paste Mode"

Adafruit CircuitPython 3.0.0-alpha.3-23-gccbe557-dirty on 2018-03-23; Metro M4 Express Rev B (Black) with samd51j19
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
paste mode; Ctrl-C to cancel, Ctrl-D to finish
=== import board
=== import digitalio
=== import busio
=== import time
=== import adafruit_bmp280
=== 
=== # Create library object using our Bus I2C port
=== i2c = busio.I2C(board.SCL, board.SDA)
=== bmp280 = adafruit_bmp280.Adafruit_BMP280_I2C(i2c)
=== 
=== # OR create library object using our Bus SPI port
=== #spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
=== #bmp_cs = digitalio.DigitalInOut(board.D10)
=== #bmp280 = adafruit_bmp280.Adafruit_BMP280_SPI(spi, bmp_cs)
=== 
=== # change this to match the location's pressure (hPa) at sea level
=== bmp280.seaLevelhPa = 1013.25
=== 
=== while True:
===     print("\nTemperature: %0.1f C" % bmp280.temperature)
===     print("Pressure: %0.1f hPa" % bmp280.pressure)
===     print("Altitude = %0.2f meters" % bmp280.altitude)
===     time.sleep(2)
=== 

Temperature: 23.9 C
Pressure: 1011.1 hPa
Altitude = 17.35 meters

Temperature: 23.9 C
Pressure: 1011.1 hPa
Altitude = 17.37 meters

sommersoft · 2018-03-23T18:47:10Z

So, my previous testing was only with continuous strings...testing fail on my part.

Using multiple lines would cause the pyexec_xxx_repl functions to trigger usb_cdc_background which was triggering unwanted reads to the USB buffer. This didn't happen in Paste Mode because it stays in it's own loop before exiting the pyexec_xxx_repl function, thereby not triggering usb_cdc_background.

@jerryneedell you are the ULTIMATE tester. Don't know what we'd do without you... Please give this another thorough run-through.

jerryneedell · 2018-03-23T19:02:05Z

nailed it ! -- almost -- see all three examples

Press any key to enter the REPL. Use CTRL-D to reload.

Adafruit CircuitPython 3.0.0-alpha.3-24-gf237657-dirty on 2018-03-23; Metro M4 Express Rev B (Black) with samd51j19
>>> 
>>> 
>>> 
>>> import board
>>> import digitalio
>>> import busio
>>> import time
>>> import adafruit_bmp280
>>> 
>>> # Create library object using our Bus I2C port
>>> i2c = busio.I2C(board.SCL, board.SDA)
>>> bmp280 = adafruit_bmp280.Adafruit_BMP280_I2C(i2c)
>>> 
>>> # OR create library object using our Bus SPI port
>>> #spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
>>> #bmp_cs = digitalio.DigitalInOut(board.D10)
>>> #bmp280 = adafruit_bmp280.Adafruit_BMP280_SPI(spi, bmp_cs)
>>> 
>>> # change this to match the location's pressure (hPa) at sea level
>>> bmp280.seaLevelhPa = 1013.25
>>> 
>>> print("\nTemperature: %0.1f C" % bmp280.temperature)

Temperature: 25.5 C
>>> print("Pressure: %0.1f hPa" % bmp280.pressure)
Pressure: 1010.8 hPa
>>> print("Altitude = %0.2f meters" % bmp280.altitude)
Altitude = 19.94 meters
>>> 
>>>

also with Paste Mode

Press any key to enter the REPL. Use CTRL-D to reload.

Adafruit CircuitPython 3.0.0-alpha.3-24-gf237657-dirty on 2018-03-23; Metro M4 Express Rev B (Black) with samd51j19
>>> 
paste mode; Ctrl-C to cancel, Ctrl-D to finish
=== import board
=== import digitalio
=== import busio
=== import time
=== import adafruit_bmp280
=== 
=== # Create library object using our Bus I2C port
=== i2c = busio.I2C(board.SCL, board.SDA)
=== bmp280 = adafruit_bmp280.Adafruit_BMP280_I2C(i2c)
=== 
=== # OR create library object using our Bus SPI port
=== #spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
=== #bmp_cs = digitalio.DigitalInOut(board.D10)
=== #bmp280 = adafruit_bmp280.Adafruit_BMP280_SPI(spi, bmp_cs)
=== 
=== # change this to match the location's pressure (hPa) at sea level
=== bmp280.seaLevelhPa = 1013.25
=== 
=== print("\nTemperature: %0.1f C" % bmp280.temperature)
=== print("Pressure: %0.1f hPa" % bmp280.pressure)
=== print("Altitude = %0.2f meters" % bmp280.altitude)
=== 
=== 

Temperature: 25.6 C
Pressure: 1010.8 hPa
Altitude = 19.71 meters
>>>

but - sometimes paste mode fails with a syntax error -- I'm not sure why.
This is reminiscent of some early issues we had with REPL sometimes giving syntax errors on Line 1.

Adafruit CircuitPython 3.0.0-alpha.3-24-gf237657-dirty on 2018-03-23; Metro M4 Express Rev B (Black) with samd51j19
>>> 
>>> 
>>> 
paste mode; Ctrl-C to cancel, Ctrl-D to finish
=== import board
=== import digitalio
=== import busio
=== import time
=== import adafruit_bmp280
=== 
=== # Create library object using our Bus I2C port
=== i2c = busio.I2C(board.SCL, board.SDA)
=== bmp280 = adafruit_bmp280.Adafruit_BMP280_I2C(i2c)
=== 
=== # OR create library object using our Bus SPI port
=== #spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
=== #bmp_cs = digitalio.DigitalInOut(board.D10)
=== #bmp280 = adafruit_bmp280.Adafruit_BMP280_SPI(spi, bmp_cs)
=== 
=== # change this to match the location's pressure (hPa) at sea level
=== bmp280.seaLevelhPa = 1013.25
=== 
=== print("\nTemperature: %0.1f C" % bmp280.temperature)
=== print("Pressure: %0.1f hPa" % bmp280.pressure)
=== print("Altitude = %0.2f meters" % bmp280.altitude)
=== 
=== 
=== 
Traceback (most recent call last):
  File "<stdin>", line 1
SyntaxError: invalid syntax
>>> 
>>>

jerryneedell · 2018-03-23T19:05:59Z

Note - the occasional paste mode failures are not new to the latest commit. They occurred in the previous one as well. I thought it was user error but now it looks like a possible issue.

jerryneedell · 2018-03-23T19:11:52Z

the syntax errors I was thinking about were from #124
May or may not be relevant...

tannewt

Amazing work! Thank you for figuring this out and improving it.

tannewt · 2018-03-23T21:27:08Z

            return true;
        }
-    }
+    }*/


Go ahead and delete this.

tannewt · 2018-03-23T21:27:49Z

 bool usb_bytes_available(void) {
+    // Check if the buffer has data, but not enough
+    // space to hold another read.
+    if (usb_rx_count > 64) {


Want to use USB_RX_BUF_SIZE here so this changes with it?

Can do. Or, we can use the EP max packet size constant; any future use of USB HS is what would be the determination of going above 64 bytes per EP buffer. Yes?

tannewt · 2018-03-23T21:28:43Z

+    /*if (!pending_read && usb_rx_count == USB_RX_BUF_SIZE - 1) {
        start_read();
-    }
+    }*/


Go ahead and delete this too. Git has it in its history.

tannewt · 2018-03-23T21:29:23Z

    if (mp_interrupt_char != -1 && cdc_enabled() && !pending_read) {
-        start_read();
+        // Make sure we have space in the buffer
+        if (usb_rx_count < 64) {


Use USB_RX_BUF_SIZE here too and merge the condition with the outside if statement. No need to have two.

sommersoft · 2018-03-23T21:30:23Z

The only change I see so far from the fix to #124 is the addition of VFS gc collection. Could have reintroduced the previous problem. Will keep looking, unless wiser minds chime in...

tannewt · 2018-03-23T21:33:02Z

Lets get this in and we can chase the syntax error down separately if we want.

tannewt · 2018-03-23T21:35:55Z

You know better than I do. :-)

…

On Fri, Mar 23, 2018 at 2:34 PM sommersoft ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ports/atmel-samd/usb.c <#698 (comment)> : > @@ -243,9 +245,16 @@ static bool cdc_enabled(void) { } bool usb_bytes_available(void) { + // Check if the buffer has data, but not enough + // space to hold another read. + if (usb_rx_count > 64) { Can do. Or, we can use the EP max packet size constant; any future use of USB HS is what would be the determination of going above 64 bytes per EP buffer. Yes? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#698 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADNqXYMD8J7GUQwvoby9dCFi96FS1VXks5thWpTgaJpZM4S4_xe> .

jerryneedell · 2018-03-23T21:36:26Z

No objection from me! Great work on this. I think it will be good for others to use it and see how often the syntax error occurs. For me it is about 25% to 50% of the time but there was a lot of flailing around.

sommersoft · 2018-03-24T00:59:54Z

So, I pulled back on all of the "futurization" I mentioned earlier in discord. The only SAM chips that run High Speed USB are M7s; no real reason to put that in for now.

tannewt

Thanks! I really appreciate you tackling this.

tannewt · 2018-03-26T05:48:41Z

    // Check if the buffer has data, but not enough
    // space to hold another read.
-    if (usb_rx_count > 64) {
+    if (usb_rx_count > CDC_BULKOUT_SIZE) {


You've been awesome with feedback so I'll nitpick just a smidge more. This should be USB_RX_BUF_SIZE - usb_rx_count > CDC_BULKOUT_SIZE here and below. This works now because USB_RX_BUF_SIZE is 128. This will break if we make that buffer bigger.

USB_RX_BUF_SIZE - usb_rx_count > CDC_BULKOUT_SIZE caused REPL to be non-responsive. Puzzled me until I thought about the truth table.

usb_rx_count = 0 # REPL will always start at zero USB_RX_BUF_SIZE = 128 CDC_BULKOUT_SIZE = 64 if ((128 - 0) > 64) { return > 0;}

All that was needed was flipping the buffer to the other side of the eval. Tests successfully.

tannewt

Yup, good catch on the flip. One more nitpicky thing for clarity. Great work!

tannewt · 2018-03-26T07:01:17Z

    // space to hold another read.
-    if (usb_rx_count > CDC_BULKOUT_SIZE) {
+    if (usb_rx_count > USB_RX_BUF_SIZE - CDC_BULKOUT_SIZE) {
        return usb_rx_count > 0;


One last thing. This will always be true because USB_RX_BUF_SIZE - CDC_BULKOUT_SIZE is always more than zero. So, just return true;.

tannewt

W0000t!!!! Thanks!

sommersoft added 2 commits March 23, 2018 15:07

add buffer check before triggering new usb read

d434635

removed leftover debugging bits

ccbe557

extended buffer check to usb_cdc_background

f237657

tannewt self-requested a review March 23, 2018 21:25

tannewt requested changes Mar 23, 2018

View reviewed changes

updated with requested changes

ef16109

tannewt requested changes Mar 26, 2018

View reviewed changes

future-proof for buffer size changes

23009fd

tannewt requested changes Mar 26, 2018

View reviewed changes

minor cleanup

9bd55cf

tannewt approved these changes Mar 26, 2018

View reviewed changes

tannewt merged commit ea39f43 into adafruit:master Mar 26, 2018

sommersoft deleted the repl_fix branch March 26, 2018 21:17

sommersoft mentioned this pull request Mar 31, 2018

3.0: can only paste 128 chars into REPL #417

Closed

sommersoft mentioned this pull request Jul 13, 2018

nrf pca10056 - control C will not break out of loop #1021

Closed

Conversation

sommersoft commented Mar 23, 2018

Uh oh!

jerryneedell commented Mar 23, 2018

Uh oh!

sommersoft commented Mar 23, 2018

Uh oh!

jerryneedell commented Mar 23, 2018

Uh oh!

sommersoft commented Mar 23, 2018

Uh oh!

jerryneedell commented Mar 23, 2018

Uh oh!

jerryneedell commented Mar 23, 2018

Uh oh!

jerryneedell commented Mar 23, 2018

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sommersoft commented Mar 23, 2018

Uh oh!

tannewt commented Mar 23, 2018

Uh oh!

tannewt commented Mar 23, 2018 via email

Uh oh!

jerryneedell commented Mar 23, 2018

Uh oh!

sommersoft commented Mar 24, 2018

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants