Revert default to case-insensitive Bus

[paul-demo]

• the case insensitivity feature added in this commit should not have changed the default beahvior and it should have defaulted to False. 1c6b2ac
• case insensitivity feature is completely breaking simulations on multiple backends and on multiple runtime platforms: confirmed with Verilator on MacOS and with VCS on RHEL.

[alexforencich]

This is a known issue with verilator that IIRC has been fixed upstream. But it's possible there are multiple issues in play here with verilator. Flip side, Verilator I think has some other issues with the PLI that need to be fixed upstream and I don't think it has the same level of support in cocotb as some of the other simulators. So, implementing workarounds in cocotb for verilator bugs is something that should be avoided, especially when those workarounds will break things on other simulators. This setting has been the default for years now, changing it is going to break people's testbenches. Anyone using cocotbext-axi in particular would be affected if they're using ALL CAPS in their signal names in their modules, as cocotbext-axi uses lowercase internally. If you're dead set on removing it in cocotb-bus, then I will likely end up adding it directly to cocotbext-axi since it has been the default for basically the entire existence of cocotbext-axi, which will bring us right back to where we are now.

But, the issue in VCS sounds new and should be reported and tracked down, I was under the impression that only verilator was getting screwed up by dir.

[paul-demo]

I think that's the wrong choice. I have never seen a single style guide or a single project where all-caps was the recommended style for RTL signals. Capitals in most languages are reserved for constants and enum values.

Despite it being the default for 2-3 years, when it was originally introduced in March 2021, it changed the default behavior and that broke peoples' testbenches as documented here:

December 2021
#39 (comment)

March 2022
#42 (comment)

And additional reports in cocotb:
cocotb/cocotb#3259
cocotb/cocotb#2244

I'm sure it has negatively affected a lot of people.

Why can't you just be explicit about the capitalization that people want when the object is created, and not iterate through the DUT looking for both attributes? To me it seems like you wanted cocotbext-axi to have a case-insensitivity feature and adding that to cocotb-bus is the wrong place; that should instead be a flag/feature of cocotbext-axi and the Bus class should simply do as it's told and use the signal names the user selects. At least it should be the default behavior.

[paul-demo]

This was actually a reported problem with the case-insensitivity change just 2 weeks after it was introduced. The change was made March 4 2021 and by March 15 2021 it was clear that it was incompatible with some simulators:

cocotb/cocotb#2473 (comment)

I think we should roll it back or disable that feature until it works on more platforms and more/all backend simulators that are supported by Cocotb (ex: those listed in runner.py, for example).

[alexforencich]

Well, for some reason people seem to like using ALL CAPS for signal names with AXI components specifically, because that appears to be part of the spec. I personally don't, and as a result cocotbext-axi explicitly looks for lowercase signal names. Anyone using ALL CAPS names then has to deal with remapping the names somehow. The problem is that "fixing up" the signal names when instantiating the bus objects is a real nightmare, you have to create a bunch of dicts with both sets of names and pass everything through. I'm actually not even sure if that method works with the current setup or if you have to jump through even more hoops so each sub-bus is correctly handled.

Anyway, short term solution was to add case insensitive matching to avoid having to deal with all of the manual signal name remapping.

Long term solution is to take a deeper look at how the Bus object itself works and most likely replace it with something more flexible. IMO, it wasn't really designed very well in the first place. I think there are much better ways of handling the signal connections that just aren't supported by the current Bus object, but I haven't had the time to rewrite it and get all of the kinks worked out. The objects in cocotbext-axi, etc. that extend the Bus object are intended as a placeholder for a future bus object replacement.

Also, I was not aware of this being an issue with any simulator other than Verilator until today when you added me on your issue (I also have not seen most of the issues that you linked until today). It seems most of the issues that people have had with dir() are related to verilator specifically, and verilator has had a number of bugs with its PLI that prevent operation with cocotb. Because of this, for quite some time any bugs associated with verilator were effectively marked as "won't fix" and referred upstream to be fixed in the verilator PLI. It was only quite recently that I think several of the most problematic Verilator PLI bugs have been fixed. So everything related to verilator should be checked with the latest verilator version to see if it is still a problem.

Also, I think we need to try to figure out what's going on with VCS before rolling back this change. Rolling it quickly would have been appropriate if it was reported as a problem with something other than Verilator shortly after it was introduced. But it has been the default for a couple of years, so either way things are broken. At any rate, it's always better to fix the underlying issue whenever possible, since ultimately this stems from calls to dir() causing things to break, and dir() could be called for other reasons.

Incidentally, it would probably be a good idea to add dir() to some of the cocotb tests so it gets proper CI coverage.

[alexforencich]

Also, it seems like there are multiple issues here. Issues with hierarchical names seem like they're probably related more to the signal name matching itself and not some sort of simulator PLI bug that's triggered by dir(), and that's likely something that can just be fixed.

[paul-demo]

Agreed there are multiple issues with the case-insensitive function:

1. With Verilator, the case-insensitive function finds something which is incorrect and proceeds to run the testbench, creating basically no stimulus and observing no real outputs of the dut. This is like Wilson said in your Verilator thread (Fix VPI writes to internal signals to remap back onto the driver verilator/verilator#3919), probably an issue with Verilator not returning identical handles when you use the wrong capitalization. This is a bit more concerning than issue (2) because it fails silently, not with some python exception.

2. With VCS, the case-insensitive function finds nothing when case_insensitive=True (even though it should since case-insensitive should be a superset of case-sensitive results), and this results in 'NoneType' object has no attribute 'setimmediatevalue'.

In both cases it seems like case-insensitivity isn't fully baked yet, regardless of whether that's in cocotb-bus, cocotb, or each individual simulator's VPI interface. I think we can agree Verilator's VPI interface is not fully baked either, since it just got officially released a year ago with version 5.

In any case, it's mostly your work and you can do what you like. I think having mixed capitalization across port names is probably being too lenient (ie, you either want all-caps or all-lowercase).

For the basename, the user can obviously specify it themselves:
AxiStreamBus.from_prefix("my_basename") vs AxiStreamBus.from_prefix("MY_BASENAME")

So all you would need in cocotbext-axi, in order to not rely on the default behavior of case-insensitivity, would be to add an all_caps=False parameter default to your __init__, from_entity, and from_prefix, and just use either _signals or _signals_all_caps when you call the Bus initializer on line 240. It doesn't seem very complicated: one if/else statement, no dict required, etc.

[alexforencich]

What version of verilator are you using? My understanding is that the issue with regards to TOP has been fixed: verilator/verilator#4618 , which should fix dir(). Seems like this should have been included in 5.020. If it isn't fully fixed, then that should be investigated.

[paul-demo]

Verilator 5.018 2023-10-30

I guess I can update and see if the problem goes away. On Mac I'm using whatever is in Homebrew, so I'm not sure if 5.020 is available yet. On Linux, I control it and can release a build from any git comit.

[alexforencich]

So, what happens if you upgrade to 5.020? We should probably specify that the min supported verilator version for cocotb is 5.020.

[paul-demo]

That's actually not true. According to their documentation 5.006 and above are supported. But anyway, yes I will try updating it and see what happens...I'll post back here once I've done that.

[alexforencich]

The TOP bug was present in 5.006, that's the first version that fix a serious PLI regression that was introduced after I think 4.106. So the docs should be updated for min version 5.020.

[paul-demo]

Homebrew already has 5.022 so shouldn't be an issue to upgrade. Hopefully this fixes it.

[paul-demo]

Seems to work with Verilator 5.022! Since it's fixed now, I'm happy to rescind my PR and close the issue I opened in cocotb-bus. Thanks for the help @alexforencich.

[alexforencich]

Good to hear. Sounds like there are still a couple of things that need to be sorted out though - hierarchical matching, and whatever is going on with VCS and dir().

[cmarqu]

Long term solution is to take a deeper look at how the Bus object itself works and most likely replace it with something more flexible. IMO, it wasn't really designed very well in the first place.

FWIW, there is https://github.com/Intuity/forastero now.