esp32: consolidate check_esp_err functions - #5850

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 check_esp_err. But I'm not sure about defining EOTHER (which seems non-standard, can you link to a use of this name?), or about allocating a string for each OSError (it takes memory, fragments the heap).

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).

Thanks, it's good to consolidate the use of check_esp_err. But I'm not sure about defining EOTHER (which seems non-standard, can you link to a use of this name?),

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.

For standard errors it would be better to translate IDF codes to errno values, eg using a look-up table.

There are a couple in the PR, I believe I could add 2-3 more tops.

[...]or about allocating a string for each OSError (it takes memory, fragments the heap).
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).

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 uos.strerror function that applies some heuristics to determine whether it's a std error, an esp-idf error, or an mbedtls error and returns an appropriate string. Only bummer is that moduos is port-specific, i.e., lots of duplicated code.

I didn't appreciate that CPython actually defines OSError() to take multiple arguments, the second being an OS-supplied error string (first is errno). So that would be a good standard to follow, if a port has the code space (and in this case the error strings are already there).

So how about raising exceptions as OSError(errno, "error string"), with the errno being positive if it's a standard one, and negative if it's an IDF-specific one. The error string is always found using esp_err_to_name() (which is O(N)... so slow...). The reason to use negative IDF numbers is so that programs which catch and inspect the exception can act on this error code (instead of it being a generic EOTHER/ENOANO and having to inspect the error message).

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 OSError that is raised to the user, and the point with error strings is to improve user experience, it's not for use by programs.

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 _esp_exceptions in modnetwork.c can be completely replaced with the new function from here, which would clean it up even more.

OK, here we go for another round...

• I removed the EOTHER stuff and check_esp_err now uses the negative ESP error code
• Instead of static alloc I went for m_new_obj_maybe, this way if there's an emergency exception buffer the user gets the error code without string, which seems OK to me, and I really don't think that the fact that in general these errors involve two allocations instead of one is going to be noticed anywhere
• I didn't see more error codes to map to posix and in fact I propose to remove the existing three mappings because I think they end up producing more confusion than anything else. Getting an ESP-IDF error makes it clear where it comes from. Maybe we can't remove those mappings because someone's code will break?
• I also didn't deal with _esp_exceptions in modnetwork.c because it would change the error messages quite a bit. E.g. "Wifi Invalid Password" would become (-12299, "ESP_ERR_WIFI_PASSWORD"). I'd be OK with that and am happy to make the change, but others may not be thrilled.

Just to be explicit: this is ready for review.

Thanks, this looks pretty good now.

Instead of static alloc I went for m_new_obj_maybe, this way if there's an emergency exception buffer the user gets the error code without string, which seems OK to me,

Yes that's a nice compromise. The only remaining thing is that creating an OSError will compute a hash via qstr_compute_hash(), which is O(length of string). But I think this is acceptable (any error message with a string will anyway compute the hash).

I propose to remove the existing three mappings because I think they end up producing more confusion than anything else.

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).

Maybe we can't remove those mappings because someone's code will break?

I don't know if code would break, it might... but this would be better suited for a separate PR (if anything).

I also didn't deal with _esp_exceptions in modnetwork.c because it would change the error messages quite a bit. E.g. "Wifi Invalid Password" would become (-12299, "ESP_ERR_WIFI_PASSWORD"). I'd be OK with that and am happy to make the change, but others may not be thrilled.

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 check_esp_err() at the top of mphalport.c (because it's more fundamental than the other functions already in that file).