More handle cleanup

[ktbarrett]

This should be the final PR. Closes #3548.

This PR:

• renames the value handles
• removes what was ModifiableObject and the non-functional drivers and loads methods (closes Change ModifiableObject.drivers and ModifiableObject.loads to be Python iterables #3678)
• adds the set method to set values in a type-safe way
• improves typing and documentation

[ktbarrett]

So currently the issue is with LogicObject and how it works with multi-dimensional packed arrays and logic scalars.

Issues with scalars

When a logic, reg, etc. in Verilog is seen, or a std_logic in VHDL, a LogicObject is used. But such an object is not indexable, doesn't have a range, and isn't iterable. However, LogicObject provides all that functionality, so it must be designed to work in those situations.

Currently there is an _is_indexable property we are checking, which should be False for scalars, which causes indexing to raise, iteration to return nothing, and range to be 1-length and whatever the defaults are (currently it's [-1, -1] from the GPI base classes).

Issues with multi-dimensional packed arrays

When a multi-dimensional packed logic array is seen in Verilog, we can't assume we can index it, as simulators can assume that it's an indivisible object. This means that it also isn't iterable. But LogicObject lets you do both. In these cases LogicObject assumes indexing will fail, and so there is some logic in __iter__ to convert that failure into empty iteration.

Another issue is that the length of the object is the number of total bits in the object, so a logic[2:0][2:0] has a length of 9. This makes sense because when getting/setting the value, you do so with the whole object at a time. But when getting the range, we only get the outer-most dimension of the range: [2:0]. So len(handle) != len(handle.range). This means we can't set the range when getting the value appropriately.

Solutions

IMO, we should separate scalars, single-dimension packed arrays (and std_logic_vector in VHDL), and multi-dimension packed arrays into separate types. None of them should be iterable or indexable.

Scalars would not have a range and getting/setting the value would use Logic instead of a LogicArray.

Single-dimension packed arrays would have a range and getting the value would return a LogicArray with the appropriate range set. Setting the value would take any SizedIterable of the same length (as if it were converted into LogicArray), or an int that would be converted into an unsigned bitvector.

Multi-dimensional packed arrays would ideally return some composite range object. Getting the value would return an Array[LogicArray] or a custom MultiDimensionalLogicArray that could handle multiple-levels of indexing and also maybe setting the whole value at once. Setting would take either a flattened SizedIterable of the same length as the full object, SizedIterables of SizedIterables with the correct dimension and size (as if it were converted into MultiDimensionalLogicArray), or an int that would be converted into an unsigned bitvector. This will require changes the GPI to return the ranges of packed objects, which seems unlikely to be supported.

Failing the above answer, multi-dimensional packed arrays could return a range of 0 to length-1, which I feel would be valid because flattened values don't necessarily have a valid range, so it can be whatever. Getting and setting the value would work like single-dimension packed arrays at that point.

The single-dimension and simple multi-dimension solution could be combined if we can ensure that at the GPI level, the range of packed objects is listed as 0 to length-1 rather than the range of the first dimension.

[ktbarrett]

I decided to split this PR up. The previous comment no longer applies to this PR.

[codecov]

Codecov Report

Attention: Patch coverage is 88.98305% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 68.64%. Comparing base (318ec4b) to head (1efbc9a).
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/cocotb/handle.py	88.69%	11 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3733      +/-   ##
==========================================
- Coverage   68.88%   68.64%   -0.25%     
==========================================
  Files          49       49              
  Lines        7640     7648       +8     
  Branches     2204     2215      +11     
==========================================
- Hits         5263     5250      -13     
- Misses       1301     1326      +25     
+ Partials     1076     1072       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cmarqu]

A first look, will go over it again later.

[ktbarrett]

@cmarqu Take your time. This module needs lots of attention wrt documentation.

[cmarqu]

The inheritance diagram at https://cocotb--3733.org.readthedocs.build/en/3733/library_reference.html#simulation-object-handles is now outdated.

It could be regenerated by locally reverting the change in https://github.com/cocotb/cocotb/pull/3457/files and overwriting the SVG with the image gotten from a browser.
Or maybe Sphinx has been fixed in the meantime?

[ktbarrett]

@cmarqu It's outputting SVG, but not displaying the image, just the code. Any ideas?

[cmarqu]

@cmarqu It's outputting SVG, but not displaying the image, just the code. Any ideas?

Works for me (and doesn't have the size problem described in #3457 (comment)). Do you have a file like cocotb/.nox/.cache/docs_out/_images/inheritance-788f556e7491e4b60b5d6114abe9f8ac3c8d40f5.svg?
If not, is it really outputting SVG, not graphviz/dot syntax (if so, you are probably missing the dot tool)?

Anyway, the output looks a bit "wilder" than before, maybe we should draw it by hand anyway...

[ktbarrett]

Yeah, it definitely needs to be done by hand. There is ABC and Generic and a few other base classes we really don't need to show. Ideally though, we wouldn't document the inheritance hierarchy and instead document only each concrete class. I don't expect many users understand inheritance at any level.

[ktbarrett]

I'm going to leave the diagram unfinished for now, as there will be another follow on commit that changes the hierarchy (bye bye IndexableValueObjectBase).

[cmarqu]

Ideally though, we wouldn't document the inheritance hierarchy and instead document only each concrete class. I don't expect many users understand inheritance at any level.

I'm fine with just removing the diagram.

[ktbarrett]

I think as long as we are documenting inheritance, it's valuable. But if we don't, obviously it isn't.

[ktbarrett]

Considering I'm putting off the diagram update until the next PR, can I get a final review @cocotb/maintainers?

[marlonjames]

Not sure why but the docs show the parameters and return type twice https://cocotb--3733.org.readthedocs.build/en/3733/library_reference.html#cocotb.handle.SimHandle

[cmarqu]

Maybe because SimHandleBase is both a return type annotation and a reference in the "Returns:" part of the docstring?
It's not duplicated in the old version, https://docs.cocotb.org/en/latest/library_reference.html#cocotb.handle.SimHandle

[ktbarrett]

I ran into this in other cases as well, like on the value getters and setters. It's a bug in Sphinx that has something to do with the cache AFAICT. A rebuild fixed the errors that I saw locally, let's see if it does the same here.

[marlonjames]

Looks like still an issue but I'm fine with merging and seeing if it is fixed on master.

[ktbarrett]

Yeah there's something up with the cache. I fixed the missing " character from your last comment, but it's still missing from the documentation. Maybe master will fix itself. I wonder if there's a way to clear or not use the cache that we can set in .readthedocs.yml