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

Compile error for ESP32C3 #480

Closed
justind000 opened this issue Apr 14, 2022 · 8 comments
Closed

Compile error for ESP32C3 #480

justind000 opened this issue Apr 14, 2022 · 8 comments
Labels
fixed The issue has been resolved

Comments

@justind000
Copy link

With the most recent ESP32 core (2.0.3-RC1), there are compile errors when compiling for ESP32C3. I haven't tried with different core versions.

Full error log here
Relevant error is AutoConnect.h:94:12: error: call of overloaded 'IPAddress(unsigned int)' is ambiguous dns2(0U) {}

A quick search shows this error has come up for another library as well with no upstream fix yet.

Fix is to change AutoConnect.h lines 90-94 and 134-138 by adding a typecast.
staip(0U), staGateway(0U), staNetmask(0U), dns1(0U), dns2(0U) {}
to
staip((uint32_t)0U), staGateway((uint32_t)0U), staNetmask((uint32_t)0U), dns1((uint32_t)0U), dns2((uint32_t)0U) {}

@Hieromon
Copy link
Owner

@justind000
I'm not sure about this one... So the configuration of the SDK for the ESP32C3 is different. It is for risc, not xtensa gcc. Well, of course.

I will need to look into this very carefully. Is it a simple type inference order difference or are there other reasons?
Anyway, thanks for inform to me know I'll post when I get some clarification.

@justind000
Copy link
Author

My guess was that the IPAddress constructor definition changed somewhere; adding or dropping some constructor that made passing 0U ambiguous to the compiler. I didn't compare previous/past versions or core files to confirm.

@Hieromon
Copy link
Owner

Hieromon commented Apr 14, 2022

@justind000

My guess was that the IPAddress constructor definition changed somewhere

I don't think so; in 2.0.3-rc1, there are no changes in the core IPAddress.h and IPAddress.cpp. I have confirmed that before mentioning my previous comments. The problem is the interpretation of literal suffixes and type inference by gcc. The suffixes u and U are interpreted as unsigned int, unsgineg long int, or unsigned long long int in decimal bases, it's not the uint32_t.

On the other hand, there is no constructor originally defined for the IPAddress class that takes only one unsigned int argument. Only const uint8_t* and uint32_t are compatible with constructor just one argument. It has remained unchanged for a period of several years.
https://github.com/espressif/arduino-esp32/blob/6cfe4613e4b4846e1ab08c7f78b7ea241f52c7da/cores/esp32/IPAddress.h#L50-#L51

In other words, it is not appropriate to call the constructor with a 0U argument if we are faithful to the IPAddress class declaration. In that sense, your mention of casting to uint32_t is correct.
However, since uint32_t is a machine architecture-dependent type, it must be derived to unsigned int to conform to the bit width of CPU registers. The role of the SDK is to fill this gap. Incidentally, the SDK for espressif-xtensa gcc is that uint32_t is equivalent to unsigned int. As a result, the declaration IPAddress foo(0U); is correct.
If you still maintain the esp32-arduino 2.0.2 environment, you will find the following definitions in C:\Users\[USERNAME]\AppData\Local\Arduino15\packages\esp32\tools\xtensa-esp32-elf-gcc\gcc8_4_0-esp-2021r2\xtensa-esp32-elf\sys-include\machine\_default_types.h::

typedef unsigned int __uint32_t;

And __uint32_t will be deployed to uint32_t in C:\Users\[USERNAME]\AppData\Local\Arduino15\packages\esp32\tools\xtensa-esp32-elf-gcc\gcc8_4_0-esp-2021r2\xtensa-esp32-elf\include\sys\_stdint.h:

typedef __uint32_t uint32_t ;

This deployment by Espressif SDK follows the same approach as up to ESP32-S2. However, they have changed to a different deployment method to support ESP-C3, even though ESP-C3 also has the same RISC architecture as ESP-S2. ESP-IDF4.4 is accompanied by these breaking changes. It is at the expense of backward compatibility.

I suspect that the equivalence of uint32_t and unsigned int is partially lost in the ESP32-C3 compilation environment. I am still investigating whether this is limited to IPAddress class definitions or whether it includes spillover to others.

You have identified a similar issue in the ESPAyncWebServer library. And PR to fix, which is a cast to uint32_t, has not yet been merged. So I suspect that things are not so simple. But maybe it is a simple matter of (uint32_t) only. I still can't say for sure at this point.

@justind000
Copy link
Author

Perhaps (uint32_t) will just have to be a crude workaround of Espressif's "sort it out later" attitude they have taken to their SDK.

@Hieromon
Copy link
Owner

Hieromon commented Apr 15, 2022

@justind000 I deployed the ESP-IDF tool chain to explore the source of this phenomenon. As I thought, the type alias in the ESP-IDF RISC-V based tool chain is different with int332_t, uint32_t. And I don't think this problem can be solved with each individual casting. I would fear the possibility of unintended consequences of modifying only some of the types scattered throughout the library code.

type alias base type on xtensa-esp32 base type on riscv32-esp bit width
int8_t signed char signed char 7+1
uint8_t unsigned char unsigned char 8
int16_t signed short signed short 15+1
uint16_t unsigned short unsigned short 16
int32_t signed int signed long 31+1
uint32_t unsigned int unsigned long 32
int64_t signed long long signed long long 63+1
uint64_t unsigned long long unsigned long long 64

During the course of the research, I found out the approach esp-idf has adopted in supporting ESP32-C3. Their priority was to ensure the continued compatibility of the RISC-V tool-chain. That makes sense.
espressif/esp-idf#6906 (comment)

Many Arduino libraries assume that uint32_t will be assigned to an unsigned int. That is never a good practice (including AutoConnect). However, the most stable solution may not be to wait for individual library fixes. To override the type with a tweak that is only available at Arudino build time.
I'll consider a few more options to see what the best solution would be.

@justind000
Copy link
Author

Well I see it is a bit more involved than a changed definition. I will leave it up to you to decide the best course for your library and keep some note for next time I get a -C3 compile error.

@Hieromon
Copy link
Owner

@justind000 In the end, it seems that the alias definition of uint32_t in the riscv-32 toolchain will still settle on unsigned long.
I tried to force the toolchain to rewrite the default typedef header it generates at build time, but was met with a warning that the intended unsigned long type conversion was lost. This is evidence of scattered code other than AutoConnect, which assumes that unsigned int and unsigned long have the same bit width.

So, I decided to rely on forced type matching by casting for the 32-bit signature of IPAddress. After all, your outlook was the least risky way. I will release AutoConnect v1.3.5 soon, which will include ESP32-C3 support. You can pre-verify it in the enhance/v135 branch.

Thank you for your contribution.

@justind000
Copy link
Author

I guess this is a case of technically-correct versus popularly-correct.

@Hieromon Hieromon mentioned this issue Jul 2, 2022
@Hieromon Hieromon added the fixed The issue has been resolved label Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed The issue has been resolved
Projects
None yet
Development

No branches or pull requests

2 participants