esp-rs / esp-idf-sys

Bindings for ESP-IDF (Espressif's IoT Development Framework)
Apache License 2.0
240 stars 114 forks source link

Time for `espidf_time32`? #280

Open ivmarkov opened 3 months ago

ivmarkov commented 3 months ago

Background

Currently, when compiling for ESP IDF 5+, user needs RUSTFLAGS="--cfg espidf_time64 in their environment. This is because ESP IDF 5+ migrated to a new GCC toolchain that defines the time(val?) type as 64 bit type, whereas in ESP IDF < 5 it is 32 bit. ... and since Rust's libc (and by extension - Rust STD) is just a bunch of hard-coded type definitions, completely unaware of esp-idf-sys's bindgen superpowers, it needs to be explicitly instructed whether it should treat timeval as 64 bit or 32 bit.

Proposal

With ESP IDF 4 getting out of maintenance by middle of this year, I suggest to switch - in a backward incompatible way - how libc is compiled. Namely:

Timing to introduce this change? Might depend on which option is chosen of course.

@MabezDev @jessebraham @bjoernQ @Vollbrecht @igrr

bjoernQ commented 3 months ago

t.b.h. I almost never use Rust-std on ESP-IDF 🙈 so my opinion probably shouldn't weight much but I think option A but probably not before July

MabezDev commented 3 months ago

I think option A is probably the right call, if users are using IDF versions that are EOL, they need to upgrade. In hindsight, we probably should have had an espidf_time32 in the first place, but we can keep that in mind for future breaking changes in esp-idf versions.

Regarding the timing, it might be a bit hard to time precisely as we don't have control on a) libc merging b) libc upgrades in rustc. Maybe we get the ball rolling fast, after announcing (in esp-rs? anywhere else?) the plan to only support 5.0 in the future. Users of 4.4 can still use an old compiler (I guess 1.76 might be last supported) to build their 4.4 projects until they upgrade.

ivmarkov commented 3 months ago

I think option A is probably the right call, if users are using IDF versions that are EOL, they need to upgrade. In hindsight, we probably should have had an espidf_time32 in the first place, but we can keep that in mind for future breaking changes in esp-idf versions.

I can agree to that. Though removing the espidf_time64 flag from upstream libc is just as much effort as removing this flag and adding espidf_time32 for 4.4. But you are probably right - why bother? Or maybe...? You know, just to be on the safe side if anybody is using this in production?

MabezDev commented 2 months ago

You're right, if we're already touching that code, we may as well add the time32 flag. It's better to have the work around there than not at all!

You know, just to be on the safe side if anybody is using this in production?

If they are, I would hope that they are taking care of upgrading there esp-idf version before it's EoL or contacting Espressif for an extended support period :D, but again adding the time32 flag is probably a good idea.