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

Feature Request: VerilatedVcd callback on rolloverMB #1479

Open
veripoolbot opened this issue Jul 18, 2019 · 5 comments
Open

Feature Request: VerilatedVcd callback on rolloverMB #1479

veripoolbot opened this issue Jul 18, 2019 · 5 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Jul 18, 2019


Author Name: Marc Jessome
Original Redmine Issue: 1479 from https://www.veripool.org

Original Assignee: Marc Jessome


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:

  1. 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.
  2. Add a rollovercb callback through VerilatedVcd::addCallback() that gets invoked in OpenNext(). This would still require m_filename access.
  3. 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!

Thanks

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 19, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-19T11:57:17Z


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 19, 2019


Original Redmine Comment
Author Name: Marc Jessome
Original Date: 2019-07-19T15:06:29Z


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 19, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-19T15:36:58Z


I'm not sure what problem you are forseeing. Perhaps give inheriting a try? We welcome patches that result.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 22, 2019


Original Redmine Comment
Author Name: Marc Jessome
Original Date: 2019-07-22T20:09:54Z


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.

Thanks again!

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jul 22, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-22T22:15:22Z


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.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.