Co-existence of old and legacy driver wrappers in downstream crates

[ivmarkov]

In light of this issue - and if it does not get resolved soon - the scheme below is the only one I could come up with so as to workaround the issue with a minimal effort and a minimal cognitive load to the end user:

Necessary changes to the esp-idf-sys build system

• Change the esp-idf-sys Cargo native build as follows:
  • From "build all components by default" (status quo)...
  • ... to "build all components by default except the new (non-legacy) drivers"
• Find a way to do the same with the PIO build (open topic, TBD - it might be the case that PIO always builds all ESP IDF components! ... and then we cannot really support this scheme for PIO)
• In the esp_idf_components env var and cargo-metadata parameter, introduce a new value which is a "pseudo-component": default
  • If the esp_idf_components env var is not specified, it is assumed to have a value of "default" (see below for the meaning of this value)
  • If the esp_idf_components env var IS specified, it is assumed (as it is right now) to contain a list of ESP IDF components, plus (and this is the new thing) optionally the default value
  • If the default value is contained in esp_idf_components, it is first expanded to ALL ESP IDF components except the ones which designate "new" (non-legacy) drivers
  • We'll have to - unfortunately - implement a hard-coded list of such "new" drivers which we expand over time
  • Getting the list of ALL components for a particular ESP IDF installation is also not trivial. (Currently, we get this list after the ESP IDF build is finished, from a cmakelists.txt file) but now we need it before the ESP IDF build, so that we can expand the default "special" value to a list of components. Re-using the PIO-build hack of "traversing" all directories listed directly under the ESP IDF's components repo dir might work...
  • Once the default special value is expanded and then its expansion is de-duplicated with whatever additional components the user has entered in the esp_idf_components env var, build proceeds as usual (as it is today)

Necessary changes to esp-idf-hal

To comply with the "whole object file" style of linking described in the ESP IDF issue we have to keep the following invariant:

• Assuming esp-idf-hal has a wrapper for both a legacy XYZ driver, and a new XYZ driver, for any build configuration exactly one of these (or none of these) should be built. Where by "built" what we mean is that the other wrapper (or both) should be #ifndef-ed out. I.e. under no circumstances we should generate the code for BOTH Rust driver wrappers. Either one, or the other, or none of them, but not both.
• The above - for esp-idf-hal means that we need to keep the existing #[cfg(esp_idf_comp_XYZ)] conditional for the new driver
• But additionally (new) we should protect the legacy driver wrapper code with #[cfg(all(esp_idf_comp_driver, **not(esp_idf_comp_XYZ)**))] (** emphasis mine)
• This way, the moment the user enables the NEW driver component, the legacy driver wrapper is automatically compiled out

Code in third party crates outside of esp-idf-sys should follow the same invariant so as to deal with the issue.

How would the above ^^^ solve the problem

• The default build of esp-idf-hal does not build any new driver, ergo, no conflicts possible
• If the user enables - say - the new ADC oneshot driver (by e.g. specifying esp_idf_components = default, esp_adc), we would build BOTH the legacy and the new ESP IDF ADC oneshot drivers' C code, but will build only the new ADC oneshot Rust driver wrapper. This way, the legacy ADC C driver - even if present on the linker command line, will be pruned early, including its __attribute__((constructor)) constructor, which is causing us the headaches

Alternative: doing it all with features

Rather than changing the esp-idf-sys build system from above, we can alternatively introduce a new cargo feature - in esp-idf-hal - for each new driver that pops-up in ESP IDF and keep this list of new features growing ad-infinitum (or until the legacy driver is retired in - say - ESP IDF 6 or later). To elaborate:

• New ADC oneshot driver gets a "new-adc-oneshot-driver" feature
• New I2C driver gets a "new-i2c-driver" feature
• ... and so on and so forth

The above features would not be enabled by default.

• In the absence of - say - new-adc-driver feature enabled, esp-idf-hal compiles the legacy ADC oneshot Rust wrapper (i.e. it needs #[cfg(all(not(feature = "new-adc-oneshot-driver"), esp_idf_comp_driver)] - but only if the driver component is enabled of course)
• In the presence of new-adc-driver feature enabled, esp-idf-hal compiles the new ADC oneshot Rust wrapper, and compiles-out the legacy one. The new oneshot Rust wrapper would only be compiled however if both feature ="new-adc-oneshot-driver" and esp_idf_comp_esp_adc is enabled.

Pros:

• Simpler scheme, no changes to the esp-idf-sys build are necessary. Might be a good short-term solution for the upcoming release of the esp-idf-* crates

Cons:

• Duplication: compiling in (or out) a new driver now depends not just on whether the new driver component is enabled, but also on whether the corresponding cargo feature is enabled (i.e. we are duplicating one flag into two flags - one feature flag, and the existing component flag to avoid changes to esp-idf-sys)
• Third-party crates using esp-idf-sys directly should introduce their own similar feature-based scheme and should be carefully applying it, by understanding all the details elaborated here. In contrast, changing the build system of esp-idf-sys by extending with the default pseudo-feature might result in a setup which is a tad easier to explain, and does not result in any new features.

Cons of both approaches

For backwards compatibility, both approaches will keep the legacy drivers "enabled", and the new drivers disabled. Which in the long term is not ideal, as we expect folks to start using the new drivers.

But the current reality is, we have just one new driver - ADC oneshot, with possible another in the works (I2C). Everything else is legacy drivers.

[Vollbrecht]

Thanks for summarizing all the points!

While not the root cause, i still would like to the gating at the bindgeh.h level. One example to do it with features would be to create different versions of bindgen.h and use them depending on what driver features are set on a esp-idf-sys level. Because if we do not create bindgen for old and new driver, its impossible to misuse them in a downstream wrapper since you ever get the new or the old one.

I know that this would not solve the issue with external esp_idf_components that might relight on legacy drivers etc. I only think that could be an extra measure around the things you are proposing.

[ivmarkov]

Thanks for summarizing all the points!

While not the root cause, i still would like to the gating at the bindgeh.h level. One example to do it with features would be to create different versions of bindgen.h and use them depending on what driver features are set on a esp-idf-sys level. Because if we do not create bindgen for old and new driver, its impossible to misuse them in a downstream wrapper since you ever get the new or the old one.

I know that this would not solve the issue with external esp_idf_components that might relight on legacy drivers etc. I only think that could be an extra measure around the things you are proposing.

Yes - as long as you shift the new-driver-XYZ features to the esp-idf-sys level (and then adopt and re-export on esp-idf-hal and esp-idf-svc level) that would work.

This would bind us forever with the "features" approach though.

If we take the lighter-weight approach of "if you use the sys bindings, you are on your own what you do, be careful" (which anyways applies, right), we can remove the features at some point in time.

[Vollbrecht]

Yeah that's why i thought we may could create some "user defined" kconfigs that would translate to rust cfg attributes. That way we don't need "rust features" and we would not need to split the bindgen.h into multiple files. And for example in a potential IDFv6 that has no legacy drivers we could drop them since not defined kconfigs are just ignored. E.g a IDV v6 driver could be included with cfg(any( all(adc_oneshoot, not (legacy_adc)), esp_idf_major >= 6)).

[ivmarkov]

First to elaborate about the features and what I mean that we should be probably minimalistic there:

Assume we just want to release the new crates ASAP, and not wait on the ESP IDF issue to get resolved. Then we can use the "features" approach as a minimum "band-aid" until we have clarity what is the path forward.

• An absolute minimum would be to introduce - today - just one new feature for the only new-style driver we are having: adc-new-driver)
• (Or maybe two such features if I'm omitting another new driver wrapper we might be having; pcnt?)
• We can introduce it/them only in esp-idf-hal (not even in esp-idf-sys for now) and maybe not even propagate them to esp-idf-svc. Thus, if a user really wants to use the new ADC oneshot - she has to depend on esp-idf-hal directly, in addition to esp-idf-svc (even if svc re-exports the hal API)

In one release iteration from now, we might get the default (components-based) solution (whenever we get some time to write it) and we can retire the just-introduced feature. (If we have the default solution, we can actually extend it to the bindings.h header too - it would be trivial and would make sense). Or we might even get a better solution coming from ESP IDF itself. Or become more intelligent ourselves in the meantime.

Yeah that's why i thought we may could create some "user defined" kconfigs that would translate to rust cfg attributes. That way we don't need "rust features" and we would not need to split the bindgen.h into multiple files. And for example in a potential IDFv6 that has no legacy drivers we could drop them since not defined kconfigs are just ignored. E.g a IDV v6 driver could be included with cfg(any( all(adc_oneshoot, not (legacy_adc)), esp_idf_major >= 6)).

Please elaborate it the way I elaborated the other two solutions, with pros and cons, and ideally - comparing to the other two solutions in terms of development complexity, ergonomics for the end user and so on. Then we have a meaningful basis to discuss on. Because for the moment, I'm a bit in the dark how this solution would improve on the other two.

That way we don't need "rust features" and we would not need to split the bindgen.h into multiple files.

I'm not sure from where the need to split bindgen.h is coming from?

• With either features, or with the default approach (i.e. just components), protecting and not generating bindings for a driver is very simple and does not need any "split".

=> Example with components and the ADC oneshot drivers:

• Changes to the new driver #includes: none
• Changes to the old (legacy) driver #includes: add extra #if C prepro condition: ... && !defined(ESP_IDF_COMP_ESP_ADC)

=> Example with features and the ADC oneshot drivers:

• Changes to the new driver #includes: add extra #if C prepro condition: ... && defined(ESP_IDF_SYS_FEATURE_NEW_ADC_ONESHOT)
• Changes to the old (legacy) driver #includes: add extra #if C prepro condition: ... && !defined(ESP_IDF_SYS_FEATURE_NEW_ADC_ONESHOT)

... where ESP_IDF_SYS_FEATURE_NEW_ADC_ONESHOT is a -D bindgen parameter that the esp-idf-sys build system defines and passes to bindgen only if feature = new-adc-oneshot-driver is enabled (which is of course not to be checked with a #[cfg()] but there is a way.