-
Notifications
You must be signed in to change notification settings - Fork 37
Add module selector #283
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
Add module selector #283
Conversation
|
Great! the CI passes... this PR is now ready for review :) |
xtask/src/main.rs
Outdated
|
|
||
| // Unfortunate hard-coded list of non-codegen options | ||
| const IGNORED_CATEGORIES: &[&str] = &["editor", "optional", "toolchain"]; | ||
| const IGNORED_CATEGORIES: &[&str] = &["editor", "module", "optional", "toolchain"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be reverted, all code needs to be tested that esp-generate can generate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bugadani thanks for putting time to my PR: Reverting this would cause the CI to fail! so do you think this approach is worth it: Should I populate the module category in the xtask, similar to how the main program does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before you do anything, I'd probably wait for us to actually decide if we want this feature or if the idea is just bad. Then, as I said, code that is generated, needs to be tested: CI will need to be fixed, instead of ignoring the problems it has.
Also, because of the way esp-generate works, I don't think you need a separate Generic entry - selecting none of the modules is already a valid option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! @bugadani thanks for your explaination... and I agree that its too early to push this new feature until the team has decided if its worth it or not!
Also I understand your point about the Generic option, using “no modules selected” as the generic case is cleaner and avoids redundancy.
I’m happy to pause work on this PR for now and wait until there’s a clearer decision on the feature itself. If it’s decided to move forward later, I’d be glad to help with implementing proper CI coverage for the generated code.
Maybe it makes sense to continue the discussion on the original issue you opened about this feature?
|
Ugh, messed up Ig, thus I am converting it into a draft for now |
|
You should probably be aware of esp-rs/esp-hal#4728 where I started encoding the different pin limitations. |
|
It works! |
fb5a1eb to
effda67
Compare
This PR tends to resolve #277
This PR adds a module selector to esp-generate that allows users to specify their exact ESP module. When a module is selected, the generated code automatically reserves GPIOs that are connected to flash/PSRAM, preventing accidental use that could crash the MCU.