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

Analyzer: Generic Wishbone Device Analyzer #1427

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 98 additions & 3 deletions artiq/coredevice/comm_analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,83 @@ def process_read(self, address, data, read_slack):
raise NotImplementedError


class GenericWishboneHandler(WishboneHandler):
"""Generic Wishbone Bus Data Handler.

Useful for testing out-of-tree PHY devices and viewing their bus transactions
in a VCD file.
"""

comm_buses_and_width = {
"out_address": 8,
"out_data": 32,
"out_stb": 1,
"in_data": 14,
"in_stb": 1
}
def __init__(self, vcd_manager, name, channel):
"""Create the Wishbone Data Handler.

Args:
vcd_manager (VCDManager): VCD Manager to add this device's channels to.
name (str): Name of the coredevice that communicates on this channel.
"""
self.channels = {}
with vcd_manager.scope("bus_device/{}(chan{})".format(name, channel)):
for bus_name, bus_width in self.comm_buses_and_width.items():
self.channels[bus_name] = vcd_manager.get_channel(
"{}/{}".format(name, bus_name),
bus_width
)

def set_channel(self, channel_name, value):
"""Set channel to a specific value.

Formats the data properly for interpretation by VCD viewer
(on a per-channel basis).

Args:
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the usual way those are formatted, see e.g.

:param channel: DAC channel to read (8 bits)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I for one also prefer the numpy/napoleon style.

Copy link
Collaborator

@dnadlinger dnadlinger Mar 13, 2020

Choose a reason for hiding this comment

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

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.)

Copy link
Member

Choose a reason for hiding this comment

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

I meant in general. I agree that consistency trumps that preference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack. will change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

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).

channel_name (str): Name of the channel to write
value (int): Value to be stored to VCD. Should be some sort of integer.
"""
# format all outputs as binary of proper length
chan_fmt_string = "{{:0{}b}}".format(self.comm_buses_and_width[channel_name])
self.channels[channel_name].set_value(chan_fmt_string.format(value))

def process_message(self, message):
"""Process a Wishbone message into VCD events.

Args:
message (typing.Union[InputMessage, OutputMessage]):
A message from the coredevice, either an InputMessage
(signal from PHY to coredevice) or an OutputMessage
(signal from coredevice to PHY).
"""
# TODO: doesn't reset bus (addr/data) to 0 after bus transaction is done
# TODO: stb doesn't display width, but edges register when jumping through waveform
if isinstance(message, OutputMessage):
logger.debug(
"Wishbone out @%d adr=0x%02x data=0x%08x",
message.timestamp,
message.address,
message.data
)
self.set_channel("out_stb", 1)
self.set_channel("out_stb", 0)
self.set_channel("out_address", message.address)
self.set_channel("out_data", message.data)
elif isinstance(message, InputMessage):
logger.debug(
"Wishbone in @%d(ts=%d) data=0x%08x",
message.rtio_counter,
message.timestamp,
message.data
)
self.set_channel("in_data", message.data)
self.set_channel("in_stb", 1)
self.set_channel("in_stb", 0)
Copy link
Member

Choose a reason for hiding this comment

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

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.



class SPIMasterHandler(WishboneHandler):
def __init__(self, vcd_manager, name):
self.channels = {}
Expand Down Expand Up @@ -474,11 +551,11 @@ def create_channel_handlers(vcd_manager, devices, ref_period,
and desc["class"] in {"TTLOut", "TTLInOut"}):
channel = desc["arguments"]["channel"]
channel_handlers[channel] = TTLHandler(vcd_manager, name)
if (desc["module"] == "artiq.coredevice.ttl"
elif (desc["module"] == "artiq.coredevice.ttl"
and desc["class"] == "TTLClockGen"):
channel = desc["arguments"]["channel"]
channel_handlers[channel] = TTLClockGenHandler(vcd_manager, name, ref_period)
if (desc["module"] == "artiq.coredevice.ad9914"
elif (desc["module"] == "artiq.coredevice.ad9914"
and desc["class"] == "AD9914"):
dds_bus_channel = desc["arguments"]["bus_channel"]
dds_channel = desc["arguments"]["channel"]
Expand All @@ -488,11 +565,29 @@ def create_channel_handlers(vcd_manager, devices, ref_period,
dds_handler = DDSHandler(vcd_manager, dds_onehot_sel, dds_sysclk)
channel_handlers[dds_bus_channel] = dds_handler
dds_handler.add_dds_channel(name, dds_channel)
if (desc["module"] == "artiq.coredevice.spi2" and
elif (desc["module"] == "artiq.coredevice.spi2" and
desc["class"] == "SPIMaster"):
channel = desc["arguments"]["channel"]
channel_handlers[channel] = SPIMaster2Handler(
vcd_manager, name)
elif (
"channel" in desc["arguments"].keys() and
desc["type"] == "local" and
"core" not in name.lower() and
"core" not in desc["class"].lower()
):
channel = desc["arguments"]["channel"]
logger.info(
"Adding Wishbone coreanalyzer channel (RTIO#%i): %s: %s",
channel,
name,
desc,
)
channel_handlers[channel] = GenericWishboneHandler(
vcd_manager,
name,
channel,
)
return channel_handlers


Expand Down