Fix compatibility with upcoming cocotb 2.0 #84
Conversation
p12tic
force-pushed
the
cocotb-2.0-compat
branch
from
d820917 to
c3f8221
Compare
|
@cmarqu @ktbarrett Just a friendly ping.. |
|
I'll look at this later today. |
| @@ -223,8 +223,12 @@ async def run_test(dut, data_in=None, config_coroutine=None, idle_inserter=None, | |||
|
|
|||
| pkt_count = await tb.csr.read(1) | |||
|
|
|||
| assert pkt_count.integer == tb.pkts_sent, "DUT recorded %d packets but tb counted %d" % (pkt_count.integer, tb.pkts_sent) | |||
| dut._log.info("DUT correctly counted %d packets" % pkt_count.integer) | |||
| assert convert_binary_to_unsigned(pkt_count) == tb.pkts_sent, ( | |||
There was a problem hiding this comment.
Is there a reason to not just use .integer here if the intent is to support both 1.X and 2.X?
There was a problem hiding this comment.
The problem is that .integer throws a deprecation warning on 2.0 which is hard to turn off. It wouldn't be a problem if cocotb-bus was a leaf package, then we could turn off the warning globally and call it a day. However, cocotb-bus can be used by third-party packages as well. This means that we should disable the warning on each call site. Since this functionality would be very similar in each call site, we would be tempted to extract it to a function and thus we would arrive to something like convert_binary_to_unsigned again.
Upcoming cocotb 2.0 will change the type of the value returned from signals. In order to support both cocotb 1.9.x and 2.x the library must abstract these two types. Fortunately, there are few major differences and they can all be abstracted into several functions.
Not creating a new function should improve tracebacks for example.
p12tic
force-pushed
the
cocotb-2.0-compat
branch
from
c3f8221 to
44ba345
Compare
Up until cocotb/cocotb@bfd811b setimmediatevalue() did not actually work as expected. Using regular value assignment preserves the old behavior.
b9dd5ee1 is most recent cocotb 2.0 version as of 2024-09-24
|
@ktbarrett The PR is ready for a review again. |
There was a problem hiding this comment.
I can't say I love the approach of the compat layer, but nothing else comes to mind.
I was considering dropping the DeprecationWarnings on the LogicArray interfaces until 2.1, so there's at least one release where there's no warning, but then we have the same issue once 2.1 drops, so that doesn't solve anything really. I guess this is just proof its easier to get it right the first time.
The root cause is that cocotb-bus supports cocotb as old as 1.6. If only 1.9.x and 2.x were supported then it would be possible to backport a lot of things to 1.9.x and reduce the amount of deprecation warnings without a solution on cocotb-bus side. E.g. missing functions can be added to BinaryValue so that similar semantics are supported and a switch from BinaryValue to LogicArray changes nothing at the source level at least for that part of functionality. |
|
@p12tic I think this PR can come in as is, but that does bring up a good point. We only ever support the most recently released version of cocotb, so removing support for cocotb<1.9, and only supporting 1.9 and 2.0+ from here on out is totally fine by us. Especially considering that I'm pretty sure the only changes that have been made since the last release are pretty minor, so I don't see doing so as gatekeeping new features. |
|
Sorry, I'm still on vacation so cannot do a real review. |
The PR still supports the older cocotb versions, so there's need for new release from before this PR. On the other hand, this PR drops requirement on |
|
@p12tic Sorry about the delay. We usually like to have more than one maintainer approve larger PRs or merge them after a couple days if there are no comments, but this got lost. |
This PR fixes remaining issues that prevented compatibility with cocotb 2.0, mostly BinaryValue -> LogicArray migration.
CI for cocotb 2.0 has been enabled and compatibility with cocotb 2.0 has been declared in setup.py.
Note that latest cocotb master doesn't work due to cocotb/cocotb@bfd811b breaking something.