-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
runtime/abort: Add support for vm_abort in standard runtime #14419
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14419 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 161 161
Lines 21204 21204
=======================================
Hits 20864 20864
Misses 340 340 ☔ View full report in Codecov by Sentry. |
Code size report:
|
acf431b
to
2e3e0c9
Compare
I see that some checks/ports are failing because the function |
shared/runtime/pyexec.c
Outdated
if (mp_obj_is_subclass_fast(MP_OBJ_FROM_PTR(((mp_obj_base_t *)nlr.ret_val)->type), MP_OBJ_FROM_PTR(&mp_type_SystemExit))) { | ||
#ifdef MICROPY_ENABLE_VM_ABORT | ||
if (nlr.ret_val == NULL) { // abort | ||
ret = 255; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to define a macro for this value and document it.
Unhanded exceptions still return ret = 0 though, so why is this different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this should probably be followed by a goto
that jumps to a label just before MICROPY_BOARD_AFTER_PYTHON_EXEC()
at the end of the function so that no other MicroPython runtime stuff runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to define a macro for this value and document it.
Ok. Where is a good place to define it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unhanded exceptions still return ret = 0 though, so why is this different?
Is this on purpose? I consider it a bug. In my port/usecase I want the exceptions to throw a non-success exit code and I was planning opening an additional PR later. As well as exposing the exit code from system exit, so os.exit(1)
in python actually exits with the exit code according to the argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this should probably be followed by a goto that jumps to a label just before MICROPY_BOARD_AFTER_PYTHON_EXEC() at the end of the function so that no other MicroPython runtime stuff runs.
Is it wrong to run the repl stuff? And the mp_hal_stdout_tx_strn
function. Those are the only two things that are potentially run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My use case is this:
I am building a device which is programmable by micro python by the end users. The device/firmware provides an additional "OS" layer - display/UI/menu/settings/file system/... and API for the user program. When the user program is being executed, it is rendered on the display, sending stdout to the display and other info. When the program exits, I want to be able to show a screen "Finished successfully" or "Crashed" (triggered by an unhandled exception or os.exit(non-zero) or "Killed/aborted". The abort is triggered from the device itself/button combination. I need the abort to be instant (as perceived by the user). Which means, there is some time to finish some instructions, flush std out, ...; However no more python instructions should be run (so the program wont be able to "prevent" being killed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, it makes sense to me to return exit codes on the exit. Not just always zero. I am not sure if there is a use case, where you would want to have always zero exit code. If the host fw/vm doesn't care about it, it can just ignore it. Right?
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am building a device which is programmable by micro python by the end users.
We did something similar with Pybricks. What we ended up doing was just raise SystemExit
to stop user programs. We only use the abort if we are powering off the device. (If a user manages to somehow always catch the SystemExit
, they will have to power off the device, but this doesn't really seem to happen that we've notified).
Here is the source if you are interested: https://github.com/pybricks/pybricks-micropython/blob/79a0e010ac36fb64593d5ef5b213b64f1a03985b/bricks/_common/micropython.c#L229
If the host fw/vm doesn't care about it, it can just ignore it. Right?
Makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Should one be preferred over the other? Or is this approach OK as well?
Is there anything blocking this merge at the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will have to wait for @dpgeorge to chime in here (sometimes it can take a while before he has time).
// at the moment, the value of SystemExit is unused | ||
ret = pyexec_system_exit; | ||
} else { | ||
} else { // other exception | ||
mp_obj_print_exception(&mp_plat_print, MP_OBJ_FROM_PTR(nlr.ret_val)); | ||
ret = 0; | ||
} |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
f3015ad
to
aaa8d3e
Compare
deb4b0e
to
5fac003
Compare
Signed-off-by: John Smith <jsmith@jsmith.cz>
5fac003
to
efe6c3e
Compare
Ok, in the meantime, I have implemented the functionality, which sets exit code according to the SystemExit exception. It is set up such, that it is reverse compatible (the current exit codes won't change for anyone, unless they decide to use this by setting the appropriate exit code values) |
a977bfb
to
403f9b2
Compare
Signed-off-by: John Smith <jsmith@jsmith.cz>
403f9b2
to
b1cca67
Compare
There is a unit test failing, something with thread locking, but I am not sure what the output of the test means or how is it related to my code. Can you please advice/help? Thanks |
That is a flaky test that fails randomly, so nothing to worry about. |
Ok, thanks |
What is the current status? All is ready and we are waiting for @dpgeorge? Is there something that I can do in the meantime, to prepare or fix something, to speed things up? |
Hi. Is there any update on this? Can we merge this? |
Add abort setup code (
nlr_set_abort
) to standard runtime. This makes the standard runtime respond to abort signal without any further modifications.When aborted, the program exits with 255 exit code, to differentiate from a normal shutdown.