espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.19k stars 7.17k forks source link

Add errno.h error codes reference to IDF programming guide (IDFGH-6926) #8546

Open maBarabas opened 2 years ago

maBarabas commented 2 years ago

Environment

Problem Description

Functions under the CONFIG_VFS_SUPPORT_DIR fail at runtime if this option is not turned on.

Expected Behavior

When CONFIG_VFS_SUPPORT_DIR is off, compilation fail if these functions are used or a runtime assert is triggered when they are called, with comments explaining how to fix the issue.

Actual Behavior

The calls to these functions fail at runtime, and errno is set.

I have tested remove, this sets errno to ENOTSOCK. Other functions that are not working are stat and rename. These are the only 3 that I've tested, probably more of them are affected.

opendir causes a linker error, as expected. This is the only way I was able to find that the fix is enabling CONFIG_VFS_SUPPORT_DIR .

Steps to reproduce

  1. Open examples/storage/spiffs
  2. Disable CONFIG_VFS_SUPPORT_DIR
  3. Run the project on a devkit
  4. rename fails at runtime

Debug Logs

I (0) cpu_start: Starting scheduler on APP CPU.
I (330) example: Initializing SPIFFS
I (440) example: Partition size: total: 896321, used: 0
I (440) example: Opening file
I (540) example: File written
I (540) example: Renaming file
E (540) example: Rename failed

Other items if possible

maBarabas commented 2 years ago

files.zip

igrr commented 2 years ago

Hi @maBarabas, failing on these functions at compile or link time would be a nice property, but is actually relatively difficult to achieve. If not provided in IDF, the functions will be found in newlib (libc.a). Short of dropping some object files from libc.a (which is quite error prone), it is not possible to tell linker to not use those functions from newlib and thus produce a linker error.

Regarding run time behavior, in my opinion, returning an error is a better generic approach than asserting. If the function returns an error, the caller can always check the return value and assert or abort. If the function asserts, the caller has no chance to recover.

maBarabas commented 2 years ago

My main complaint is that it's hard to come across the fix after receiving ENOTSOCK from remove. Please consider documenting this behaviour somewhere.

igrr commented 2 years ago

I see, ENOTSOCK is indeed quite unexpected! We'll take a look why this happens.

Can you describe how you went about disabling CONFIG_VFS_SUPPORT_DIR and then using filesystem functions? The help text for that option seems to say that directory related functions become unavailable when this option is disabled...

igrr commented 2 years ago

Another question, where/how did you look up the ENOTSOCK name corresponding to errno value set by remove?

Newlib doesn't define ENOTSOCK at all, as far as I can tell (sys/errno.h) Edit: my bad, i misread the ifdefs... It is defined.

maBarabas commented 2 years ago

I just greped in esp-idf for the decimal value which was 88.

components/lwip/lwip/src/include/lwip/errno.h
137:#define  ENOTSOCK        88  /* Socket operation on non-socket */

The value from your link makes a bit more sense

#define ENOSYS 88   /* Function not implemented */
igrr commented 2 years ago

I see now! Yes, ENOSYS is what is usually returned in case a function is just a stub and is not implemented.

Do you think having a list of errno values in IDF Programming Guide would have helped you find the right name? That's something we can do.

We can also extend the documentation of CONFIG_VFS_SUPPORT_DIR and mention ENOSYS as the expected errno value.

maBarabas commented 2 years ago

Can you describe how you went about disabling CONFIG_VFS_SUPPORT_DIR and then using filesystem functions? The help text for that option seems to say that directory related functions become unavailable when this option is disabled...

The option has been disabled for a while in our project. The only use of the remove function is in an obscure feature, and the failure was not picked up in our testing until recently. The product functions correctly in most cases, even if this function doesn't work.

Do you think having a list of errno values in IDF Programming Guide would have helped you find the right name? That's something we can do.

We can also extend the documentation of CONFIG_VFS_SUPPORT_DIR and mention ENOSYS as the expected errno value.

That sounds great.

Could you clarify if the lwip component's errno.h is used at all? I've found someone else that used that file to look up the errno in this issue. It looks like we both made the exact same mistake of not enabling a feature, then interpreting errno 88 (ENOSYS) as ENOTSOCK.

igrr commented 2 years ago

lwip component's errno.h is not used in IDF. It is provided by lwip project for systems which don't come with their own errno.h file. It only gets used if LWIP_PROVIDE_ERRNO is defined, which isn't the case in ESP-IDF. What makes the issue harder to spot is that the real errno.h file which does get used is not part of ESP-IDF repository — it comes from Newlib library and is included in the cross-compiler toolchain.

cc @david-cermak — not sure if would consider adding a small patch to esp-lwip project, adding some notice at the top of lwip's errno.h file like /* NOTICE for ESP-IDF users: this is not the errno.h file used by ESP-IDF. Please see (link) instead. */? Or simply removing the file in our fork to avoid confusion, given that it's not used?..

KaeLL commented 2 years ago

Do you think having a list of errno values in IDF Programming Guide would have helped you find the right name? That's something we can do.

I rather have this, and a note saying lwip's errno is unused there. I think it's more search engine friendly and more likely for people to find the right answer, similar to mbedtls errors in ssl.h

david-cermak commented 5 months ago

Will add errno.h codes to the esp32 docs. Currently it only contains links to newlib headers: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/lwip.html#socket-error-reason-code

The idea it so move these error codes to the API reference, similar to esp_err_t values:

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/error-codes.html

jcecconi commented 3 days ago

Hi, has anybody solved this issue? I have the same problem... When I try to connect to an unreachable device with ESP-IDF v5.2, the connect function returns -1, with errno set to 113 (which equals ECONNABORTED), while EHOSTUNREACH is equal to 118."

Let me know if you need any further adjustments!