cfelton / rhea

A collection of MyHDL cores and tools for complex digital circuit design
MIT License
84 stars 33 forks source link

Added more tests for fifo_fast #14

Closed gcc42 closed 8 years ago

gcc42 commented 8 years ago

Three things:

  1. I've made slight mods to fifo_fast, adding some limits, please review them.
  2. The underflow test doesn't work, because for some reason it takes 2 yield clock.posedge statements to update values at certain times, thus it gives an assertion error instead of expected ValueError. I've also commented in the code. Need to debug that later.
  3. As we discussed, we need some means of error checking. I too like the idea of an ExceptionSignal common to rhea, which we could then include in various interfaces or use separately.

Your thoughts?

cfelton commented 8 years ago

One of the things we can do is use pytest.mark.xfail, meaning that we know the test is failing and we will get to it in the future.

cfelton commented 8 years ago

I commented before looking at the code, you used the xfail, perfect.

cfelton commented 8 years ago

In general this works for now, specific tests can still be setup to verify an exception is raised:

with pytest.raises(ValueError):
    run_testbenchd(bench, args=args)

This will allow the test to produce the error conditions and validate the exception is being raised. This is very helpful in development, because developers are informed of these error early on.

Right now, the errors mainly occur on the nvacant and ntenant, I need to think about it more but the range of these counters can be extended so the errors don't occur here but can occur else where. The whole idea of ExceptionSignals will take a while to flush out.

gcc42 commented 8 years ago

Reverted the changes to fifo_fast. I still think those 2 checks I inserted would be useful, but that is the topic of another discussion now. This PR now only contains the added tests. If the Travis passes(it will take a long time, though :) ), I think you can now merge it into master.

cfelton commented 8 years ago

@forumulator thanks, yes I for the fifo_fast we need to do some homework and make sure the fifo_fast still infers the SRL type FIFO, since the SRL FIFO is limited resources I think the additional checks will break the inference. Two ways to check this: 1) look at the vendor synthesis guides; 2) (better) use rhea.build to compile the fifo_fast for various targets and verify before the changes and after the changes.