IOstreamer sample lost bugfix

purdeaandrei commented 3 months ago

Please see individual commits to understand the changes more easily.

purdeaandrei commented 3 months ago

This one's ready for review

whitequark commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

purdeaandrei commented 2 months ago

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

GlasgowEmbedded / glasgow

IOstreamer sample lost bugfix #678