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

Unify configuration methods #551

Merged
merged 12 commits into from
Feb 2, 2024
Merged

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Jan 17, 2024

  • Checks for config file in:

    • <current_folder>/.config/espflash.toml
      • Not sure if this is the best path. Maybe other option could be reusing .cargo/config.toml as its present in most(or all) our project.
    • <config_dir>/espflash/espflash.toml
  • Supports the following configurations:

    • Preferred serial and usb connections
      • If we set the ESPFLASH_PORT it will be used instead of the config file value
    • Bootloader
    • Partition table
    • Baudrate
      • If we set the ESPFLASH_BAUD it will be used instead of the config file value
  • [cargo-espflash]: We no longer support setting bootloader, format and partition table in cargo metadata

@SergioGasquez SergioGasquez linked an issue Jan 17, 2024 that may be closed by this pull request
@SergioGasquez SergioGasquez marked this pull request as ready for review February 1, 2024 10:48
@MabezDev
Copy link
Member

MabezDev commented Feb 1, 2024

<current_folder>/.config/espflash.toml

What's the rationale around having it inside <current_dir>/.config, instead of <current_dir>? It doesn't matter much to me, but I just wonder if there is a particular reason. Most other tools with configs, namely probe-rs (cargo embed) place there configuration files in the root of the project.

.get("bootloader")
.map(|bl| package.root().join(bl.as_str().unwrap()));

// TO BE REMOVED?
Copy link
Member

Choose a reason for hiding this comment

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

Could you remind me again what this is? I don't use cargo-espflash very often 😅.

Copy link
Member Author

Choose a reason for hiding this comment

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

You could set the partition-table, bootloader and format in the package metadata, see https://github.com/esp-rs/espflash/tree/main/cargo-espflash#package-metadata. At the moment, I removed baudrate and partition-table properties. IMHO, we can also remove the format

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, thanks! Yes lets remove format too.

@SergioGasquez
Copy link
Member Author

What's the rationale around having it inside <current_dir>/.config, instead of <current_dir>? It doesn't matter much to me, but I just wonder if there is a particular reason. Most other tools with configs, namely probe-rs (cargo embed) place there configuration files in the root of the project.

Tbh, I just followed the suggestion in #469, I can update the path to be <current_dir>

@MabezDev
Copy link
Member

MabezDev commented Feb 1, 2024

imo it should probably be in the current directory, but I'll let @jessebraham @bjoernQ and @JurajSadel weigh in too.

@SergioGasquez
Copy link
Member Author

imo it should probably be in the current directory, but I'll let @jessebraham @bjoernQ and @JurajSadel weigh in too.

Awesome, let's hear their opinion, I am happy to change it with whatever we decide

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 1, 2024

I think I would prefer the current directory

@jessebraham
Copy link
Member

Sorry if I've missed something, why was the Cargo package metadata loading in cargo-espflash crippled? What's the point of even having it in its current rendition?

@SergioGasquez
Copy link
Member Author

Sorry if I've missed something, why was the Cargo package metadata loading in cargo-espflash crippled? What's the point of even having it in its current rendition?

I'd say we can remove the Cargo metadata, as it is now in the config file and its the same for espflash/cargo-espflash

@jessebraham
Copy link
Member

That's disappointing, I use it in most of my projects :/ Guess I will switch over, thanks.

@SergioGasquez
Copy link
Member Author

That's disappointing, I use it in most of my projects :/ Guess I will switch over, thanks.

I mean, we can keep it. No hard opinion there, but then we would have multiple ways of defining those configs.

@SergioGasquez
Copy link
Member Author

SergioGasquez commented Feb 2, 2024

I updated the local config to live under <current_dir>/espflash.toml, removed the format param from cargo metadata for cargo-espflash and udpated the docs

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM

@SergioGasquez SergioGasquez merged commit a55179f into esp-rs:main Feb 2, 2024
20 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.

RFC: Rework configuration methods Attempt to read espflash config from project_dir/.config/espflash.toml
5 participants