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

[MabezDev]

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.

[jessebraham]

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]

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

Moved to #854 (comment)

[JurajSadel]

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.

I tried to play around with those ideas and it haven't turned very nicely - here is my experimental (not finished and not working) branch

[SergioGasquez]

Just investigated this issue and based on @JurajSadel, I created SergioGasquez@1184967 which removes the connection field out of the Flasher struct and I could try to apply the other suggestions, the thing is, that branch already includes breaking changes.