esp-rs / esp-idf-template

A "Hello, world!" template of a Rust binary crate for the ESP-IDF framework.
373 stars 43 forks source link

Update to new crates; simplify advanced options #159

Closed ivmarkov closed 8 months ago

ivmarkov commented 8 months ago

Following changes:

ivmarkov commented 8 months ago

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

ivmarkov commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

(updated)

@SergioGasquez Would appreciate your review. Summary of changes:

ivmarkov commented 8 months ago

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.