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.

Sign up for GitHub

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

Fix unwanted exceptions from dac 15 class. #324

Merged
merged 1 commit into from
Mar 9, 2016
Merged

Fix unwanted exceptions from dac 15 class. #324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
84 changes: 69 additions & 15 deletions fpgalib/dac.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1172,6 +1172,23 @@ class DAC_Build15(DAC_Build8):
19 d[1]
...
33 d[15]

Table documenting how each op code can move the sram, jump table index

OP | BEHAVIOR
------------------------------------------------------------------------
IDLE | increment sram_idx and increment jt_idx
NOP | increment sram_idx and increment jt_idx
CHECK | move sram_idx and jt_idx OR increment sram_idx and jt_idx
JUMP | move sram_idx and jt_idx
CYCLE | move sram_idx and jt_idx OR increment sram_idx and jt_idx
END | no next entry

Attributes:
increment_ops (list[jump_table.Operation]): Jump table op codes that can
increment the sram index, jump table index
jump_ops (list[jump_table.Operation]): Jump table op codes that can
jump the sram index, jump table index
"""

HAS_JUMP_TABLE = True
Expand All @@ -1181,21 +1198,20 @@ class DAC_Build15(DAC_Build8):
SRAM_WRITE_PKT_LEN = 256
SRAM_WRITE_DERPS = SRAM_LEN // SRAM_WRITE_PKT_LEN

JUMP_TABLE_LEN = 528
JUMP_TABLE_PACKET_LEN = 528
NUM_COUNTERS = 4
COUNTER_BYTES = 4
IDLE_BITS = 15
JUMP_TABLE_ENTRY_BYTES = 8
JUMP_TABLE_COUNT = (JUMP_TABLE_LEN - (NUM_COUNTERS * COUNTER_BYTES)
) / JUMP_TABLE_ENTRY_BYTES
JUMP_TABLE_COUNT = 64
JT_FROM_ADDR_OFFSET = -2
JT_END_ADDR_OFFSET = JT_FROM_ADDR_OFFSET - 1
JT_IDLE_OFFSET = -0
JT_IDLE_MIN = 0 - JT_IDLE_OFFSET
JT_IDLE_MAX = 2**IDLE_BITS - 1 - JT_IDLE_OFFSET
JT_MIN_FROM_ADDR_SPACING = 2
JT_MIN_FROM_ADDR = JT_MIN_FROM_ADDR_SPACING - JT_FROM_ADDR_OFFSET
JT_MIN_END_ADDR = JT_MIN_FROM_ADDR_SPACING - JT_END_ADDR_OFFSET
JT_MIN_CLK_CYCLES_BETWEEN_OPCODES = 4
JT_MIN_FROM_ADDR = JT_MIN_CLK_CYCLES_BETWEEN_OPCODES
JT_MIN_END_ADDR = JT_MIN_CLK_CYCLES_BETWEEN_OPCODES
JT_MIN_TO_ADDR = 0
JT_MAX_FROM_ADDR = SRAM_LEN // 4 + JT_FROM_ADDR_OFFSET
JT_MAX_END_ADDR = SRAM_LEN // 4 + JT_END_ADDR_OFFSET
Expand All @@ -1205,6 +1221,15 @@ class DAC_Build15(DAC_Build8):
MONITOR_0 = 5
MONITOR_1 = 10

increment_ops = (
jump_table.IDLE, jump_table.NOP, jump_table.CHECK,
jump_table.CYCLE
)

jump_ops = (
jump_table.CHECK, jump_table.JUMP, jump_table.CYCLE
)

@classmethod
def convert_from_address(cls, x_ns):
""" Convert a from address from ns to SRAM index.
Expand Down Expand Up @@ -1430,7 +1455,7 @@ def make_jump_table_entry(cls, name, arg):
:param str name: one of: END, NOP, IDLE, CYCLE, JUMP, CHECK, RAMP
:param list[int] arg: arguments for the op.
:return: list of jump table entries
:rtype: list[jump_table.JumpEntry]
:rtype: jump_table.JumpEntry
Copy link
Contributor

Choose a reason for hiding this comment

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

huh, good catch here.

"""
if name == 'CHECK':
raise NotImplementedError("Check not implemented yet")
Expand Down Expand Up @@ -1477,23 +1502,52 @@ def make_jump_table_entry(cls, name, arg):
def make_jump_table(cls, jt_entries, counters=None, start_address_ns=0):
"""Make a jump table out of the given entries and counters.

We verify that jump table commands are not executed too frequently. We
check when our current command executes, and compare to execution of
the next command. The next command depends on the specifics of the
current command, arguments and conditions. See attributes increment_ops,
jump_ops.

:param list[jump_table.JumpEntry] jt_entries: JT entries
:param list[int] counters: counter values, or None for all 0s
:param int start_address_ns: SRAM start address, in ns
:return: jump table object
:rtype: jump_table.JumpTable
"""
for i, a in enumerate(jt_entries):
for j, b in enumerate(jt_entries[i+1:]):
if abs(a.from_addr - b.from_addr) < cls.JT_MIN_FROM_ADDR_SPACING:
raise ValueError(
"Entries {}({}) and {}({}) have from addrstoo close together.".format(
i, a.operation, i+j+1, b.operation)
)

# jt_entries does not include initial NOP; see #325
if len(jt_entries) + 1 > cls.JUMP_TABLE_COUNT:
raise ValueError("Jump table too long: {} + starting NOP > {}"
.format(len(jt_entries), cls.JUMP_TABLE_COUNT))

# no two opcodes executed within JT_MIN_CLK_CYCLES_BETWEEN_OPCODES
for k, jt_entry in enumerate(jt_entries):
# first pass we check all increment cases
if isinstance(jt_entry.operation, cls.increment_ops):
sram_idx = jt_entry.from_addr
next_jt_idx = k + 1
next_from_addr = jt_entries[next_jt_idx].from_addr
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we check anywhere that the last entry is an END? Or does that get added automatically (probably)? If not it will error here with a confusing message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK we don't check for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely doesn't get added automatically as the user needs to specify when a sequence will end.

if ((next_from_addr - sram_idx) <
cls.JT_MIN_CLK_CYCLES_BETWEEN_OPCODES):
raise ValueError("Entry {} and entry it increments to "
"executed too closely in time"
.format(jt_entry))
# second pass we check all jump cases
if isinstance(jt_entry.operation, cls.jump_ops):
sram_idx = jt_entry.to_addr
# take off 1 as first entry is always NOP (self.toString)
next_jt_idx = jt_entry.operation.jump_index - 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a -1 here? I thought the NOP would be added already and the indices fixed up before sending anything to the fpga server.

Also, what is the self.toString supposed to indicate?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I see now that the toString method implicitly adds a NOP in addition to the listed entries. This is really unfortunate, because it means the jump table entries we create are not self-consistent, since the numerical indices don't match up with the indices in the list of entries. I'd much rather do either of the following:

  1. require the user to add the NOP, so that then the relative jt indices are consistent (and identical to what goes in fpga memory)
  2. OR, add the NOP as here but also add one to jump indices so that when we compute jt indices they point to the right places in our list.

I would lean toward 2. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Looking at the code, I don't see where this supposed NOP is inserted in toString.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to squint very hard, but this line puts op code 5 (NOP) as the first entry in the jump table. This is required, it's just a question of where we enforce that requirement, and how to do it in a way that the other jump table calculations are self-consistent.

Copy link
Member

Choose a reason for hiding this comment

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

Oh jeez. That was really stupid, sorry. I like option 1 over option 2, but I realize it's more work.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, the reason that it ended up this way is because the first JT op code is required to be NOP, and the argument to it is the start address. Since we already had parameters being passed around for the start address from the previous build of the DACs, it seemed natural to just use those and stick it in there when constructing the JT packet. Perhaps not ideal given that it essentially 1-indexes the rest of the JT entries, but there you have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this is a larger problem that would require coordinated changes across server and client software, I think it's best not to let it hold up the rest of this PR. I'll file a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I was going to suggest that as well. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #325

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was super confusing, it's actually not well described in the documentation that all sequences must begin with an NOP. I happened to remember this was the case but it still took me some time to find it.

next_from_addr = jt_entries[next_jt_idx].from_addr
if ((next_from_addr - sram_idx) <
cls.JT_MIN_CLK_CYCLES_BETWEEN_OPCODES):
raise ValueError("Entry {} and entry it jumps to executed "
"too closely in time".format(jt_entry))

return jump_table.JumpTable(
start_addr=cls.convert_to_address(start_address_ns),
jumps=jt_entries,
counters=counters
counters=counters,
packet_len=cls.JUMP_TABLE_PACKET_LEN,
)

@classmethod
Expand Down
22 changes: 16 additions & 6 deletions fpgalib/jump_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ def __init__(self, from_addr, to_addr, operation):
:param int to_addr: the to address (should be 0 if not used)
:param Operation operation: the operation
"""

# TODO: Why is this -1 hard coded?
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT from the docs, there is no restriction on the relationship between SRAM addresses from_addr and to_addr on a single jump table op. I think this check is just bogus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought the same thing but I am not 100% sure

if abs(from_addr - to_addr) - 1 == 0:
raise ValueError(
"from_addr: {} and to_addr: {} are too close".format(
Expand Down Expand Up @@ -114,6 +116,8 @@ class CHECK(Operation):
which_daisy_bit (int): Which daisy bit to check.
Need to explain how the numbering works.
jump_index (int): Index of jump entry to activte after the check.
Note: jump_table gets an extra NOP start entry at run time in
self.toString, so first user entry is index 1.
bit_state (bool): Selects whether to fire on 0 or 1.
Need to explain more.

Expand Down Expand Up @@ -153,7 +157,8 @@ class JUMP(Operation):

Attributes:
jump_index (int): Index of jump table entry to activate after
this one.
this one. Note: jump_table gets an extra NOP start entry at run time
in self.toString, so first user entry is index 1.
"""
NAME = "JUMP"

Expand Down Expand Up @@ -195,8 +200,9 @@ class CYCLE(Operation):
"""Cycle back to another SRAM address.

Attributes:
jump_index (int): Index of jump table entry to activate after firing
this one.
jump_index (int): Index of jump table entry to activate after firing
this one. Note: jump_table gets an extra NOP start entry at run time
in self.toString, so first user entry is index 1.
counter (int): Which counter to increment at each cycle.
"""
NAME = "CYCLE"
Expand Down Expand Up @@ -262,22 +268,26 @@ class JumpTable(object):
TODO: make self.jumps a property with some checking on type and
number.
"""
PACKET_LEN = 528

# TODO: Move these hardcoded FPGA values
COUNTER_BITS = 32 # 32 bit register for counters
COUNT_MAX = 2**COUNTER_BITS - 1
NUM_COUNTERS = 4

def __init__(self, start_addr=None, jumps=None, counters=None):
def __init__(self, start_addr=None, jumps=None, counters=None,
packet_len=528):
"""

:param start_addr: start address
:param list[JumpEntry] jumps: the jump table entries
:param list[int] counters: the counter values
:param int packet_len: Number of byes in jump table write packet
"""
self._startAddr = 0
self.counters = self._initialize_counters(counters=counters)
self.start_addr = start_addr
self.jumps = jumps
self.packet_len = packet_len

@classmethod
def _initialize_counters(cls, counters=None):
Expand Down Expand Up @@ -313,7 +323,7 @@ def __str__(self):

def toString(self):
"""Serialize jump table to a byte string for the FPGA"""
data = np.zeros(self.PACKET_LEN, dtype='<u1')
data = np.zeros(self.packet_len, dtype='<u1')
# Set counter values. Each one is 4 bytes
for i, c in enumerate(self.counters):
data[i * 4:(i + 1) * 4] = littleEndian(c, 4)
Expand Down
126 changes: 126 additions & 0 deletions fpgalib/test/test_dac.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
"""This is intended to test fpgalib/dac.py"""

import pytest
import numpy as np
import fpgalib.dac as dac


class TestDAC15(object):
@classmethod
def setup_class(cls):
cls.dac = dac.DAC_Build15(0, 'dac_name')

# sequence SRAM: pi pulse from 0-32, pi pulse from 32-64, ro from 64-400
# experiment: first pi -> measure -> second pi -> measure -> end
cls.first_pi_start = 0
cls.first_pi_end = 32
cls.second_pi_start = 32
cls.second_pi_end = 64
cls.ro_start = 64
cls.ro_end = 400

cls.jump_args = [
('JUMP', [cls.first_pi_end, cls.ro_start, 1]),
('JUMP', [cls.ro_end, cls.second_pi_start, 2]),
('END', [cls.ro_end])
]

def make_entries(self, jump_args):
return [self.dac.make_jump_table_entry(name, args)
for name, args in jump_args]

def test_make_jump_table_entry(self):
_ = self.make_entries(self.jump_args)
# we made it this far and we're feeling pretty good about it

def test_make_jump_table(self):
entries = self.make_entries(self.jump_args)
self.dac.make_jump_table(entries)
# we made it this far and we're feeling pretty good about it

def test_catch_nop_nop_too_close(self):
entries = self.make_entries([
('NOP', [16]),
('NOP', [20]),
('END', [400])
])
with pytest.raises(ValueError):
self.dac.make_jump_table(entries)

def test_catch_jump_iterated_nop_too_close(self):

entries = self.make_entries([
('JUMP', [32, 16, 1]),
('NOP', [20]),
('END', [400])
])
with pytest.raises(ValueError):
self.dac.make_jump_table(entries)

def test_catch_jump_back_too_close(self):

entries = self.make_entries([
('NOP', [32]),
('JUMP', [100, 28, 0]),
('END', [400])
])
with pytest.raises(ValueError):
self.dac.make_jump_table(entries)

def test_catch_idle_end_too_close(self):

# from addr of 32 is followed by from addr of 36 (even though IDLE?)
entries = self.make_entries([
('IDLE', [32, 200]),
('END', [36])
])
with pytest.raises(ValueError):
self.dac.make_jump_table(entries)

def test_nop_nop_1_clk_too_close(self):
"""Test a table that executes commands 1 clock cycle too frequently"""

from_addr_ns = self.dac.JT_MIN_FROM_ADDR * 4
entries = self.make_entries([
('NOP', [from_addr_ns]),
('NOP', [from_addr_ns +
(self.dac.JT_MIN_CLK_CYCLES_BETWEEN_OPCODES - 1) * 4]),
('END', [400])
])

with pytest.raises(ValueError):
self.dac.make_jump_table(entries)

def test_nop_nop_min_clk_ok(self):
from_addr_ns = self.dac.JT_MIN_FROM_ADDR * 4
_ = self.make_entries([
('NOP', [from_addr_ns]),
('NOP', [from_addr_ns +
self.dac.JT_MIN_CLK_CYCLES_BETWEEN_OPCODES * 4]),
('END', [400])
])

def test_max_entries(self):
entry_sep = self.dac.JT_MIN_CLK_CYCLES_BETWEEN_OPCODES * 4 * 2
from_addr = (entry_sep * np.arange(self.dac.JUMP_TABLE_COUNT - 2) +
entry_sep)
entries = self.make_entries(
[('NOP', [k]) for k in from_addr] +
[('END', [from_addr[-1] + entry_sep])]
)
self.dac.make_jump_table(entries)
# we made it this far and we're feeling pretty good about it

def test_too_many_entries(self):
entry_sep = self.dac.JT_MIN_CLK_CYCLES_BETWEEN_OPCODES * 4 * 2
from_addr = (entry_sep * np.arange(self.dac.JUMP_TABLE_COUNT - 1) +
entry_sep)
entries = self.make_entries(
[('NOP', [k]) for k in from_addr] +
[('END', [from_addr[-1] + entry_sep])]
)
with pytest.raises(ValueError):
self.dac.make_jump_table(entries)

if __name__ == '__main__':
pytest.main(['-v', __file__])