Skip to content

Conversation

jessebraham
Copy link
Member

This is the second of three PRs performing refactorings on the code base. #245 was the first.

This PR is quite large, so apologies for that. Unfortunately I couldn't really cut these changes down into smaller components, so it is what it is. Fortunately a significant number of the changes are just updating imports or adding the .into_target() function call as needed, so there isn't actually too much new code here.

I've tested this on various boards just to ensure I'm still able to flash, and experienced no issues.

There were a few motivations behind these changes:

  1. In esptool they use the term targets (which is also common in the host/target sense) so this brings us more in-line with their naming (since we're heavily basing espflash off of esptool anyway) and allows us to flatten (what was previously) the chip module.
  2. Eliminate all of the proxy functions of the Chip impl. I've added the .into_target() function which now just returns a given Chip's target instead (as Box<dyn Target>). This decouples the enumeration of supported chips (targets::Chip enum) from the target details themselves (targets::Esp32*.
  3. Refactor the flash_targets module into targets, as they are strongly related
  4. Reduce duplicated code as much as possible when defining targets (some previous work has already gone into this area as well, so there wasn't a lot left to remove).

In addition to the above points, this PR removes more than it adds (which is always a goal of mine, when possible) and reduces the levels of indirection in the code base as well. We also remove a couple dependencies that weren't really necessary in the first place.

One final point, I have removed the re-exports of all types and left only the modules public. I'm not sure how people feel about this (all of the same types are still available, just namespaced) but it seemed like there wasn't really much thought or consistency put into what we were previously re-exporting, and the types that were exported seemed to be chosen fairly arbitrarily. I can revert this if there is reason to, though.

I would like to get at least a couple sets of eyes on this just due to the large number of changes, so @bjoernQ @SergioGasquez @JurajSadel if any of you are able to take a look at some point this week that would be much appreciated!

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 29, 2022

Overall looks like a good improvement to me. But lets wait for the other reviewers

@SergioGasquez
Copy link
Member

The changes look good to me. I've also tested flashing a few targets (esp32c3 and esp32s3) a few times with no issues so far. I'll keep this version for a few days for further testing.

@jessebraham
Copy link
Member Author

Thank you both for taking a look. I will continue to test-drive this new version for a couple more days as well. I'm off work tomorrow so assuming it hasn't been merged yet I will do so early next week.

@JurajSadel
Copy link
Contributor

LGTM as well, thanks @jessebraham!

@jessebraham jessebraham merged commit 575789b into esp-rs:master Oct 3, 2022
@jessebraham jessebraham deleted the fixes/refactor-2 branch October 3, 2022 14:16
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.

4 participants