Extend verilator main loop to support clock generation in C++

[marlonjames]

Closes #2108.

Provide a user callback declaration for toggling clock(s).
User defines the callback and provides the file for compiling/linking.

I'm not sure that this is the best approach, but it was the simplest in terms of additional work for users and cocotb that I could come up with.

Tagging @alexforencich, @wallento.

[codecov]

Codecov Report

Merging #2275 (53ccf1c) into master (f8c0e40) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2275   +/-   ##
=======================================
  Coverage   66.78%   66.78%           
=======================================
  Files          48       48           
  Lines        7969     7969           
  Branches     1322     1322           
=======================================
  Hits         5322     5322           
  Misses       2059     2059           
  Partials      588      588           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8c0e40...53ccf1c. Read the comment docs.

[alexforencich]

IMHO, ideally, this should be handled transparently when using a Clock object from python, or something along those lines. If we can do clock generation at the GPI layer, it should be handled in a similar way. Having to write a C++ file along side the python code and have that only apply to verilator is not great. Would it be possible to implement some sort of an API that can be called from python that's something along the lines of ref = start_clock(signal, half_period) and stop_clock(ref) or perhaps stop_clock(signal)? If that kind of API is available, then it should be straightforward to modify Clock to use that instead of starting up a coroutine. Although there probably needs to be some care taken to make sure things are cleaned up properly between tests.

[marlonjames]

@alexforencich that is the plan for #89.

[ktbarrett]

@garmin-mjames I guess I was hoping for a little more. Something where we could create multiple clock dynamically. I understand that is a good bit of work though. How does this approach handle users writing to the same clock signal from cocotb?

[marlonjames]

@ktbarrett it doesn't. This is just the Verilator equivalent of a free running HDL clock in a testbench wrapper.

A GPI clock should be the replacement for Clock internals. We suggest HDL clocks for performance but that's due to the pure Python clocks in cocotb. Perhaps it's better to abandon this and fix the clock performance so there is only one "cocotb way" to generate clocks.

[ktbarrett]

I'm fine with simpler methods we can accomplish now, but only if they are going to be forward compatible. If we add this support here and then have to remove it later once we have a better way that isn't compatible, then we are going to have some angry users.

[marlonjames]

This PR is almost out of scope of cocotb, except that we provide the Verilator main loop in verilator.cpp. This allows users to extend the functionality instead of copying verilator.cpp and Makefile.verilator and making bespoke changes to get a fast clock.

Again, it's equivalent to an HDL clock in a testbench. GPI clocks will essentially obsolete this approach, being almost as fast, more flexible, and controllable from cocotb, but there's no reason this would have to be removed later. Users would have to choose one approach, as they do now with Python vs HDL clocks.

That being said, if the GPI clocks happen soon this isn't really necessary.

[ktbarrett]

That being said, if the GPI clocks happen soon this isn't really necessary.

I doubt GPI clocks are going to happen any time soon. I need to think it out some more, but I feel like it may require we duplicate some scheduler functionality in C. And then we have object lifetime issues to worry about as well that probably deserve a garbage collector. Maybe I'm overthinking it.

This allows users to extend the functionality instead of copying verilator.cpp and Makefile.verilator and making bespoke changes to get a fast clock.

It was hard enough for us to get right. We probably should try to shield users from that. I'm coming around on this approach.

[ktbarrett]

It's certainly flexible. Users can manually manage multiple clocks if need be with a simple scheduling algo, and this allows the user to model skew as well.

[ktbarrett]

Do you need this protection here since the main loop only calls the user_clock_cb on the next clock edge by design?

Suggested change

	if (current_time % CLK_HALFPERIOD == 0) {
	topp->clk = !topp->clk;
	}
	topp->clk = !topp->clk;

[marlonjames]

Not in this single-clock example.

[ktbarrett]

Perhaps the example should be expanded for multiple clocks and skew? Something that users can copy-paste-modify for their own tests.

I'm thinking an array of next clock edge times mapped to clock handles and halfperiods. Upon entry of the callback, walk the array to see if any clocks need updating by comparing it to the current time. If they do, toggle the clock and update the time with the next time using the halfperiod and skew. Finally, return the min time in the updated clock edge time array.

Users could copy and paste and modify the array definition as they see fit.

[marlonjames]

@ktbarrett I think GPI clocks can be done fairly simply: #89 (comment)

[alexforencich]

I mean, if cocotb has a simulation loop for verilator and implementing it there means lower overhead than doing it in GPI, then I see no problem with doing both so long as the APIs are similar. The clock object can use the Verilator version when running under Verilator, and the GPI version otherwise. At least Verilator is open source so it's simple to include in regression tests, etc. Also, we could possibly implement this now and make sure the API makes sense, since the GPI clock issue has been open for 7 years now.

[ktbarrett]

@garmin-mjames Are you still interested in following this PR to completion?

[marlonjames]

If the GPI clocks get in 1.5 then I would close this rather than have two options for C++ clocks.

[marlonjames]

@ktbarrett thoughts on getting this in 1.5 now that GPI clocks are postponed to 2.0?

[ktbarrett]

Is it ready for a final review? If so, we can bring this in with 1.5.

[marlonjames]

I can update the example in the docs and the test per your previous review.

[ktbarrett]

Well this didn't make it into 1.5. Still interested in bringing it in @marlonjames?

[marlonjames]

I still think it's useful. I was focusing on fixing Verilator regression tests, then 4.108 broke compat, so I hadn't gotten around to finishing this.

Adding jitter to the example was making it pretty complex. It may be better left as an exercise for the user, or at least put only in the test but not the docs.