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

Implement a language agnostic hardware representation #404

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

RhysGretsch81
Copy link

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2021

Codecov Report

Merging #404 (657716c) into development (679eb13) will decrease coverage by 2.91%.
The diff coverage is 7.76%.

❗ Current head 657716c differs from pull request most recent head a68eba2. Consider uploading reports for the commit a68eba2 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development     #404      +/-   ##
===============================================
- Coverage        90.77%   87.86%   -2.92%     
===============================================
  Files               24       25       +1     
  Lines             6028     6245     +217     
===============================================
+ Hits              5472     5487      +15     
- Misses             556      758     +202     
Impacted Files Coverage Δ
pyrtl/importexport.py 67.64% <7.54%> (-18.02%) ⬇️
pyrtl/hardwareSchema.py 14.28% <14.28%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 679eb13...a68eba2. Read the comment docs.

@mdko
Copy link
Contributor

mdko commented Sep 22, 2021

Hi @RhysGretsch81!

Thanks for the PR. Some requests:

  • Could you add a detailed description here in the conversation about the motivation for this change, what problem it solves, and an example or two?
  • Could you also add some unit tests to test_importexport.py (CodeCov is showing a large decrease in overall coverage)?
  • Does a new file (hardwareSchema.py) necessarily need to be created? Or would this all work by just instead putting the hardwareSchema() function in importexport.py?
  • (Nitpick): Could you change the name of hardwareSchema() to hardware_schema() (for the function) and tohardwareschema.py (for the file, if it ends up needing to stay in, depending on the answer to the previous bullet point)? This makes it more consistent with other naming conventions currently used.

(Also, is this related at all to JSON formats produced by things like Yosys?)

@RhysGretsch81
Copy link
Author

Hi @RhysGretsch81!

Thanks for the PR. Some requests:

  • Could you add a detailed description here in the conversation about the motivation for this change, what problem it solves, and an example or two?
  • Could you also add some unit tests to test_importexport.py (CodeCov is showing a large decrease in overall coverage)?
  • Does a new file (hardwareSchema.py) necessarily need to be created? Or would this all work by just instead putting the hardwareSchema() function in importexport.py?
  • (Nitpick): Could you change the name of hardwareSchema() to hardware_schema() (for the function) and tohardwareschema.py (for the file, if it ends up needing to stay in, depending on the answer to the previous bullet point)? This makes it more consistent with other naming conventions currently used.

(Also, is this related at all to JSON formats produced by things like Yosys?)

Hi @mdko

  • This change was motivated by Dr. Sherwood. He wanted a language agnostic hardware representation to allow for easier communication with other hardware languages, as well as allowing a simple hardware representation for analysis without having to deal with language processing. For more information on the motivation I would ask him.
  • Sure, I am working on adding tests now
  • The hardwareSchema is a way to validate that the JSON fits the standard required for importing/exporting. I believe its useful to keep this as a separate file so other applications can use this schema simply by grabbing this file instead of having to copy/paste.
  • I can update the naming conventions. I'll add a commit to this PR with the updated names and unit tests

I was unaware of Yosys' JSON representation. I'll look into seeing if I should align the protocols

@RhysGretsch81
Copy link
Author

I have updated the protocols based off of a conversation with Dr. Sherwood.
I have also added test cases for both the import and export functions, as well as updating the naming conventions

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