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

Ignore comments and add error checking. #542

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

d-m-bailey
Copy link
Contributor

@d-m-bailey d-m-bailey commented May 3, 2024

Require default constants in 13'hxxxx format.
Require default constants to be 13 binary bits maximum.
Standardize file names to lower case hex digits.

NOTE: Invalid values are merely ignored. No error code is returned.

Fixes #539 and #540

Require default constants in 13'hxxxx format.
Require default constants to be 13 binary bits maximum.
Standardize file names to lower case hex digits.

NOTE: Invalid values are merely ignored. No error code is returned.
Copy link
Contributor

@RTimothyEdwards RTimothyEdwards left a comment

Choose a reason for hiding this comment

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

I think this should handle most of the ways that a user would mess with the user defaults file. Ideally, it needs a proper verilog parser to catch all the possible ways that a user could screw up this script. I wonder if it needs a more forceful exit on error in a way that would halt the build process instead of just printing an error message. Or does the makefile target catch the error?

@d-m-bailey
Copy link
Contributor Author

@RTimothyEdwards

I wonder if it needs a more forceful exit on error in a way that would halt the build process instead of just printing an error message.

Good point.
What about caravan? It doesn't use gpio 14-24, so should it throw an error for invalid values for those gpio?
What if the user mistakenly deletes a definition for a gpio?

One solution would be to fail on missing definitions (bad definitions would be missing) when doing the replacement in the mag/verilog.

Can we make that a new issue and go ahead with this update?

Copy link
Contributor

@RTimothyEdwards RTimothyEdwards left a comment

Choose a reason for hiding this comment

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

I meant to approve the code last time, but apparently forgot.

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.

make gpio_defaults can incorrectly parse user_defines.v Verilog block comments
2 participants