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

Refactored top_gen.py #214

Merged
merged 3 commits into from
Oct 1, 2024
Merged

Refactored top_gen.py #214

merged 3 commits into from
Oct 1, 2024

Conversation

HU90m
Copy link
Member

@HU90m HU90m commented Sep 28, 2024

I was struggling to follow along with what top_gen.py was doing, so I refactored it a little as I went.

The commits:

  1. I added types, made the configuration immutable, and simplified the logic a little.
  2. I split the two stages of top_gen.py, configuration parsing and file generation, into their own files.

Note, the templates and generated output remain untouched to show this refactor doesn't change functionality.

Copy link
Contributor

@AlexJones0 AlexJones0 left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this, it's looking good! I've left some comments on parts that might be refactored further to increase readability. Feel free to not change stuff if you disagree.

I'm happy with this, but I'd recommend waiting for @marnovandermaas to review to ensure that the existing functionality that was originally created is retained with this PR.

util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Show resolved Hide resolved
util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Outdated Show resolved Hide resolved
util/top_gen/generator.py Show resolved Hide resolved
I was struggling to follow along with what top_gen.py
was doing so I added types, made the configuration immutable,
and simplified the logic a little.
A top_gen module allows us to split the two stages of top_gen.py,
configuation parsing and file generation,
into their own files. This is a little easier to read.
No change in functionality
Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Wow, this is a massive rewrite. From an initial read-through it looks better to me than the original. When you run this new top generator, does it generate identical SystemVerilog for the pinmux?

@AlexJones0
Copy link
Contributor

When you run this new top generator, does it generate identical SystemVerilog for the pinmux?

As far as I can tell from running it locally, running this refactor results in no change to any of the generated output. The generated RTL, docs etc. are the exact same.

@HU90m
Copy link
Member Author

HU90m commented Oct 1, 2024

Wow, this is a massive rewrite. From an initial read-through it looks better to me than the original. When you run this new top generator, does it generate identical SystemVerilog for the pinmux?

Yep, I put a bit of work into making sure it generates exactly the same output

@HU90m
Copy link
Member Author

HU90m commented Oct 1, 2024

@marnovandermaas happy for me to merge this?

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Approving since RTL matches according to Hugo.

@marnovandermaas marnovandermaas merged commit f5602b7 into lowRISC:main Oct 1, 2024
2 checks passed
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.

3 participants