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

Jump to bottom

Feature Request: VerilatedVcd callback on rolloverMB #1479

Closed
veripoolbot opened this issue Jul 18, 2019 · 6 comments
Closed

Feature Request: VerilatedVcd callback on rolloverMB #1479

veripoolbot opened this issue Jul 18, 2019 · 6 comments
Labels
effort: hours Expect this issue to require roughly hours of invested effort to resolve resolution: abandoned Closed; not enough information or otherwise never finished type: feature-IEEE Request to add new feature, described in IEEE 1800 type: feature-non-IEEE Request to add new feature, outside IEEE 1800

Comments

@veripoolbot
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@veripoolbot veripoolbot added effort: hours Expect this issue to require roughly hours of invested effort to resolve type: feature-IEEE Request to add new feature, described in IEEE 1800 type: feature-non-IEEE Request to add new feature, outside IEEE 1800 labels Dec 22, 2019
@wsnyder wsnyder added the resolution: abandoned Closed; not enough information or otherwise never finished label Aug 24, 2020
@wsnyder
Copy link
Member

wsnyder commented Aug 24, 2020

This is quite old, assuming if it will be needed in the future the patch will be completed.

@wsnyder wsnyder closed this as completed Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
effort: hours Expect this issue to require roughly hours of invested effort to resolve resolution: abandoned Closed; not enough information or otherwise never finished type: feature-IEEE Request to add new feature, described in IEEE 1800 type: feature-non-IEEE Request to add new feature, outside IEEE 1800
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@wsnyder @veripoolbot