Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

samblenny
Copy link

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.

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.
@samblenny
Copy link
Author

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.

@dhalbert
Copy link
Collaborator

@samblenny Did you just do make translate at the top level? That updates circuitpython.pot programmatically. You should not need to edit circuitpython.pot by hand.

@samblenny
Copy link
Author

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 pre-commit run --all-files as mentioned in https://learn.adafruit.com/improve-your-code-with-pylint/check-your-code-with-pre-commit. So, that's what I did.

@samblenny
Copy link
Author

I tried running make translate just now, and it didn't appear to make any additional changes.

@dhalbert
Copy link
Collaborator

I tried running make translate just now, and it didn't appear to make any additional changes.

Probably what you did was equivalent. But when pre-commit complains, just run make translate and commit those changes. We should put that in a message somewhere (though I thought that was somwhere already).

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.

2 participants