Conversation
84e465d to
df4493f
Compare
|
Argh, did you notice #408? |
|
(happy to work on this together, no worries, just a bit concerned about the work overlap ,-P) |
@drinkcat I suggest you and @mh0pe collaborate, if possible. Ideally we should merge one set of PRs. What I'm a bit against in that concrete PR:
|
|
@drinkcat Also I wonder how this PR managed to do it without the introduction of a "fake" sysroot, as you did in yours? |
| # Picolibc's -specs=picolibc.specs is incompatible with clang/bindgen. | ||
| CONFIG_LIBC_NEWLIB=y | ||
|
|
||
| # Suppress deprecation warnings for legacy drivers still included in bindings |
There was a problem hiding this comment.
I like this, I wonder if we should actually build the bindings with -Werror so that we're aware of those.
There was a problem hiding this comment.
(I quickly tried, maybe not worth it)
I'll take a look. There's a few things that seem worth picking (as part of a 6.0 support MR, or as follow-ups).
Yep, that's it. I considered forcing to use newlib as well, but I decided to keep the default from esp-idf 6.0 (picolibc), and figure out how to make it work. Apart from the sysroot hack, the rest of support looked easy, I think? (https://github.com/esp-rs/esp-idf-hal/pull/578/changes#diff-2bd60f29989e7f927a4f3f4325d2ec2254df11c50d040d772fbeb67ce3abf008 this should be looked at carefully though, didn't really check how to test) |
I agree. Let's keep your approach then! |
Ah, almost forgot. There's a tool that can do comparison between what Rust's |
drinkcat
left a comment
There was a problem hiding this comment.
Yeah, had a quick look, may have missed things:
- I have a way to support Picolibc -- not particularly pretty though
- I didn't remove 4.x support (this makes the 2 PR a bit difficult to compare -- it'd be nicer to do that as a separate MR IMHO, as it doesn't make 6.0 that much more complicated)
- pcnt handling is different, the advantage of what I have is that it'll become a LOT simpler one we drop support for 5.x. Eventually.
- This PR adds header files in bindings for a bunch of new drivers. I guess that's a good thing to do eventually (but not that terribly useful without the new drivers?)
| #endif | ||
| #endif | ||
| #ifdef ESP_IDF_COMP_ESP_PARTITION | ||
| #ifdef ESP_IDF_COMP_ESP_PARTITION_ENABLED |
There was a problem hiding this comment.
Not sure to understand this one
There was a problem hiding this comment.
This is valid. There is likely no ESP_IDF_COMP_ESP_PARTITION. The correct name is ESP_IDF_COMP_ESP_PARTITION_ENABLED
| #if OLD_DRIVER_COMP_TWAI || defined(ESP_IDF_COMP_ESP_DRIVER_TWAI_ENABLED) | ||
| #include "driver/twai.h" | ||
| #endif | ||
| #if ESP_IDF_VERSION_MAJOR > 5 && defined(ESP_IDF_COMP_ESP_DRIVER_TWAI_ENABLED) |
There was a problem hiding this comment.
I guess this is worth it, maybe not terribly useful if we don't have a driver yet -- but doesn't hurt.
There was a problem hiding this comment.
TWAI = CAN. We do have a driver for that? Or do you mean there is a new twai/can driver? I might have forgotten that...
Also, getting as many raw bindings included (even of we don't have type-safe Rust drivers for those) in general IS useful, as then the user could just call the unsafe C bindings to work with the ESP-IDF drivers.
If the drivers are not even included in the bindings, then there is no escape hatch, which is pretty bad.
With that said, I'm not sure I understand this change...
There was a problem hiding this comment.
Oh I didn't even know what TWAI is ,-P
More generally talking about adding new header files to the bindings -- I assumed a new header means a new esp-idf driver, so no support on the Rust side (yet). Understood it makes sense to add them.
src/include/esp-idf/bindings.h
Outdated
| #if ESP_IDF_VERSION_MAJOR < 6 | ||
| #include "driver/rmt.h" | ||
| // Rename to avoid conflict with rmt_channel_t enum in rmt.h | ||
| #define rmt_channel_t rmt_drv_channel_t |
There was a problem hiding this comment.
I didn't need this... somehow.
| #endif | ||
| #ifdef ESP_IDF_COMP_ESP_DRIVER_SDM_ENABLED | ||
| #include "driver/sdm.h" | ||
| #endif |
There was a problem hiding this comment.
ditto, probably worth adding
| #endif | ||
| #endif | ||
|
|
||
| // New drivers (ESP-IDF v6.0+) |
| @@ -1,9 +1,8 @@ | |||
| #![allow(non_camel_case_types, non_upper_case_globals)] | |||
There was a problem hiding this comment.
That file is simply not needed on 6.0+ (legacy API removed), so I compiled it out.
But I had to add more code to remove pcnt from the blocklist then.
|
More background: rust-lang/libc#3920 |
df4493f to
efdd207
Compare
8f0feef to
13601a5
Compare
@mh0pe On this topic of working together ,-) I'm not sure if I see the value of squashing most of (all?) my changes into this branch, especially given that I attempted to separate changes in logical units with reasonable commit descriptions. BUT, maybe a good approach would be to rebase and stack your changes on top of mine? The new header definitions (and misc fixes in bindings.h) are worth picking at least (not sure what else there is) -- and are technically independent of the 6.0 support itself. Please let me know! |
0c962cd to
31b0942
Compare
|
@drinkcat Rebased on top of your branch as you suggested. Your 10 commits are preserved as-is, with 2 of ours on top:
Also closed the hal (#577) and svc (#647) PRs in favor of your #578 and #650 -- our changes there were entirely cherry-picked from yours. Let me know if anything needs adjustment! |
I do not understand - why cherry-pick from PR X and then open a separate PR Y with the cherry-picked changes, given that PR X is 2 days old max? What am I missing? |
|
Once #408 is merged (and it might be squashed in the process, so you might have to rebase), we'll know the actual surface change of your remaining changes. In any case, thanks for working with @drinkcat and figuring out a common path forward! |
|
@ivmarkov Fair point -- the hal and svc PRs were a mistake on our part. We initially wrote our own fixes, then incorporated @drinkcat's more complete changes, and should have simply deferred to their PRs from the start instead of cherry-picking into ours. That's why we closed them. This sys PR (#409) is the only one where we have unique contributions: the bindings fixes (ESP_PARTITION, lwip_napt, esp_vfs_cdcacm, lcd_panel_rgb) and new driver headers (TWAI, SDM, Camera, ISP, JPEG, etc.), plus the full CI matrix with version exclusions. Those are rebased cleanly on top of @drinkcat's branch. |
db6ac5d to
2ca888e
Compare
- Fix ESP_IDF_COMP_ESP_PARTITION guard (missing _ENABLED suffix) - Guard lwip/lwip_napt.h for v6.0 (removed upstream) - Guard esp_vfs_cdcacm.h for v6.0 (moved to esp_usb_cdc_rom_console) - Fix esp_lcd_panel_rgb.h to remain available on v6.0 via SOC_LCD_RGB_SUPPORTED - Add new TWAI driver bindings (esp_twai.h, esp_twai_onchip.h) for v6.0+ - Add SDM driver bindings (driver/sdm.h) for v5.3+ - Add v6.0+ driver bindings: Camera, ISP, JPEG, Analog Comparator, Parallel IO Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ession Uncomment all ESP-IDF versions (v4.4.8 through v6.0) in the CI matrix and add proper exclusions for targets not supported in older versions: - riscv32imac (ESP32-C6/H2): exclude v4.4.8 - riscv32imafc (ESP32-P4): exclude v4.4.8 through v5.2.5 This gives 43 CI jobs covering the entire supported version range. Also add deprecation warning suppression for legacy i2c/twai/touch drivers in sdkconfig.defaults (v6.0-only Kconfig options, silently ignored by v5.x). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- uart_vfs.h: tighten guard from >= 5.0 to >= 5.3 (header only exists since the driver component split in v5.3) - mbedtls: replace build_info.h + MBEDTLS_VERSION_MAJOR < 4 guard with ESP_IDF_VERSION_MAJOR < 6 (build_info.h does not exist in MbedTLS 2.x used by ESP-IDF v4.4) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2ca888e to
f220717
Compare
|
@ivmarkov Rebased on latest master (with #408 merged). Now just 3 commits with our unique additions:
|
|
Not sure about the value of adding back old esp-idf versions to CI, but otherwise LGTM. |
Summary
Additional v6.0 fixes and new driver bindings, rebased on top of @drinkcat's #408 per their suggestion.
Commits from @drinkcat (preserved as-is):
Our additions (2 commits on top):
bindings: fix v6.0 header issues and add new driver bindings
ESP_IDF_COMP_ESP_PARTITIONguard (missing_ENABLEDsuffix -- pre-existing bug)lwip/lwip_napt.hfor v6.0 (removed upstream)esp_vfs_cdcacm.hfor v6.0 (moved toesp_usb_cdc_rom_console)esp_lcd_panel_rgb.hto remain available on v6.0 viaSOC_LCD_RGB_SUPPORTEDesp_twai.h,esp_twai_onchip.h)driver/sdm.h)CI: enable full version matrix with exclusions
Test plan