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

Untangle Flasher code, that isn't related to flashing. #793

Open
MabezDev opened this issue Feb 27, 2025 · 2 comments
Open

Untangle Flasher code, that isn't related to flashing. #793

MabezDev opened this issue Feb 27, 2025 · 2 comments
Labels
RFC Request for comment status:needs-investigation Issue requires further investigation
Milestone

Comments

@MabezDev
Copy link
Member

As discussed in our meeting, Flasher became a bit of a dumping ground for various features. It's quite tangled now, and untangling it will be tricky.

In the meeting we discussed the idea of "some other" structure, that would handle the non-flasher parts of the host <-> chip process, like reading back various parameters. There still needs to be some design work here, and some more investigation therefore I've attached the RFC label.

@MabezDev MabezDev added RFC Request for comment status:needs-investigation Issue requires further investigation labels Feb 27, 2025
@MabezDev MabezDev added this to the v4 milestone Feb 27, 2025
@jessebraham
Copy link
Member

jessebraham commented Feb 27, 2025

I've managed to rip at least a few functions out of the Flasher struct with relative ease, but probably makes more sense to come up with a bit of a game plan before PRing these changes. Even with what I've removed I still think this is violating the single responsibility principle a bit.

@MabezDev
Copy link
Member Author

MabezDev commented Mar 5, 2025

I've been plugging away at this today, after going around in circle for the last few hours, I think I have some ideas about what we can do here.

I think many of the functions have been dumped on to Flasher because Connection lives inside it. I see the pub fn connection(&mut self) -> &mut Connection { as a bit of a smell, because if the Connection is needed outside, it likely shouldn't be inside the Flasher itself (though it looks like it's just for a CLI reset, which maybe isn't so bad).

One, perhaps naive, way to solve this might be to pull Connection out of Flasher and instead pass the Connection into the flasher for the various methods where it's actually needed. I haven't actually tried this in practice though, and we may end up with Connection getting passed around a lot.

Finally, looking at what is used from self, in Flasher it feels like Flasher could disappear entirely. Many of these methods should be on Chip instead (where we pass in &mut Connection to the relevant functions to load the stub etc). There are a lot of instances inside the flasher methods where the first thing we call is self.chip.something(), which makes me feel like Flasher as a concept is unnecessary.

Other misc notes

  • Regardless of above, I think Connection should be constructed outside of Flasher to avoid passing in Connection's constructor params into Flasher::connect
  • I don't know where SpiAttachParams should live, it acts like an input but we only ever use SpiAttachParams::default() or SpiAttachParams::esp32_pico_d4(). If we don't plan on allowing the users to configure this, we should try and associate it with Chip, it should simplify a lot of things.
  • For cases where there are a large number of input params, that we can't untangle, we should consider a config struct to encapsulate it together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comment status:needs-investigation Issue requires further investigation
Projects
Status: Todo
Development

No branches or pull requests

2 participants