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

Teach target-gen #2543

Closed
wants to merge 6 commits into from
Closed

Teach target-gen #2543

wants to merge 6 commits into from

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Jun 5, 2024

... to:

  • Format JEP106 codes as hex,
  • Format psel as hex (makes RP2040 a bit more readable)
  • Merge duplicate target variants
  • merge equal(ish) flash algos

The idea came while experimenting with STM detection, where the there are myriads of very similar variants of the same chip. probe-rs only cares about memory layouts, core configuration and flash algos currently, so these targets can be merged.

The original target names are preserved under aliases and this information can also be used in the future if a "de-deduplicate" command will become necessary. This is not kept for algos because I don't think they are meaningful if separate.

In total, this PR removes about 40% (in binary size) of our targets/ folder. The website needs to be adjusted to expand targets based on the aliasses, I think.

@bugadani bugadani added the changelog:skip This PR does not require a changelog entry on release label Jun 5, 2024
@bugadani bugadani force-pushed the refresh branch 4 times, most recently from 28da96b to 2908074 Compare June 5, 2024 19:01
@bugadani bugadani marked this pull request as ready for review June 5, 2024 20:09
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good from my perspective :). -110000 on the diff is always nice :D

@bugadani
Copy link
Contributor Author

bugadani commented Jun 6, 2024

Yeah getting rid of 2MB of raw YAML is always a win. But let's not be hasty, there might be unforseen side effects here.

@MabezDev
Copy link
Member

MabezDev commented Jun 6, 2024

there might be unforseen side effects here.

I agree, I only tested esp devices 🙈

@bugadani
Copy link
Contributor Author

bugadani commented Jun 6, 2024

I did a spot-review on most of the changes and they seemed fine, but this PR breaks most of the website targets so that part isn't ideal :) I need a green-light from Noah and probably a website patch to deal with this, first.

Copy link
Contributor

@noppej noppej left a comment

Choose a reason for hiding this comment

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

The 'refresh' is a nice addition to target-gen, and gives us the ability to make future optimizations.

Thanks for the effort to reduce duplicates in the definitions. Clearly a valuable optimization!

@bugadani
Copy link
Contributor Author

bugadani commented Jun 6, 2024

Haha I didn't expect #2540 to significantly impact this idea, but it does. Looks like merging the targets like this is actually not viable, at least not if we want to keep variant name somewhat meaningful (this PR generates duplicates).

@bugadani bugadani closed this Jun 6, 2024
@bugadani bugadani deleted the refresh branch June 6, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:skip This PR does not require a changelog entry on release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants