greatscottgadgets / cynthion

USB test instrument
https://greatscottgadgets.com/cynthion/
BSD 3-Clause "New" or "Revised" License
42 stars 11 forks source link

Add HyperRAM buffering to analyzer #110

Closed martinling closed 2 weeks ago

martinling commented 3 weeks ago

This PR is stacked on #104 and tries to add HyperRAM buffering to the analyzer.

The code is taken from this branch by @mndza. The integration has been adapted to apply on top of #104, with some additional testing and verification. The Stream16to8, StreamFIFO and HyperRAMPacketFIFO modules are taken unmodified from that branch, and were known to work there.

I have broken down the changes into stages to help isolate the remaining issues. Tests pass at all but the final two steps.

  1. Remove a couple of confusing and unused signals. :heavy_check_mark:
  2. Simplify and improve the USBAnalyzer unit tests. :heavy_check_mark:
  3. Use a 16-bit read port for the FIFO, followed by a Stream16to8 conversion. :heavy_check_mark:
  4. Move the Stream16to8 conversion into the top level module. :heavy_check_mark:
  5. Move the FIFO read port into the sync clock domain, and use a StreamFIFO to cross back to usb. :heavy_multiplication_x:
  6. Insert HyperRAMPacketFIFO into the output pipeline. :heavy_multiplication_x:

The problems appear at step 5, and are caught by the unit tests, e.g:

  File "/home/martin/repos/cynthion-analyzer/dependencies/cynthion/cynthion/python/src/gateware/analyzer/analyzer.py", line 362, in expect_data
    self.assertEqual(received_data, expected_data)
AssertionError: Lists differ: [0, 10, 0, 10, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7] != [0, 10, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

First differing element 3:
10
0

- [0, 10, 0, 10, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7]
?            -      ------

+ [0, 10, 0, 0, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
?                                     ++++++

Basically the output is almost right, but slightly out of sync.

martinling commented 3 weeks ago

Updated, I got a bit further.

The tests get the correct data now, but the tests still fail because the valid signal is not deasserted at the end.

martinling commented 3 weeks ago

Updated. Thanks to @mndza spotting a remaining misuse of the data_commit signal, the unit tests all pass now. We're just trying to get it to pass timing now.

martinling commented 3 weeks ago

Updated again. Current status:

martinling commented 2 weeks ago

Updated once more, @mndza fixed the HyperRAMPacketFIFO!

Remaining issues:

martinling commented 2 weeks ago

Thanks to more fantastic work from @mndza this is now good to go!