SimClk: clock implemented on the simulator side.

[gilbertoabram]

This reimplements #3954 but in the simulator module without touching the GPI interface, and removing the cycles argument when starting a clock per the comments there. These notes apply here too:

I don't fully understand what's needed from https://github.com/cocotb/cocotb/issues/2108, or if things have progressed in the meantime and that's no longer needed. One difference is that there the clock would run necessarily before any timed callback, whereas here it's implemented with timed callbacks, so the order relative to other CBs at the same time is not guaranteed. However, the current clocks in python also work via Timer and have the same limitation, so maybe it doesn't matter.

One notable difference with using this (and previous PRs' implementations for what I can tell) is that the toggling happens immediately, and not during ReadWrite. @ktbarrett pointed out that if using inertial writes the behavior should be identical, so one option is to use GpiClk automatically only if inertial writes are configured.

One reason in favor of still doing this in all cases (assuming tests pass) is that it's good for performance: the clock toggles before eval() is called, usually reducing the number of eval calls by one (although https://github.com/cocotb/cocotb/pull/3861 should fix that in most cases). And an HDL clock implementation is already different from the python clock in this regard, so if things are written in a way that is agnostic to where the clock is generated, it should also work with this.

I agree this is a better approach than #3954 for what this change is doing. My rationale for doing it in GPI was that it would be possible in a separate change to specialize GpiClk for the various underlying APIs, allowing some further performance gains, whereas doing this in the simulator means it's done as it is. I assumed that would be too much to be in a single PR, but if anyone thinks that makes sense when carried out to the full extent, I don't mind trying to implement it all the way.

That said, just removing the need to go in and out of python and for the extra eval will provide the majority of the performance gain, so in most real use cases the difference with a fully implemented GpiClk would be minimal/below measurement noise.

I thought earlier on I bumped into something in the tests that needed CancelledError in order to work, but it seems that the verilator tests all run to completion for me, so I'd be curious to see what in CI fails. That said, even if they all pass I still think that's required for it to work properly.

[ktbarrett]

My overarching comment is to not use SimClk or simulator in the name here. simulatormodule.cpp is a terrible name. That's our fault for not renaming it sooner.

This file is a part of the layer that provides an embedded Python interpreter and wraps the GPI in Python, so a better name would be PyGPI. That's the name I would use throughout.

One notable difference with using this (and previous PRs' implementations for what I can tell) is that the toggling happens immediately, and not during ReadWrite. @ktbarrett pointed out that if using inertial writes the behavior should be identical, so one option is to use GpiClk automatically only if inertial writes are configured.

This will definitely need to be added before this comes in as well. We will have to leave the existing implementation in and only sub it out when cocotb._conf.trust_interial is True.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 93.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 73.45%. Comparing base (c4b9a26) to head (c80ee78). Report is 139 commits behind head on master.

Files with missing lines	Patch %	Lines
src/cocotb/share/lib/simulator/simulatormodule.cpp	90.90%	0 Missing and 7 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3983      +/-   ##
==========================================
+ Coverage   73.20%   73.45%   +0.24%     
==========================================
  Files          51       51              
  Lines        8006     8107     +101     
  Branches     2194     2215      +21     
==========================================
+ Hits         5861     5955      +94     
+ Misses       1640     1634       -6     
- Partials      505      518      +13     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[gilbertoabram]

I assumed that the existing inertial write test would cover this, but I realize now that it needs to be run explicitly. Do I need to add a test case using both impls explicitly? Also I'm a bit surprised that the other checks succeed. Should I try to get another test in there that breaks due to lack of CancelledError (or figure out why it works without it)?

[ktbarrett]

I have a feeling the failing tests are due to clocks not being stopped properly. I'm guessing there's a difference in how the GC works between 3.11 and 3.12 that prevents the destructor from running immediately.

[gilbertoabram]

I replicated locally, and it seems that the last test I added causes the problem (artificial double start on the same GpiClock) exactly by not getting cleaned up and toggling the signal unexpectedly in subsequent tests. I'll wait on #3874 since I suspect that is the underlying problem. I don't fully understand why it's happening in 3.12 only though (I checked that it also happens in verilator with 3.12, and it fails in the same exact way)

[gilbertoabram]

I realized that with the failing test not using the python Clock object, CancelledError would not fix anything, so I changed it to stop the clock explicitly with finally. I'm still not sure why with Python 3.12 the GpiClock object is not dealloc'd, but it is in the previous versions.

[gilbertoabram]

Apologies if the PR reviews feel nitpicky, but thanks for working through them.

No worries, thanks for reviewing!