Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support remote components and turn component manager on #222

Merged
merged 8 commits into from
Aug 30, 2023

Conversation

N3xed
Copy link
Collaborator

@N3xed N3xed commented Jul 27, 2023

This adds the ability to specify remote components in the Cargo.toml and the esp-idf component manager is now turned on by default, and it will manage all such remote components (specified on the Rust side, or in esp-idf components).

I've changed the template cmake project structure to turn the component manager on. It now uses the normal project setup with project.cmake. It should, in theory, work the exact same, though I haven't tested all of it.

Remote components make use of the component manager, and its component_idf.yml file. All specified remote_components actually map to equivalent entries in that file.

They can be specified with the remote_component field in an extra component, e.g.:

[[package.metadata.esp-idf-sys.extra_components]]
remote_component = { name = "espressif/mdns", version = "1.2" }

Note though that remote_component does not accept a list. If you want to have multiple remote components, use multiple extra component entries.
Other than that, the syntax that @daggerrz suggested in #220 and esp-rs/embuild#78 is still supported, though component_refs is now remote_component.
See the docs for all supported fields (the same as the dependencies section in the idf_component.yml).

As I said, some more testing is still needed, it would be appreciated.
Feedback is also appreciated.

Fixes #193

@N3xed N3xed changed the title Add remote components and turn component manager on Support remote components and turn component manager on Jul 27, 2023
@daggerrz
Copy link

daggerrz commented Aug 2, 2023

I finally got back and was able to test this. I see the components are downloaded properly, and are part of the build path, but the cfg set is the file system "projection" / direcotry name of the component FQN. E.g for espressif/mdns, the cfg is esp_idf_comp_espressif__mdns_enabled

This does not line up with bindings.h in esp-idf-sys:

#ifdef ESP_IDF_COMP_MDNS_ENABLED

and in lib.rs in esp-idf-svc:
https://github.com/esp-rs/esp-idf-svc/blob/cf0b85c7d92c7382cd87d5abb6b3a1bede70de6f/src/lib.rs#L59

I'm not sure if it's better to use the "bare" component name in the cfg to make it compatible with the current bindings or to update the bindings.

For reference, from the build logs:

[esp-idf-sys 0.33.1] cargo:rustc-cfg=esp_idf_comp_espressif__mdns_enabled

@N3xed
Copy link
Collaborator Author

N3xed commented Aug 3, 2023

Thanks for testing it.

Yeah, I noticed this aswell. As far as I see it, we could:

  1. If we want to support the existing component names, add an exception to espressif/* components, so that we also define the same component without the espressif__ prefix.

    This would only be worth it if we expect further components to become remote components, and/or there are many cfgs for these components already in the code.

  2. As you said, change the #[cfg]s in the code.

I really don't know how much of an effect these name changes have. And whether there will be more and many. Maybe @ivmarkov knows more?

@ivmarkov
Copy link
Collaborator

ivmarkov commented Aug 3, 2023

Specifically ESP_IDF_COMP_MDNS_ENABLED we can't change as we are supporting ESP IDF V4.4 where this component is built in.

How about just adding || esp_idf_comp_espressif__mdns_enabled (in uppercase of course) and then Instrumenting esp-idf-svc to check both cfgs? I'm not sure we'll see a flood of components leaving the ESP IDF monorepo, so maybe let's not boil the ocean as of yet, just because of mdns?

@daggerrz
Copy link

daggerrz commented Aug 4, 2023

How about just adding || esp_idf_comp_espressif__mdns_enabled (in uppercase of course) and then Instrumenting esp-idf-svc to check both cfgs? I'm not sure we'll see a flood of components leaving the ESP IDF monorepo, so maybe let's not boil the ocean as of yet, just because of mdns?

I think this would be very clear in terms of the esp-idf-svc code and the solution that has the least potential to cause future confusion due to component name conflict (although it's probably extremely unlikely in the first case). As more components are moved out of IDF, the esp_idf_comp_espressif__ prefix in the cfgs will make it easy for maintainers immediately identify that a component has in fact been moved out.

It would also make it easy spot new components that are only "remote" and exist for 5.x that don't exist / have to be maintained in 4.4 at all, but still are added to esp-idf-svc (as opposed to a litany of binding crates).

// IDF Provided component. 
#ifdef ESP_IDF_COMP_{component}_ENABLED
...

// Component migrated from IDF to remote in 5.x+, but IDF provided in 4.4
#if defined(ESP_IDF_COMP_{component}_ENABLED) || defined(ESP_IDF_COMP_espressif_{component}_ENABLED)
...

// Remote-only component, only available in 5.x+
#ifdef ESP_IDF_COMP_{namespace}_{component}_ENABLED
...

Allows the component manager to be used without workarounds.
Adds the ability to specify remote components in the Cargo.toml metadata.
Adds configuration option `ESP_IDF_COMPONENT_MANAGER` to allow turning
the component manager off.
@daggerrz
Copy link

Can confirm that the current brand works as expected if the cfgs also are updated in esp-idf-svc. E.g: esp-rs/esp-idf-svc@c69726e

@dacut
Copy link
Contributor

dacut commented Aug 18, 2023

@N3xed, just wanted to make sure you saw the comment over in #231 (comment). It's possible (perhaps likely) I was doing something wrong, but adding this to Config.toml:

[[package.metadata.esp-idf-sys.extra_components]]
espressif_tinyusb = { name = "espressif/tinyusb", version = "0.15.0_2" }
espressif_esp_tinyusb = { name = "espressif/esp_tinyusb", version = "1.3.1" }

Resulted in invalid cfg keys being set (the . in the version number is invalid):

[issue231 0.1.0] cargo:rustc-cfg=esp_idf_comp_espressif_tinyusb_0.15.0_2_enabled
[issue231 0.1.0] cargo:rustc-cfg=esp_idf_comp_espressif_esp_tinyusb_1.3.1_enabled
error: invalid `--cfg` argument: `esp_idf_comp_espressif_tinyusb_0.15.0_2_enabled` (expected `key` or `key="value"`)

This should be set without the version number, but I suspect that's something happening with ESP-IDF.

@N3xed
Copy link
Collaborator Author

N3xed commented Aug 22, 2023

@N3xed, just wanted to make sure you saw the comment over in #231 (comment). It's possible (perhaps likely) I was doing something wrong, but adding this to Config.toml:

[[package.metadata.esp-idf-sys.extra_components]]
espressif_tinyusb = { name = "espressif/tinyusb", version = "0.15.0_2" }
espressif_esp_tinyusb = { name = "espressif/esp_tinyusb", version = "1.3.1" }

Resulted in invalid cfg keys being set (the . in the version number is invalid):

[issue231 0.1.0] cargo:rustc-cfg=esp_idf_comp_espressif_tinyusb_0.15.0_2_enabled
[issue231 0.1.0] cargo:rustc-cfg=esp_idf_comp_espressif_esp_tinyusb_1.3.1_enabled
error: invalid `--cfg` argument: `esp_idf_comp_espressif_tinyusb_0.15.0_2_enabled` (expected `key` or `key="value"`)

This should be set without the version number, but I suspect that's something happening with ESP-IDF.

Yup, I saw it. Haven't had time to get to it yet, though. I'll investigate this and try to finish the PR next weekend.

@N3xed
Copy link
Collaborator Author

N3xed commented Aug 27, 2023

@dacut I just commented in #231 and it seems to me this is a unrelated issue.

So, this PR seems ready to be merged. I'll do it in the coming days if there are no further issues.

@dacut
Copy link
Contributor

dacut commented Aug 28, 2023

@N3xed Sounds good to me. I was probably "holding it wrong" as you point out. 😄

@N3xed N3xed merged commit 429bd57 into master Aug 30, 2023
4 checks passed
@N3xed N3xed deleted the feature/remote-components branch August 30, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using espressif camera,
4 participants