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

RFC: Rework configuration methods #507

Closed
SergioGasquez opened this issue Nov 15, 2023 · 4 comments · Fixed by #551
Closed

RFC: Rework configuration methods #507

SergioGasquez opened this issue Nov 15, 2023 · 4 comments · Fixed by #551
Assignees
Labels
RFC Request for comment
Milestone

Comments

@SergioGasquez
Copy link
Member

SergioGasquez commented Nov 15, 2023

At the moment we have several ways of configuring esplfash/cargo-espflash behavior:

The idea would be to simplify and integrate most of this configuration options into a single one.

Related issues: #504, #469

@SergioGasquez SergioGasquez added this to the v3 milestone Nov 15, 2023
@SergioGasquez SergioGasquez added the enhancement New feature or request label Nov 15, 2023
@SergioGasquez
Copy link
Member Author

I'll list some pros and cons about each method:

  • Environment variables:
    • Pros:
      • Can be combined with setting them in the [env] section of the config.toml when using cargo-espflash or the proposed cargo feature of espflash. This would allow modifying settings at different levels.
      • Env variables can be used in when not using cargo or not in a Rust project folder
    • Cons:
      • Not sure how to support array tables like we use at the moment for VID/PID.
      • config.toml may also have some further limitations
  • Package metadata:
    • Pros:
      • Common way of modifying settings in Rust ecosystem
    • Cons:
      • It may require some extra logic or the user to use cargo-espflash/the cargo feature of espflash
      • Not useful when not in a Rust project folder.
  • espflash config file:
    • Pros:
      • Can be configured at different levels (project and global configuration at least)
      • Decoupled from Cargo and user environment
    • Cons:
      • More work on our side

@SergioGasquez SergioGasquez changed the title Rework configuration methods RFC: Rework configuration methods Dec 1, 2023
@SergioGasquez SergioGasquez added RFC Request for comment and removed enhancement New feature or request labels Dec 1, 2023
@bjoernQ
Copy link
Contributor

bjoernQ commented Dec 7, 2023

I never used package metadata (since I never use cargo-espflash) or global config file (didn't knew that exists) but I use env variables for a few things a lot!

e.g. my use-case is I have a few boards attaches, have a terminal open for working with each of them and set ESPFLASH_PORT the right way in each terminal

I don't think we should use env vars as a general config mechanism but for the things we currently support we should definitely keep them. e.g. I have seen people committing their working baud rate (or even serial port) to .cargo/config.toml runner ... makes not too much sense since it depends on your setup what works and what not

In general, those two env vars are also not really config but an alternative way to specify something which is usually given as a command line parameter (we even use a clap feature for that)

TL;DR I don't care much about any config files since I don't use them but please, please, please keep the env vars

@kevin-june
Copy link

I use package metata useful in my daily development workflow, specifically the partition_table option. I can envision some projects might use the bootloader option to reference a precompiled bootloader, but I don't do tha tmyself.

While I don't currently use environment variables, I can see how they could be quite convenient.

@SergioGasquez
Copy link
Member Author

Relevant link: configuration file of esptool

@SergioGasquez SergioGasquez linked a pull request Jan 17, 2024 that will close this issue
@SergioGasquez SergioGasquez self-assigned this Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants