-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add python simulator backend #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments, seems like it's on the right path functionality wise. I agree we should nail down the semantics of peek, poke, and expect. I'll try to write up a specification.
fault/python_simulator_target.py
Outdated
print("Advancing clock") | ||
simulator.advance(2) | ||
elif port.isoutput(): | ||
print(f"Setting {self._circuit.name}.{name} to {val}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should change these to use some logging infrastructure. Can we add a fault.logging
module which just uses Python's builtin logging module for now? We can use info
, debug
, and error
levels for now. These are probably best put under info
or debug
. Doing this early let's us add useful logging info throughout the code base but have an easy to silence it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See PR #10. Will update this once that's merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
fault/python_simulator_target.py
Outdated
sim_val = simulator.get_value(port) | ||
assert sim_val == val | ||
else: | ||
raise NotImplementedError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation seems reasonable, but can we decompose this a bit into smaller functions? It's quite large and will get larger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Let me know if you want to merge this as is or wait for more functionality to be filled out. One thing to test would be nested arrays, I think this should just work since we don't have to flatten types. We may need to support that for the coreir simulator target. We're starting to collect a bunch of these In theory, we should be able to run the same set of tests on each target, so it would be good to abstract some tests in this way so that we know the targets are consistent. |
Pull Request Test Coverage Report for Build 73
💛 - Coveralls |
Adding backend for python simulator. Also adding test for python simulator backend. TODO(rsetaluri): Add tests for synchronous circuit (i.e. w/ clk).
This change includes the following: -- Rewrite of the conversion from test vectors to python simulator inputs -- Added fault.logging.debug -- Added more tests for python simulator target
6b1e282
to
4bca960
Compare
Note: addresses issue #4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some minor changes but I'll push them myself.
@leonardt I don't think this is complete yet. Just wanted to throw it out there to make sure it was on the right track. What do you think? I think solving issue #8 will make it obvious what to do here.
Adding backend for python simulator. Also adding test for python
simulator backend.
TODO(rsetaluri): Add tests for synchronous circuit (i.e. w/ clk).