Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

tve
Copy link
Contributor

@tve tve commented Mar 31, 2020

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.

@tve tve force-pushed the esp32-check-err branch from b3f9a46 to 500e404 Compare April 2, 2020 07:00
@tve
Copy link
Contributor Author

tve commented Apr 2, 2020

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

@dpgeorge
Copy link
Member

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

@tve
Copy link
Contributor Author

tve commented Apr 16, 2020

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

@tve
Copy link
Contributor Author

tve commented Apr 16, 2020

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.

@tve tve force-pushed the esp32-check-err branch from 500e404 to 5427e0f Compare April 17, 2020 01:21
@tve tve force-pushed the esp32-check-err branch from 5427e0f to 54ba288 Compare April 17, 2020 01:22
@dpgeorge
Copy link
Member

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.

@tve
Copy link
Contributor Author

tve commented Apr 19, 2020

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.

@tve
Copy link
Contributor Author

tve commented Apr 22, 2020

Just to be explicit: this is ready for review.

@dpgeorge
Copy link
Member

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.

@tve
Copy link
Contributor Author

tve commented Apr 22, 2020

Sounds good. I didn't pick-up any to-do item for me.

@dpgeorge
Copy link
Member

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

@dpgeorge dpgeorge closed this Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants