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

Make callCbs public (again) and return if callbacks were called #1512

Closed
veripoolbot opened this issue Sep 19, 2019 · 4 comments
Closed

Make callCbs public (again) and return if callbacks were called #1512

veripoolbot opened this issue Sep 19, 2019 · 4 comments

Comments

@veripoolbot
Copy link
Contributor

@veripoolbot veripoolbot commented Sep 19, 2019


Author Name: Stefan Wallentowitz (@wallento)
Original Redmine Issue: 1512 from https://www.veripool.org

Original Assignee: Stefan Wallentowitz (@wallento)


For the cocotb support we need VPI callbacks of all sorts. The access to callCbs() was public before but has not been recently. These patches makes it public again, along with returning whether callbacks were called from within the function. The latter is used to iterate a design evaluation until no further callbacks are called (in particular rw synch).

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 19, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-19T23:01:32Z


I'm ok applying this conceptually, but do you think this is the API you really want to use instead of abstracting through the VPI? Or is there a later patch coming that uses this in the VPI implementation?

Nit:

     bool called = cbObjList.size() > 0;

The standard says this can be O(n), so better is !cbObjList.empty();

However there's a bug in that if the continue below is taken, I don't think you want called set, but it will be. Instead set called = true where the callback is actually called.

Also please squash to a single patch next round, thanks.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 20, 2019


Original Redmine Comment
Author Name: Stefan Wallentowitz (@wallento)
Original Date: 2019-09-20T13:47:12Z


Hi,

conceptually the VPI library is generic in cocotb and it used some init hooks to install the callbacks. But Verilator never calls them itself, right? So, similar to the value change callback, we are calling those callbacks in the main loop. This is the relevant part from the main cpp file we supply to verilator:

     vlog_startup_routines_bootstrap(); // let the vpi lib register callbacks etc.
     VerilatedVpi::callCbs(cbStartOfSimulation);

     while (!Verilated::gotFinish()) {
         bool again = true;
         while (again) {
             again = false;

             // Evaluate design
             top->eval();

             VerilatedVpi::callValueCbs();
             again = VerilatedVpi::callCbs(cbReadWriteSynch);
         }

         VerilatedVpi::callCbs(cbReadOnlySynch);

         VerilatedVpi::callTimedCbs();
         main_time++;
         VerilatedVpi::callCbs(cbNextSimTime);
     }

     VerilatedVpi::callCbs(cbEndOfSimulation);

So, I believe this conceptually makes sense. The simulator kernel is our main.cpp, it does not concern the vpi library (so we are not calling those functions from there).

I currently see the following alternatives:

  1. Don't let cocotb-vpi register the callbacks and call them directly where the callCbs() are. This is not very elegant and requires updates whenever something is changed in that common part.

  2. We make Verilator call those callbacks automatically, which in most cases doesn't work, right? I mean, that the time advance is unknown to Verilator runtime itself, similar the start of simulation does not exist per se, I believe. So, we might end up adding Verilated::startSimulation() etc. Not sure thats better.

  3. An improvement to the current patch is to explicitly expose the callbacks identical to value and time change, i.e. add callStartOfSimulationCbs(), callReadWriteSynchCbs(), callReadOnlySynchCbs(), callNextSimTimeCbs() and callEndOfSimulationCbs(). It is probably cleaner, but conceptually not different.

Re your other comment, I think the continue cannot occur on the first iteration as there is no reason the callback got deleted between checking the size and checking if it was deleted from the list. But I am not sure enough and have changed it to update called after the call. Also squashed.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Sep 21, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-09-21T11:45:56Z


Your description seems reasonable. One reason why I was asking was if this should be documented in the main docs, but I think it's unlikely there will be many this sophisticated users, so we'll leave it for now.

Thanks for the patch, pushed to git towards eventual 4.020 release.

@veripoolbot
Copy link
Contributor Author

@veripoolbot veripoolbot commented Oct 6, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-10-06T14:07:45Z


In 4.020. Thanks for reporting this; if there are additional related problems, please open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.