Skip to content

Commit

Permalink
Add test, error too long jump tables.
Browse files Browse the repository at this point in the history
  • Loading branch information
JulianSKelly committed Mar 8, 2016
1 parent 870a9a5 commit 0d0b03d
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 5 deletions.
11 changes: 8 additions & 3 deletions fpgalib/dac.py
Original file line number Diff line number Diff line change
Expand Up @@ -1198,12 +1198,12 @@ 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_COUNT = (JUMP_TABLE_PACKET_LEN - (NUM_COUNTERS * COUNTER_BYTES)

This comment has been minimized.

Copy link
@maffoo

maffoo Mar 8, 2016

Contributor

I'd suggest naming this JT_MAX_ENTRIES. Also, it seems to me it would be better to just put a constant number here, rather than computing it from things like the packet length. It's a quirk of the current communication protocol that the packet length and jump table length are the same; this may not be the case in the future. For example, we may be able to upload multiple jump tables in one sram write, or split a long jump table across multiple writes.

This comment has been minimized.

Copy link
@JulianSKelly

JulianSKelly Mar 8, 2016

Author Contributor

Renamed.

) / JUMP_TABLE_ENTRY_BYTES
JT_FROM_ADDR_OFFSET = -2
JT_END_ADDR_OFFSET = JT_FROM_ADDR_OFFSET - 1
Expand Down Expand Up @@ -1516,6 +1516,10 @@ def make_jump_table(cls, jt_entries, counters=None, start_address_ns=0):
:rtype: jump_table.JumpTable
"""

if len(jt_entries) > cls.JUMP_TABLE_COUNT:

This comment has been minimized.

Copy link
@maffoo

maffoo Mar 8, 2016

Contributor

I think there's an off-by-one error here because jt_entries does not include the first NOP instruction (that gets added in JumpTable.toString, but JUMP_TABLE_COUNT is the count of all entries, including the initial NOP.

This comment has been minimized.

Copy link
@JulianSKelly

JulianSKelly Mar 8, 2016

Author Contributor

Oh god, that is really coming to back to annoy us. Good catch.

This comment has been minimized.

Copy link
@JulianSKelly

JulianSKelly Mar 8, 2016

Author Contributor

I put a -1 here and a reference to the issue number

raise ValueError("Jump table too long: {} > {}"
.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
Expand All @@ -1542,7 +1546,8 @@ def make_jump_table(cls, jt_entries, counters=None, start_address_ns=0):
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
8 changes: 6 additions & 2 deletions fpgalib/jump_table.py
Original file line number Diff line number Diff line change
Expand Up @@ -268,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):

This comment has been minimized.

Copy link
@maffoo

maffoo Mar 8, 2016

Contributor

let's call this packet_len since it is no longer a class-level constant.

"""
: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 packets in a JT write

This comment has been minimized.

Copy link
@maffoo

maffoo Mar 8, 2016

Contributor

Number of bytes in jump table SRAM 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
12 changes: 12 additions & 0 deletions fpgalib/test/test_dac.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""This is intended to test fpgalib/dac.py"""

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


Expand Down Expand Up @@ -99,5 +100,16 @@ def test_nop_nop_min_clk_ok(self):
('END', [400])
])

def test_too_many_entry(self):

This comment has been minimized.

Copy link
@maffoo

maffoo Mar 8, 2016

Contributor

nit: test_too_many_entries

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

This comment has been minimized.

Copy link
@maffoo

maffoo Mar 8, 2016

Contributor

The list here has JUMP_TABLE_COUNT + 2 entries. Let's make it JUMP_TABLE_COUNT to ensure that still fails (remember that there's an additional NOP added for the start operation, so if we have JUMP_TABLE_COUNT here there'll be JUMP_TABLE_COUNT + 1 total, which should fail).

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

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

0 comments on commit 0d0b03d

Please sign in to comment.