search

adafruit / circuitpython

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

RP2040 and Mu Editor cause Ctrl-C hang-ups #7183

Open Kriechi opened 1 year ago

Kriechi commented 1 year ago

CircuitPython version

Adafruit CircuitPython 7.3.3 on 2022-08-29; Raspberry Pi Pico with rp2040
Board ID:raspberry_pi_pico

Code/REPL

import time
print("Hello World!")

while True:
    print("foobar" * 50)
    time.sleep(1)

Behavior

Code runs and prints messages once per second as expected.

Interrupting the code execution is not possible in this state. I have to reset the board to bring it back. Even a soft-reload via auto-reload on USB file saving does not fix the input problem: it does reload and restart executing, but Ctrl-C is still not recognized.

Description

It is 100% reproducible for me with the Mu Editor only.

Mu Editor logs:

Starting Mu 1.1.1
2022-11-09 11:02:10,230 - root:270(run) INFO: uname_result(system='Darwin', node='147ddaa50b43.ant.amazon.com', release='21.6.0', version='Darwin Kernel Version 21.6.0: Thu Sep 29 20:12:57 PDT 2022; root:xnu-8020.240.7~1/RELEASE_X86_64', machine='x86_64', processor='i386')
2022-11-09 11:02:10,252 - root:271(run) INFO: Platform: macOS-10.16-x86_64-i386-64bit
...
2022-11-09 11:02:12,690 - mu.logic:1549(change_mode) INFO: Workspace directory: /Volumes/CIRCUITPY
2022-11-09 11:02:12,785 - mu.logic:962(restore_session) INFO: Starting with blank file.
2022-11-09 11:02:13,561 - mu.logic:747(check_usb) INFO: circuitpython device connected on port: /dev/cu.usbmodem14301(VID: 0x239A, PID: 0x80F4, manufacturer: 'Raspberry Pi')
2022-11-09 11:02:19,350 - mu.modes.base:112(open) INFO: Connecting to REPL on port: /dev/cu.usbmodem14301
2022-11-09 11:02:19,362 - mu.modes.base:130(open) INFO: Connected to REPL on port: /dev/cu.usbmodem14301
2022-11-09 11:02:19,373 - mu.modes.base:503(add_repl) INFO: Started REPL on port: /dev/cu.usbmodem14301
2022-11-09 11:02:19,374 - mu.modes.base:474(toggle_repl) INFO: Toggle REPL on.
2022-11-09 11:03:25,183 - mu.modes.base:136(close) INFO: Closing connection to REPL on port: /dev/cu.usbmodem14301
2022-11-09 11:03:25,189 - mu.modes.base:471(toggle_repl) INFO: Toggle REPL off.

related bug reports that keep getting closed or pointed to some other component: https://github.com/mu-editor/mu/issues/1505

and other similar ones: https://github.com/mu-editor/mu/issues/842 https://forums.adafruit.com/viewtopic.php?f=60&t=151826 https://github.com/micropython/micropython/issues/7867 https://github.com/micropython/micropython/issues/7996 https://github.com/micropython/micropython/commit/587339022689187a1acbccc1d0e2425a67385ff7

Additional information

No response

dhalbert commented 1 year ago

This might be due to escape-code handling. @Neradoc notes here: https://github.com/mu-editor/mu/issues/1505#issuecomment-830061810 that

serial_port = "/dev/cu.usbmodem144443131"
import serial
VT100_RIGHT = b"\x1B[C"
channel = serial.Serial(serial_port)
channel.write(VT100_RIGHT*50)
channel.close()

causes the issue.

jepler commented 1 year ago

I don't think it has to do with escape codes. After pasting "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX" to my terminal program, ctrl-c doesn't work either. I suspect an internal serial buffer fills and that prevents ctrl-c from being received..

jepler commented 1 year ago

@hathach can you comment here? We are using tud_cdc_set_wanted_char(CHAR_CTRL_C); and depending that we'll see a call from tud_cdc_rx_wanted_cb. However, when a lot of characters have gone un-read, the wanted CB is no longer called. Is this a TinyUSB bug or do we need to change what we do in CircuitPython (for instance, to do something to drain the un-read characters even if the Python code isn't reading anything)?

jepler commented 1 year ago

@tannewt @dhalbert and I discussed this and we feel that there's no way we can make ctrl-c work -- if we potentially throw away characters from the buffer, it will break things that rely on reliable buffers like file uploads in mu and thonny. But if we don't throw away characters, the "ctrl-c" method can't work, because the ctrl-c character never leaves the host computer.

We might be able to add an alternative out of band interrupt signal such as using DTR or BREAK, which would then have to be added to advanced editors. TinyUSB seems to have a "send break" callback. I'm interested, know how to send break from tio so I'll look into it.

jepler commented 1 year ago

as another alternative, maybe we could discard any pending characters when USB CDC disconnects.

tannewt commented 1 year ago

Here are the TinyUSB callback APIs: https://github.com/hathach/tinyusb/blob/73896a3b71c525a3ee4cefa7e35ce3b3a93786ef/src/class/cdc/cdc_device.h#L141-L148

hathach commented 1 year ago

@jepler I am late for the party, from what I could understand, Ctrl-C isn't sent to tinyusb since the rx buffer is already full !! If it is the case then indeed, we couldn't resolve this without dropping characters unless as you mentioned we use an additional channel such as modem/control signal. Let me know which modem signal that you plan to use and isn't supported yet by tinyusb, BREAK seems to be a great choice.

dhalbert commented 1 year ago

Break now needs to be implemented in Mu and other REPL terminals.

Kriechi commented 1 year ago

@dhalbert I assume you are referring to this PR?

While this sounds like an improvement - the "unexpected behaviour" for users still is not fixed - until everybody learns about this new break control signal and all terminals implement it.

What would happen if the input buffer gets flushed, or discards just enough data to at least receive one Ctrl-C signal / 0x03 character from the user? Do we have such fine-grained control over the buffer? and would discarding data, i.e., prioritising the well-known Ctrl-C input over any other user (ascii) input from the input buffer be more or less surprising to the user?

dhalbert commented 1 year ago

@Kriechi As mentioned above, if we discard characters when the buffer is full, then we could be discarding valuable data. Unfortunately, there is no way to tell that a ctrl-C is waiting on the host side without reading it over USB; it is not prioritized in any way by the host. The "break" scheme is the only "out of band" method to communicate something down to the board.

jepler commented 1 year ago

It's not the only possible way (there are other serial control lines we could have chosen) but it's what we've chosen to implement. This implementation doesn't have to be the only solution, and having a conversation to arrive at a better solution would be welcome. Dan accurately identifies why we want a solution that is not an in-band character value as the only way to generate a KeyboardInterrupt, and why we don't want to implement a solution that will lose characters.

jepler commented 1 year ago

If there's concern about this solution we also have the option of marking it as an unstable feature e.g., via our release notes and documentation (in fact I probably failed to adequately document this, because I don't remember doing it)

Neradoc commented 1 year ago

My vote when I reported the issue the first time was for Mu to stop spamming the serial port with cursor movements, that would remove 99% of the cases encountered in the wild IMO. I can reopen that Mu issue with suggestions.

On a side node, considering this:

Even a soft-reload via auto-reload on USB file saving does not fix the input problem: it does reload and restart executing, but Ctrl-C is still not recognized.

We could reset the buffer on reloads maybe ? Or are there situations where a user is sending valuable data to the serial port and expects it to be received across reloads ?

jepler commented 1 year ago

I considered, but did not implement

I did not know if either of these scenarios would help mu. However, at least the first alternative might also help users, because re-starting the terminal program could be a natural impulse in the case that the buffer was filled. I'm not aware of how to do this in the TinyUSB interface but could investigate it.

Kriechi commented 1 year ago

:thumbsup: Yes, discarding pending data would improve UX a lot - IMO an intuitive reflex in Mu and any other serial terminal, is to either close+reopen the Serial tab/panel/window or restart the connection.

dhalbert commented 1 year ago

If CDC disconnects, I think all bets are off, and it would be OK to discard that data. I did not want to discard data casually on an existing connection.

dhalbert commented 1 year ago

@Neradoc Your mu issue appears to be https://github.com/mu-editor/mu/issues/1505. I think it would be good to re-open that and point out that a running program that is not receiving input may have far too much input piled up. I also do not even see the point of the cursor movements, since the Serial window in Mu should be a stream, not an editor window. Do you understand the use case for Mu to generate those cursor movements.

Neradoc commented 1 year ago

I also do not even see the point of the cursor movements, since the Serial window in Mu should be a stream, not an editor window. Do you understand the use case for Mu to generate those cursor movements.

It only makes sense when in the REPL typing a command, it lets you click to move the insertion point inside your command, and there is even code to select and delete the selection. It doesn't seem super useful to me, but I'll admit to being used to moving the cursor with arrows.