-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
esp32: consolidate check_esp_err functions #5850
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
Conversation
Currently CI fails on MacOS on basics/dict_failed, which is completely unrelated to this PR. I don't think there's anything I can do about this... (I'm pretty sure it's rebased right off of current master.) |
Thanks, it's good to consolidate the use of For standard errors it would be better to translate IDF codes to errno values, eg using a look-up table. For IDF specific error codes that don't map sensibly to any errno code, it would be simpler to just pass through the IDF code, eg as a negative value. That way it can be programmatically checked (rather than testing against a string in code). |
About EOTHER, I noticed ENOANO in the posix list and googled that: https://unix.stackexchange.com/questions/167368/what-is-enoano-no-anode-intended-to-be-used-for#167460; tl-dr: <<For a very long time, it wasn't used anywhere; it's since then been used now and then in some drivers as “I didn't know what error code to use”.>> I hesitated between using ENOANO literally or that value with a different name. Either would be fine with me.
There are a couple in the PR, I believe I could add 2-3 more tops.
I get what you mean but it's a really poor user-experience! And it's not like the esp-idf errors get generated very frequently as a normal matter of operation like EAGAIN or EINTR. I guess I'll make it a default-off MP_ config so users that want sane error messages can turn it on... |
Here's another proposal. It seems there's an undocumented convention of using negative error codes for "port-specific" errors, e.g. esp-idf and mbedtls. How about I add a |
I didn't appreciate that CPython actually defines So how about raising exceptions as And to prevent heap allocation for the string object it can have a static one, like: static mp_obj_str_t idf_exc_str_obj; which is reused for each new error. This is unlikely to cause any issues because in most cases it's usually just a single Note that the exception creation code is finely tuned so that it can still work even without any heap memory available (or a locked heap) so it makes sense to preserve that property here. Then probably |
OK, here we go for another round...
|
Just to be explicit: this is ready for review. |
Thanks, this looks pretty good now.
Yes that's a nice compromise. The only remaining thing is that creating an OSError will compute a hash via
I take your point that for consistency they should not be mapped. But OTOH OS-error can (or at least should) only be raised for errors generated by the underlying "operating system", so if you get one you know where it came from (the IDF).
I don't know if code would break, it might... but this would be better suited for a separate PR (if anything).
I'd be +1 to change these to use the new exception function in this PR, for consistency, and to remove all those strings. But, again, a topic for another PR. |
Sounds good. I didn't pick-up any to-do item for me. |
Now merged in 1ae7e0e, with a minor change to place |
This PR consolidates a number of
check_esp_err
functions that check whether an esp-idf return code is OK and raises an exception if not. The exception raised is an OSError with a new EOTHER error code as first arg and the esp-idf error string as second arg. EOTHER=55 which is EANODE in posix, which seems to be popular as "some other error" code (google it).This PR also fixes esp32 Partition set_boot to use check_esp_err in order and uses it for a unit test.