Closed jedrzejboczar closed 4 years ago
Thanks, i won't probably be able to review it today, but will try to do it early next week. You can indeed continue on things that are not blocking your for now.
I've finished adding tests for _Steerer
and Multiplexer
.
This is a lot of code, but I tried to add a short description to each test, so that it is easier for you to check what functionality is being tested and tell me if I missed something important.
I also moved _CommandChooser
and _Steerer
tests to separate files, as the file has grown to 1068 lines.
The 4 tests mentioned in the PR description still fail, so the CI is red, but all other tests are ok. If you want I can skip these tests and create an issue for that.
Thanks, sorry i haven't been able to review the previous code yet. You can indeed skip the failing tests and create an issue for that. I could look and help you getting this working or fix myself while reviewing.
I'm a bit lagging on the reviews, so if you think the Multiplexer if well covered you can move to another module and i'll add eventual comments on the Multiplexer tests later here or will merge and do the changes while doing the review.
Ok, I'll create an issue in a moment and will continue with other modules.
@jedrzejboczar: thanks for the very good work, i did a first review and we can merge it.
Part of https://github.com/enjoy-digital/litedram/issues/155.
This is still WIP, but I wanted to consult something.
I've started by adding
_CommandChooser
tests, but some of them fail and I wonder if I understand correctly what_CommandChooser
is supposed to do. The current logic https://github.com/enjoy-digital/litedram/blob/b06e946d09807f3ab9b2e72f9c599851ab8221b4/litedram/core/multiplexer.py#L43-L47 seems to be wrong in some cases, e.g.want_cmds=1
andwant_activates=0
, ACTIVATE commands will still be considered, because they have bothis_write=0
andis_read=0
(and we have bothwant_reads=0
andwant_writes=0
), soread
andwrite
are true, and thus(read & write)
is true, even thoughcommand
is falsewant_*
are 0, then bothread
andwrite
are 1 for requests withrequest.is_cmd == 1
so the command is considered validwant_writes=1
andwant_reads=1
, both write and read commands will be considered invalid (but we never want both at the same time, so it is probably a minor problem)The following tests fail:
test_selects_cmds_without_act
test_selects_nothing
test_selects_nothing_when_want_activates_only
test_selects_writes_and_reads
I'll continue now with the tests for
_Steerer
andMultiplexer
.