Skip to content

Fix for Issue #2689 #3312

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

Closed
wants to merge 6 commits into from
Closed

Fix for Issue #2689 #3312

wants to merge 6 commits into from

Conversation

DavePutz
Copy link
Collaborator

@DavePutz DavePutz commented Aug 21, 2020

When hitting CTRL-C during the long output described in issue #2689 the pending exception was never being cleared.
As a result; in displayio_background() the call to mp_hal_is_interrupted() would always return true and the displayio screen would stop being updated. This patch simply clears any pending exceptions when readline_init() is called (i.e. when the REPL prompt is being printed).

Closes: #2689

@jepler
Copy link

jepler commented Aug 23, 2020

[I had trouble adding the related issue to Linked Issues, until I edited it into the description of this PR]

@jepler
Copy link

jepler commented Aug 23, 2020

Hello, and thanks!

I'd like to understand a bit better what is happening here, so please bear with me.

When ctrl-c is pressed and this bug occurs, I assume that (on serial) it just prints the ">>>" prompt, and not an exception? (in contrast to the interruption of a long-running calculation, say)

What type/value does mp_pending_exception have when this code is reached? Superficially, it looks like just after the call to readline_init, the call to readline_process_char will call mp_hal_stdin_rx_chr which is intended to fully deal with any exception stored in mp_pending_exception.

        // Check to see if we've been CTRL-Ced by autoreload or the user.
        if (MP_STATE_VM(mp_pending_exception) == MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_kbd_exception))) {
            // clear exception and generate stacktrace
            MP_STATE_VM(mp_pending_exception) = MP_OBJ_NULL;
            nlr_raise(&MP_STATE_VM(mp_kbd_exception));
          }

Should mp_hal_stdin_rx_chr have a catch-all for any (non-NULL) exception? I didn't find any place where an unanticipated exception was stored...

lib/utils/interrupt_char.c:    MP_STATE_VM(mp_pending_exception) = MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_kbd_exception));
ports/nrf/common-hal/watchdog/WatchDogTimer.c:    MP_STATE_VM(mp_pending_exception) = &mp_watchdog_timeout_exception;
ports/unix/unix_mphal.c:        MP_STATE_VM(mp_pending_exception) = MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_kbd_exception));
py/reload.c:    MP_STATE_VM(mp_pending_exception) = MP_OBJ_FROM_PTR(&MP_STATE_VM(mp_reload_exception));

I think this is important in understanding whether the correct solution is to suppress the exception here, rather than determining why it is not handled in mp_hal_stdin_rx_chr.

@DavePutz
Copy link
Collaborator Author

Tests show that the pending exception is, in fact, an mp_kbd_exception. The code quoted is in mp_hal_delay_ms(), and there are no
calls to sleep() to hit in the example test. Interestingly, issuing a time.sleep(1) from the serial REPL prompt does clear the exception and the screen display resumes. I don't see any code in mp_hal_stdin_rx_chr() that looks like it would be clearing the exception.

@jepler
Copy link

jepler commented Aug 23, 2020

I appear to have pasted alternate-universe contents of mp_hal_stdin_rx_chr. I apologize for the confusion.

Specifically, I appear to have been referring to a branch I haven't pushed, which was intended to fix the problem that during a call to input() or stdin.read(), ctrl-c did not raise a KeyboardInterrupt until some other char is pressed. I wrote the code I pasted and haven't gotten around to testing it.

@@ -473,6 +473,9 @@ void readline_init(vstr_t *line, const char *prompt) {
#if MICROPY_REPL_AUTO_INDENT
readline_auto_indent();
#endif
// make sure pending exceptions are cleared
MP_STATE_VM(mp_pending_exception) = MP_OBJ_NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'd do this here. The VM should be handling every pending exception to print the traceback or clear it silently. Is there an exit from somewhere that could clear it instead?

@DavePutz
Copy link
Collaborator Author

Fixed by PR #3318. Closing.

@DavePutz DavePutz closed this Aug 25, 2020
@DavePutz DavePutz deleted the issue2689 branch December 30, 2020 19:35
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

Successfully merging this pull request may close these issues.

displayio REPL output on LCD can stop updating after a control-c on long output
4 participants