Closed embediver closed 1 year ago
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.)
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
?
@igrr ^^^
@ivmarkov I'm running into this as well. Is there a corresponding issue open against esp-rs/esp-idf-svc for this?
No but there is this fix: https://github.com/esp-rs/esp-idf-svc/commit/42e60e8c3fcca670632b9653c790fd8efe475aac
Can you test with latest esp-idf-hal
(from master
)?
@igrr I think we should close this. If you could only confirm my question here.
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.
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.
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.
Is there a Issue or something other to track this in esp-idf-sys
?
Then I would close this issue here.
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.
Answers checklist.
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
inwifi_scan_config_t
breaks the build and doesn't seem to be semver compatible.Steps to reproduce.
$ cargo build
wifi_scan_config_t
Build or installation Logs.
More Information.
No response