Analyzer: Generic Wishbone Device Analyzer #1427
Conversation
Add generic handler for Wishbone PHY devices, that will save the bus transactions to a VCD file. Signed-off-by: Drew Risinger <drewrisinger@users.noreply.github.com>
Unknown RTIO channels will be converted to VCD by GenericWishboneHandler. Signed-off-by: Drew Risinger <drewrisinger@users.noreply.github.com>
drewrisinger
force-pushed
the
dr-pr-wishbone-analyzer
branch
from
c9803b7
to
83995cf
Compare
| Formats the data properly for interpretation by VCD viewer | ||
| (on a per-channel basis). | ||
|
|
||
| Args: |
There was a problem hiding this comment.
This isn't the usual way those are formatted, see e.g.
artiq/artiq/coredevice/ad53xx.py
Line 74 in 8c5a502
There was a problem hiding this comment.
I disagree. This is a standard format recognized by Sphinx (using the standard Napoleon extension). https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html
The format you cite above I consider much more obtuse and illegible, and doesn't include type information.
There was a problem hiding this comment.
I for one also prefer the numpy/napoleon style.
There was a problem hiding this comment.
Can we just stick to :param foo: bar. in the ARTIQ code base please? This is not a statement on the relative merits of the two choices, just that uniformity decreases cognitive load. (Also, with type annotations now being a thing in the language proper, there is no need to mangle them into the docstring.)
There was a problem hiding this comment.
I meant in general. I agree that consistency trumps that preference here.
There was a problem hiding this comment.
ack. will change.
There was a problem hiding this comment.
Yes, this wasn't an argument about which style is best, but simply an argument for consistency.
If you want to change the format @drewrisinger please submit a separate issue (to discuss it) and then a PR that changes it everywhere in ARTIQ. The same goes for the type annotations that you wanted earlier (though I have some reservations about them).
| ) | ||
| self.set_channel("in_data", message.data) | ||
| self.set_channel("in_stb", 1) | ||
| self.set_channel("in_stb", 0) |
There was a problem hiding this comment.
Those strobe signals are very much an artifact of hardware implementations.
For visualizing transactions from a higher level, maybe use a single VCD channel for the bus and write a string there (similar to your log message) whenever there is a transaction?
You can find code in Migen (commit 1caf61d5e44dbb60491574bf31601cd080d09ad0) to write strings to VCDs, used in FSMs.
ARTIQ Pull Request
Description of Changes
Adds a generic Wishbone device handler for unknown or out-of-tree Wishbone/RTIO devices.
Type of Changes
Steps (Choose relevant, delete irrelevant before submitting)
All Pull Requests
git commit --signoff, see copyright).Code Changes
flake8to check code style (follow PEP-8 style).flake8has issues with parsing Migen/gateware code, ignore as necessary.Tested on in-development PHY. Only deficiencies are listed in
#TODOsections, feedback appreciated.Documentation Changes
cd doc/manual/; make html) to ensure no errors.Git Logistics
git rebase --interactive). Merge/squash/fixup commits that just fix or amend previous commits. Remove unintended changes & cleanup. See tutorial.git show). Format:Licensing
See copyright & licensing for more info.
ARTIQ files that do not contain a license header are copyrighted by M-Labs Limited and are licensed under LGPLv3+.