Add access to preponed region of clock edge for monitors in Triggers

[gtaylormb]

The monitors I've seen written in cocotb (including ones in cocotb-bus) have been written to sample in the postponed region of the clock edge accessed by calling ReadOnly() after RisingEdge(), when all signals are assumed to have settled and no more updates may occur on the clock edge event. It seems this is the intended pattern for monitors written in cocotb.

There are two things wrong about this:

1. When an error is detected in the monitor, it appears to occur a clock edge early. This is confusing when debugging, but is mostly tolerable once you are aware of this behavior.
2. More importantly, it does not allow for simulation models that introduce any delays or back-annotated timing simulations. These would update after the clock edge, but typically long after the postponed region of ReadOnly() occurred. The assumption that no more updates will occur on signals after the clock edge with ReadOnly() is incorrect for anything other than purely behavioral simulation models. Of course in the real circuit, with propagation delays through combinational logic any changes are allowed to occur up until the setup time of the next clock edge.

SystemVerilog has defined the preponed region, what they refer to as #1step in 4.4.2.1 of IEEE Std 1800-2012, which occurs just before the clock edge and before any signals are allowed to update. Concurrent assertions fire in this region by default, and you may access this region in SV monitors by using a clocking block and #1step on the inputs. Sampling in the preponed region (or something equivalent) fixes both of the issues above.

Both of these issues here would be solved with this as well: cocotb/cocotb#1060 (comment) and cocotb/cocotb#204

[eric-wieser]

cocotb/cocotb#1662 is likely relevant here

[ktbarrett]

@gtaylormb Is this a general advisory? It seems like this issue belongs in cocotb-bus, where the problematic code exists, not here. I can transfer.

[gtaylormb]

@ktbarrett is there a way to access the preponed region (or equivalent) already in cocotb? If so, cocotb-bus and all other monitor examples should be updated, but if it does not exist I think the functionality needs to be added to cocotb first.

[ktbarrett]

@gtaylormb RisingEdge(clk) leaves you after clock changes, but no sympathetic signals have changed. Sampling any signal here will give you the previous clock settled values. This is equivalent to a registered process. Sounds like this is a cocotb-bus issue.

@eric-wieser I think the issue is that the cocotb-bus Monitors are sampling the transaction in the same clock cycle as the next transaction is applied by (ab)using ReadOnly and ReadWrite, rather than when the transaction is actually seen by the HDL (which is on the next clock edge typically). I don't write my monitors this way, and it is not good practice IMO.

[gtaylormb]

@ktbarrett in that case you are right, cocotb already has the hooks to accomplish sampling in the prepone region, as long as there are no race conditions with signals being updated on RisingEdge() in cocotb. Bus monitor examples should be updated (including cocotb-bus).

[ktbarrett]

@gtaylormb

as long as there are no race conditions with signals being updated on RisingEdge() in cocotb

cocotb writes are non-blocking, they are applied at the start of the ReadWrite phase, there can be no race conditions unless you use handle.setimmediatevalue(0) which you should only be using to set initial signal values at the start of a test IMO.

[gtaylormb]

@ktbarrett perfect. Thanks!

[imphil]

This is a good discussion and what we are discussing here would be valuable as both advise for others writing tests with cocotb, as well as to apply to cocotb-bus were applicable.

@gtaylormb Would you be interested in adding a bit of user "best practices" documentation and/or look at applying that to code in cocotb-bus? Just let me know and we can help you find a good place to put this information.

[gtaylormb]

@imphil sure, I can help with that.

[ktbarrett]

One issue with changing cocotb-bus Monitors is that many of the supported interfaces don't have tests. I don't feel comfortable accepting (non-trivial) changes to any component without a test. Without a test we don't know if a change works or not, or if it changes behavior in a potentially backwards incompatible way, which would require a version bump. These components shouldn't have been accepted without a test to begin with =/

[marlonjames]

xref #24