lora-gateway / esxp1302

An 8-channel ESP32 LoRa Gateway based on SX1302
Other
50 stars 26 forks source link

upgrade to esp-idf v5.x branch #17

Open dennis4lora opened 5 months ago

dennis4lora commented 5 months ago

To upgrade to esp-idf v5.x branch, it's not difficult. However, there is a small change on the GCC side makes esxp1302 containing lots of lines needing to be fixed accordingly.

Per the migration guide: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/migration-guides/release-5.x/5.0/gcc.html#int32-t-and-uint32-t-for-xtensa-compiler

For example, we have two ways to display the value of uint32_t value as:

- printf("value = %u\n", value);
+ printf("value = %" PRIu32 "\n",  value);

or simply as:

- printf("value = %u\n", value);
+ printf("value = %lu\n",  value);

I'm not sure which is the better way to choose:

So: which way do you prefer? should we keep v4.x support? your comments are welcome.

BTW: If you are interested, this link talked a lot of this issue.

danielkucera commented 5 months ago

I am already testing the updated version, this is my branch: https://github.com/lora-gateway/esxp1302/compare/main...danielkucera:esxp1302:idf-update

Mine only ignores the warnings for format because of:

build_unflags = -Werror=all

which is the default for platformio. Is is the same for plain esp-idf?

But to your question - I don't see a particular reason to keep the old version supported.

dennis4lora commented 5 months ago

which is the default for platformio. Is is the same for plain esp-idf?

I'm afraid not. Someone suggested that this can be done by adding component_compile_options(-Wno-error=format= -Wno-format) to CMakeLists.txt, but I can't make it correct to pass the compile step.

Anyway, I would like to fix it instead of bypass it. And I agree with your opinion that supporting v4.x isn't useful. So I'll follow the 2nd way.

dennis4lora commented 5 months ago

After changed some %u to %lu locally, I realized there are in fact two ways to do it (still takes uint32_t for example):

uint32_t used to be interpreted as unsigned int and now becomes unsigned long, so very likely the size extends from 32-bit to 64-bit. It's no harm for the correctness, but I'm afraid it takes more space and not be used since 32-bit is already enough in the original intention.

So change uint32_t to unsigned int seems more reasonable.

@danielkucera: What's your opinion? Thanks.

danielkucera commented 5 months ago

good find, this is the best option IMO

dennis4lora commented 5 months ago

Great!!

And I'm sure:

 sed -i 's/uint32_t/unsigned int/g' *.c *.h 

is much quicker than manually working on the 1st way. :rofl:

dennis4lora commented 5 months ago

I just checked the spec, and found that for both Xtensa (ESP32 based) and RISCV32 (ESP32C3 based), int takes 4 bytes, long also takes 4 bytes.

I also changed all uint32_t to unsigned int and compiled using PlatformIO, the produced firmware sizes match exactly before this change for both esp32 and esp32c3.

So there is no harm (but also no gain) for this change, but it's easy and needed for esp-idf v5.x branch, so let's do it this way.

danielkucera commented 5 months ago

Perfect.

dennis4lora commented 4 months ago

The updates has been done in branch esp-idf-v5.x.

I've tested the build with esp-idf v5.2, and espressif32@6.7.0, both OK, but the produced size is more than 10% bigger. :frowning_face: