Bugfix for dumping xz values incorrectly - #12
johnjohnlin wants to merge 2 commits into
Conversation
|
I've merged #11, please check if there are additional changes in this PR that should get merged. |
058401d to
fbe1292
Compare
| uses: veryl-lang/setup-verilator@v1 | ||
| with: | ||
| version: '5.050' | ||
| version: '5.044' |
There was a problem hiding this comment.
Need to revert to 5.044 since I always got http 404 recently...
There was a problem hiding this comment.
A proper solution would be to build Verilator from source or to use the official Verilator docker image, instead of relying on some third party GH action.
| static const char* kEncodedBitToCharTable = ( | ||
| "01" // Binary | ||
| "zxhu" // Verilog | ||
| "xzhu" // Verilog |
There was a problem hiding this comment.
I think xz is more reasonable than zx (see the case encoding below)
There was a problem hiding this comment.
This was intentionally changed in #11 to match the encoding defined in the VPI standard to match the internal encoding used by the new 4 state support in Verilator.
| case 2: delta_time_index = (delta_time_index<<4) | (1<<1) | 1; break; // 'Z' | ||
| case 3: delta_time_index = (delta_time_index<<4) | (0<<1) | 1; break; // 'X' | ||
| case 2: delta_time_index = (delta_time_index<<4) | (0<<1) | 1; break; // 'X' | ||
| case 3: delta_time_index = (delta_time_index<<4) | (1<<1) | 1; break; // 'Z' |
There was a problem hiding this comment.
the zxhu mapping results in 0123 -> 0110 mapping, which is not very reasonable. With xzhu, it becomes 0101
johnjohnlin
left a comment
johnjohnlin
left a comment
There was a problem hiding this comment.
The comments here explain my changes
|
The most important change here is to add an bit-true, hand-crafted integration test for xz. The existing implementation cannot pass the added integration test. The PR aims to fix the issue. I will fix the verilator docker image later. |
There was a problem hiding this comment.
What is this test for? At the moment it doesn't use libfstwriter at all. Do you intend to also add a libfstwriter writer version and compare the outputs?
There was a problem hiding this comment.
All tests under integration test build against fstapi.c (wrapper for libfstwriter) and the original fstapi.c, comparing the bit-trueness. My previous README does not mention this, so I clarify this by updating the README.
| } else if (c == 'h' || c == 'H') { | ||
| b1 |= 1; |
There was a problem hiding this comment.
Why is there an encoding for 'H' but not for 'L'?
I agree, that the CI change was outside the scope of this PR. But I noticed that you already changed CI to use the docker image. Thanks for taking care of that so quickly. |
2 participants
To fix the issue in https://github.com/gtkwave/libfstwriter/pull/11/changes