bugfix in v4.4 adds semver incompatible change to wifi_scan_config_t (IDFGH-10699)

[embediver]

Answers checklist.

• I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
• I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
• I have searched the issue tracker for a similar issue and not found a similar issue.

IDF version.

v4.4

Operating System used.

Linux

How did you build your project?

Other (please specify in More Information)

If you are using Windows, please specify command line type.

None

What is the expected behavior?

(Building esp-idf with rust esp-idf-sys and esp-idf-svc in a rust project, see details below.)

Expected behavior: Building the esp-idf-sys and esp-idf-svc libary successfully.

What is the actual behavior?

Build fails because the last bugfix (4fc8964) on v4.4 introduced a breaking change.

The added field home_chan_dwell_time in wifi_scan_config_t breaks the build and doesn't seem to be semver compatible.

Steps to reproduce.

1. Create a rust project from the official esp-idf-template
   • esp32c3 mcu
   • default settings (esp-idf v4.4, cargo)
2. hit $ cargo build
3. Build fails because a initializer in the rust esp-idf-svc isn't aware of the added field in the wifi_scan_config_t

Build or installation Logs.

Compiling esp-idf-svc v0.46.0
error[E0063]: missing field `home_chan_dwell_time` in initializer of `esp_idf_sys::wifi_scan_config_t`
  --> /home/marvin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-idf-svc-0.46.0/src/wifi.rs:93:13
   |
93 |             Self {
   |             ^^^^ missing `home_chan_dwell_time`

For more information about this error, try `rustc --explain E0063`.
error: could not compile `esp-idf-svc` (lib) due to previous error

More Information.

No response

[igrr]

Hi @MG-96,
IDF project aims for C source compatibility (described here) in minor and bugfix releases. As mentioned in that page, we do allow adding new structure members or changing the order of members. This is not a breaking change for C code since we require the configuration structures passed to IDF APIs to be zero-initialized. Another case may be with a change from an exported C function to an inline C function or a function-like preprocessor macro — a backwards compatible change for C code.

Unfortunately we can't give stronger compatibility guarantees at the moment, such that would be sufficient for the purpose of stable Rust bindings. The best I can offer is that we could set up a scheduled CI pipeline in https://github.com/esp-rs/esp-idf-sys to get notified of any breakage quickly.

(cc @ivmarkov, please comment if you have any opinion about this.)

[ivmarkov]

If indeed ESP IDF is consistently following the "if you zero-initialize the remaining (or all) fields of a configuration structure - that's OK" rule, then we have a (relatively easy way) to keep the Rust bindings (not esp-idf-sys itself, but user code based on esp-idf-sys as well as the higher-level bindings in esp-idf-hal and esp-idf-svc) from breaking.

Bindgen generates for us a Default implementation/constructor for every ESP IDF struct bound by esp-idf-sys. This constructor happens to initialize all fields of the struct to zero.

What that means is that when the user (or we - in the higher level esp-idf-* crates) initialize ESP IDF structures, we just have to follow this pattern:

let config = esp_idf_sys::wifi_scan_config_t {
    foo: 1,
    bar: true,
    ..Default::default() 
};

... or if I understand you correctly, even this all-zeros initialization

let config: esp_idf_sys::wifi_scan_config_t = Default::default();

... would be correct and would result in ESP IDF picking meaningful defaults for all fields.

The thing is - I just wasn't aware that ESP IDF is following the "0 is OK" rule consistently everywhere.

Specifically for the wifi_scan_config_t structure (where we can't look into the ESP IDF code, as this is the Wifi binary BLOB) - would you confirm that putting zeroes in all its fields is OK? And specifically for the home_chan_dwell_time?

[ivmarkov]

@igrr ^^^

[weberc2]

@ivmarkov I'm running into this as well. Is there a corresponding issue open against esp-rs/esp-idf-svc for this?

[ivmarkov]

No but there is this fix: esp-rs/esp-idf-svc@42e60e8

Can you test with latest esp-idf-hal (from master)?

[ivmarkov]

@igrr I think we should close this. If you could only confirm my question here.

[igrr]

Specifically for the wifi_scan_config_t structure (where we can't look into the ESP IDF code, as this is the Wifi binary BLOB) - would you confirm that putting zeroes in all its fields is OK? And specifically for the home_chan_dwell_time?

I think we always handle newly introduced fields this way, but some existing fields may not support 0 as default value. As a contrived example, esp_timer_create won't be happy if we pass 0 as a value of callback field. Another example might be freq_hz in ledc_timer_config_t.

[embediver]

I was going to suggest marking all structs in esp_idf_sys as non-exhaustive, but unfortunately that forbids constructing them with a StructExpression (including with functional update syntax). (Does Bindgen have functionality to do the non-exhaustive declaration?)

Making it clear, that one always has to do a functional update to initialize structs from esp_idf_sys, or finding another solution to prevent such bugs from occurring would be nice.

Thanks @igrr for clarifying things.

[ivmarkov]

I was going to suggest marking all structs in esp_idf_sys as non-exhaustive, but unfortunately that forbids constructing them with a StructExpression (including with functional update syntax). (Does Bindgen have functionality to do the non-exhaustive declaration?)

Making it clear, that one always has to do a functional update to initialize structs from esp_idf_sys, or finding another solution to prevent such bugs from occurring would be nice.

Thanks @igrr for clarifying things.

My thoughts exactly, as I found out this edge case as well. And bindgen anyway does not seem to support #[non_exhaustive] on anything but rustified enums.

Well, I guess the only thing we can do is put a documentation note in the esp-idf-sys crate that all structs have to be constructed with the functional update syntax.

[embediver]

Is there a Issue or something other to track this in esp-idf-sys?

Then I would close this issue here.

[ivmarkov]

Your original problem is fixed since a month or so in esp-idf-svc. Adding ..Default::default() everywhere I would do incrementally. If you want to do it in one shot, please open PRs against esp-idf-hal and esp-idf-svc, and you can of course open issues in these projects to track this effort.

And yes, I think this one should be closed.