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

Update to new crates; simplify advanced options #159

Merged
merged 59 commits into from
Oct 19, 2023
Merged

Update to new crates; simplify advanced options #159

merged 59 commits into from
Oct 19, 2023

Conversation

ivmarkov
Copy link
Collaborator

@ivmarkov ivmarkov commented Oct 17, 2023

Following changes:

  • All dependencies raised to the newly-released esp-idf-* crates
  • The advanced options simplified (we had too many). We now have basically "HAL?" y/n and then if "HAL" is selected, "STD?" y/n. Selecting neither HAL nor STD generates:
    • an app_main no-op Rust crate with the cargo driver
    • a no-dependencies Rust crate with the cmake driver which might be useful for Rust code which is purely computational
  • Simplified dependency tree - only esp-idf-svc, log and embuild (the last as a build dependency); no dependencies when the HAL option is not enabled

@ivmarkov
Copy link
Collaborator Author

Still in draft as I need to address all build failures.

@ivmarkov
Copy link
Collaborator Author

Also pulled-in the article of @georgik from here.

There's a slight difference between the project layout described in the article (Rust lib crate source code is separate from the ESP IDF component C source code) versus what is generated by the template (Rust and C source code mixed in a single, hybrid, ESP-IDF-component-but-also-a-rust-lib-crate thing) which might have to be addressed too.

@Vollbrecht
Copy link
Collaborator

Not directly related to the current changes but i want to bring it up while we make changes here. I think a nice addition would be to create two sdkconfig.defaults. I think the option to have two different sdkconfig for release and debug is a good usecase, but the option is in my opinion not visible enough. A good difference between the two would be to set different CONFIG_LOG_MAXIMUM_LEVEL in each. In my simple wifi only test app i shaved 100k flash usage just in strings from this settings. The idea of adding this is merely to promote more, that this options exist.

@ivmarkov
Copy link
Collaborator Author

ivmarkov commented Oct 17, 2023

A good difference between the two would be to set different CONFIG_LOG_MAXIMUM_LEVEL in each. In my simple wifi only test app i shaved 100k flash usage just in strings from this settings. The idea of adding this is merely to promote more, that this options exist.

Can you show an example how you achieved that?
In general, I think the template needs simplification rather than more bells and whistles, so doing two sdkconfig files has to have a really compelling use case.

In fact, we might be better off with a single file, and then a bunch of useful options either enabled there, or disabled via a comment.

@Vollbrecht
Copy link
Collaborator

I think creating a well configured project and files in it, is a different goal than make the generation process less complex for the user. I am on board if you say that there might be two many options for the user, but that doesn't mean that the setup should not generate "labor intensive" things that the user might want to do. Also we explicitly have a fast path for most users - using the "non-advanced" branch. I mean we labeled them "advanced" options for a reason. Just cutting things and let the user handle the task themself is not an improvement.

In the case of of sdkconfigs. We can have two files : sdkconfig.debug and sdkconfig.release. This naming scheme should work across all different mcu's so we dont need multiple per mcu target. What we should set there as a sane default is a question up for debate, but we should outline at least all the common configs that we are highly likely to encounter often.

Setting different max log levels is just one low hanging fruit. Even if we don't change it up and only have one config file, we should include more settings ( out-commented ) like the different stack_size options for event_task etc / https /wss related / ( uart - usb serial related) and similar.

This changes would not create a more or less complex cargo generate matrix. I can try to make a follow up PR with the related changes when this is finished here.

@ivmarkov
Copy link
Collaborator Author

I'm all for specifically putting some extra options in sdkconfig.defaults. Or - if really necessary - having two separate such files.

I do not agree that the purpose of the template is to automate labor intensive tasks and become a swiss army knife.
Its purpose is to get (primarily newbie) users up to speed, with something which does build out of the box, and that's it.

Which is also the original purpose of cargo new, I believe - the tool which we are approaching here.

@ivmarkov ivmarkov marked this pull request as ready for review October 19, 2023 08:21
@ivmarkov
Copy link
Collaborator Author

ivmarkov commented Oct 19, 2023

(updated)

@SergioGasquez Would appreciate your review. Summary of changes:

  • ci_cargo - I've unified the v4 and v5 build to a single job, so as to avoid copy-pasting across the two
  • ci_cmake - Support for all MCUs
  • The CMake docu now has a new "how it works" article. Initially, I used the one from georgik, but then reworked it so as not be a step-by-step guide, but rather to explain how the CMake+Cargo build works under the hood
  • The "advanced" options are now significantly simplified. I don't think we need all of those anymore
  • There's an option to generate a "no-dependencies" no_std and no-alloc crate now
  • Most importantly, the template now works with the latest versions of all esp-idf-* crates, and obviously generates a project which has a single dependency on these now - esp-idf-svc

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this huge work! Just left a few details in the comments.

Also, it would be good if we find a way to skip the run of impossible CI cases (eg: C6 with esp-idf v4.4) without complicating too much the CI files (ie: duplicating code or adding lots of ifs)

README-cmake-details.md Show resolved Hide resolved
README-cmake-details.md Show resolved Hide resolved
README-cmake-details.md Outdated Show resolved Hide resolved
README-cmake-details.md Outdated Show resolved Hide resolved
Comment on lines 1 to +8
{% unless std -%}
#![no_std]
#![no_main]
{% endunless -%}
use esp_idf_sys as _; // If using the `binstart` feature of `esp-idf-sys`, always keep this module imported
{%- if std and hal != "No" %}
use log::*;
{% endif %}

{%- unless std %}
{% endunless -%}
{% unless std -%}
#[no_mangle]
{%- endunless %}
{% endunless -%}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant this conditions be joined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Don't want to touch it too much. biggest hurdle is to get the whitespacing right so cargo fmt does not complain.

@ivmarkov
Copy link
Collaborator Author

Thanks for this huge work! Just left a few details in the comments.

Also, it would be good if we find a way to skip the run of impossible CI cases (eg: C6 with esp-idf v4.4) without complicating too much the CI files (ie: duplicating code or adding lots of ifs)

I agree, good idea, but maybe next time? I barely managed to get this one working.

@ivmarkov ivmarkov merged commit ab2c28d into master Oct 19, 2023
53 checks passed
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.

3 participants