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.

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

IOstreamer sample lost bugfix #678

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

purdeaandrei
Copy link
Contributor

Please see individual commits to understand the changes more easily.

@purdeaandrei purdeaandrei changed the title F iostreamer sample lost bugfix IOstreamer sample lost bugfix Aug 24, 2024
@purdeaandrei
Copy link
Contributor Author

This one's ready for review

@whitequark
Copy link
Member

To be honest I find the new tests pretty difficult to understand. If I needed to modify them I'm not sure if I'd been able to do that without just rewriting them (which might have to happen when IOStreamer graduates to amaranth-stdio).

@purdeaandrei
Copy link
Contributor Author

I've made some changes to make this more reviewable:

  • I removed some testcases, that didn't catch any bugs, and the descriptions of which could cause major confusion in the future, IF the implementation of IOStreamer changes
  • I've updated docstrings and comments to explain things better
  • I've refactored save_expected_sample_values_tb coroutine to use .sample() to better reflect the intent of the timing of when a signal is sampled.
  • I've removed iready_comb_path argument (to be added back in PR 675 which is where it's actually used)

I would like to note that the changes to these testcases are a generalization of the previous testcases I implemented, to make it parametrizable, so you can more easily test with various types of stimulus. For example note that most of what used to be test_sdr_input_sampled_correctly() has been factored out and moved into _subtest_sdr_input_sampled_correctly and it's behavior is now much more customizable. And it's complexity only increased slightly.

I would also like to note that these testcases don't depend at all on the implementation of IOStream. If o-stream-to-io latency changes in the future, or o-stream-to-istream latency changes, or the skid buffer's internal behavior changes, then I don't expect the tests to need to be rewritten. (in contrast to the previously existing test_basic() and test_skid() which are both gonna need to be rewritten)

Also I want to note that the bugfix in the PR has been exposed by test_random() which is now the only testcase I'm adding in this PR. If you try the testcase with pre-PR IOStreamer you will be able to see the sample lost situation.

I hope the changes I made make this more understandable.

@whitequark
Copy link
Member

whitequark commented Sep 1, 2024

Thanks! I have a really busy week upcoming (2nd to 8th) where I have travel a lot for work, so I'll only be able to get to these changes afterwards. Also, I will probably review your Amaranth changes first.

@purdeaandrei purdeaandrei force-pushed the f_iostreamer_sample_lost_bugfix branch 2 times, most recently from 20eae69 to 7294ace Compare September 11, 2024 20:18
@purdeaandrei
Copy link
Contributor Author

This PR now has a dependency on #684, and includes its changes. To look only at this PR's changes see: purdeaandrei/glasgow@f_fix_iostreamer_incorrect_sampling_and_qspi...purdeaandrei:glasgow:f_iostreamer_sample_lost_bugfix

@purdeaandrei
Copy link
Contributor Author

Temporarily setting this to draft, to see what happens to the smaller PRs first.

Added support for additionally delaying when the sample is taken.
Normally the samples are taken at a time when the input signals
change that have been launched at the same time as i_en, however
this allows us to take samples later, specified in number of
sync clock cycles. Additionally for ratio=2 iostreamer we can also
delay by half a clock cycle.
This changes how the sdr input sampling test works. Before only positive
edges on clk_out would signify to the testbench that the input has been
sampled. The old behavior put a constraint on payload, that i_en must be
high only for every second payload. This was less then ideal for developing
stronger skid buffer tests.

This change slows down the clk_out signal, and now any edge (positive or
negative), means that we have sampled something. In the waves only the
shape of clk_out changes, the rest of the signals stay the same. (i.e.
the same values are sampled at the same times)

The DDR input sample test does not require a similar change to it.
There was a corner-case in IOStreamer where a sample could be lost,
if it arrives to the skid buffer on the same cycle as when another
sample is removed from the skid buffer.

This PR also adds many more testcases.
This better relays the intent that we want to sample the
"old" value of the inputs in case they change at the same time
as clk_out, and may work better if input_generator_tb changes
in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants