Some more PoC.arith tests converted to OSVVM#32
Conversation
Paebbels
left a comment
There was a problem hiding this comment.
Thanks for providing more testcases translated to OSVVM.
/cc @stefanunrein
Some general comments:
- I suggest to keep the original author and copyright on files containing major parts of the original testbench.
Usually, this ends up in the testcontroller. In case there are multiple instances or additional code when instantiated, it would also apply to the harness. For these files we add your name as author and the new copyright line, because it was substantially modified.
Otherwise, the testcontroller, the*.profile etc. are new and authored by you and marked with the new copyright line. - As these tests are translated and equivalently good, you can also add a commit deleting the old testcases.
- Maybe we need this deletion for your last PR too :).
- If you find potential improvements to the testcode or new case ideas, don't hesitate to add them as a comment and/or issue.
As you translate testcases quite quickly to OSVVM-style, I assume you know OSVVM well :). Did you have an OSVVM class with me or @JimLewis in the past or is this all self-educated? If you a have OSVVM question or want to know more complex techniques, I can help/teach you.
|
Btw. I figured out why my notification were not working. You see I get your actions (issues, answers, PRs, ...) much faster :). Also because your first PR was accepted, your new PR runs immediately on the scope of VHDL/PoC: https://github.com/VHDL/PoC/actions/runs/19785357026 |
|
Would you like to run PoC in you account to create the online documentation as https://gmartin.github.io/PoC ? I see you have multiple forked repositories like PoC, OSVVM, ... you can synchronize them automatically like so: https://github.com/PLC2/Synchronize |
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
Co-authored-by: Patrick Lehmann <Paebbels@gmail.com>
|
If I counted correctly, I see 2 open review items. When you're done, you can re-request a review (this should trigger me again) and mark the PR as ready (remove draft state). I re-check all updates so far. |
Thank you! I really appreciate your offer to help. I started learning OSVVM when you gave me an in-person course near Munich a few years ago.
Synchronize repo is not available. Maybe it is private? |
Paebbels
left a comment
There was a problem hiding this comment.
I'll do the final adjustment on dev branch (see comment below).
Thanks for you contribution.
| constant A_BITS : positive := 13; | ||
| constant D_BITS : positive := 4; | ||
| constant MAX_POW : positive := 3; |
There was a problem hiding this comment.
These constants can be moved to the testharness, right? This harness is the only place where it's used.
The idea of simplifying the types (e.g. using T_SLVV) was to reduce complexity and remove this package. Maybe I was not clear enough in my previous review comment.
| constant CLOCK_PERIOD : time := 10 ns; | ||
|
|
||
| signal Clock : std_logic; |
There was a problem hiding this comment.
| constant CLOCK_PERIOD : time := 10 ns; | |
| signal Clock : std_logic; | |
| constant CLOCK_PERIOD : time := 10 ns; | |
| constant A_BITS : positive := 13; | |
| constant D_BITS : positive := 4; | |
| constant MAX_POW : positive := 3; | |
| signal Clock : std_logic; |
Ah, now it's confirmed. I always thought if your the Gustavo from that company that gave me the space-laser T-Shirt :)
You're right. Have a look here: https://github.com/Paebbels/SynchronizeForks this works identically. |
New Features
Related Issues and Pull-Requests