-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Improve usb.core.Device exception handling and reporting #9672
base: main
Are you sure you want to change the base?
Conversation
shared-module/usb/core/Device.c had several spots where it would raise a MicroPython exception with a NULL argument. That made it very difficult for CircuitPython code to attempt to recover from fault conditions caused by quirky USB device behavior. This adds short error strings to distinguish the different types of faults.
This commit makes sure that functions in usb.core.Device return as soon as they raise a MicroPython exception rather than continuing on with whatever code would normally run. This will hopefully help to avoid things like dereferencing null pointers.
This is the result of a `pre-commit run --all-files` to fix the translation file changes which were causing a CI check fail. I'm not convinced this change is a good idea. I would prefer to set USBError.errno, if possible, but I don't know how to do that.
I added a fix for the failing pre-commit check (translation file needed changes). I'm not convinced that my approach here of adding error strings and updating the translation file is the right way to solve the problem of distinguishing types of USB errors. If there were a way to set USBError.errno, I would prefer to do that. But, I don't know if or how that would be possible. |
@samblenny Did you just do |
I'm new to this. When I looked around for possible ways of invoking the translation pre-commit hook (skipped normally), the best looking documentation option I found was to run |
I tried running |
Probably what you did was equivalent. But when pre-commit complains, just run |
tannewt, I know you're pressed for time at the moment and probably don't have much bandwidth to discuss this. If you have any concerns about the error messages, I'd be happy to add a commit putting them back like they were before (leaving the just the changes to fix the null pointer dereferences). |
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.
Sorry for not getting back to you! Yes, I'd prefer to back out the error changes. The new messages aren't very actionable for users. It's fine for debugging but I wouldn't leave them. The cost of leaving them is translation and firmware size.
ok. sounds good. will try to get you a new commit soon. |
This puts the error messages back like they were, leaving the null pointer dereference fixes in place.
Just pushed a commit to remove the error message changes. Now the remaining changes are limited exclusively to avoiding null pointer dereferences. |
This PR includes a fix for the null pointer dereference from issue #9670 and adds short error message strings to help distinguish between different types of usb.core.USBError exceptions.
I understand that adding unique strings to the core is generally problematic, but I don't know how to avoid it in this case. For writing USB device drivers, it's very helpful to be able to identify the specific causes of the different types of exceptions that get grouped together under USBError. If there were a way to set the value of USBError.errno, that would be ideal. But, my understanding is that errno is generally not supported in MicroPython. Maybe I'm wrong about that?
In my test builds, these changes add about 132 bytes to the used flash firmware space for
BOARD=adafruit_feather_esp32s3_tft
. Shortening the "Pipe error" and "No configuration set" messages saved about 16 bytes. When I experimented with using single character strings vs. 4-5 character strings, it didn't seem to save any additional space.