Feature Request: VerilatedVcd callback on rolloverMB
I have the time to implement this feature, and am looking for input.
This is a feature for use in conjunction with VerilatedVcd's rolloverMB functionality. I would like to be notified when VerilatedVcd rolls over into a new file, as well as the paths to the old and new vcd file.A few approaches that come to mind from a quick look at VerilatedVcd:
- Add a m_filename getter to VerilatedVcd, and register a changecb on VerilatedVcdC->spTrace(). In the changecb, I would need to implement my own rollover detection.
- Add a rollovercb callback through VerilatedVcd::addCallback() that gets invoked in OpenNext(). This would still require m_filename access.
- Add a new callback (*VerilatedVcdRolloverCb)(VerilatedVcd *vcdp, void *userthis, std::string old_path, std::string new_path) and register it separately and invoke it in OpenNext().
Input from maintainers on a best approach would be appreciated!
#1 Updated by Wilson Snyder 3 months ago
- Category set to Unsupported
- Status changed from New to Feature
I'm surprised there wasn't an accessor on filename() please add that regardless.
Do you really need a callback? The alternative is to define your own class with whatever features are needed and inherit the existing class, and override open(). Then use that new class when creating the waves in your main(). If you need some existing functions changed to virtual (maybe openNext) that would be fine.
#2 Updated by Marc Jessome 3 months ago
Thanks for the feedback Wilson.
I could definitely make this work by inheriting VerilatedVcd with access to filename + virtual openNext(). However, unless I'm reading things wrong, I believe this will have implications on VerilatedVcdC in order to make use of it for standalone simulations.
#4 Updated by Marc Jessome 3 months ago
#5 Updated by Marc Jessome 3 months ago
I have rebased the initial patch on top of master, sorry for posting a second patch.
This patch adds a filename() getter and also allows for polymorphic use of VerilatedVcd and VerilatedVcdC. Please let me know if there are any further changes or if you would like me to take a different approach.
#6 Updated by Wilson Snyder 3 months ago
- Assignee set to Marc Jessome
Good work, in general it seems fine.
Please also have the patch add your name to docs/CONTRIBUTORS to acknowledge your contribution is open sourced as described there.
I'd like to understand why you need to have VerilatedVcdC have a pointer to VerilatedVcd rather than just be one itself. I'm not opposed to this, but it is a bit slower and if you derive from this class it shouldn't matter.
Also it's optional for this, but perhaps you want to add a test_regress test to check the polymorphic use? Otherwise what isn't tested is likely to break by accident in the future.
Also available in: Atom