mciepluc / cocotb-coverage

Functional Coverage and Constrained Randomization Extensions for Cocotb
BSD 2-Clause "Simplified" License
100 stars 15 forks source link

Fix sense of CoverPoint inj argument #63

Closed jonpovey closed 2 years ago

jonpovey commented 2 years ago

default value and operation of inj argument was opposite to the documentation. Fix the operation to agree with doc.

jonpovey commented 2 years ago

Note I have not tested this change - just eyeballed the code and edited it. When using cocotb-coverage in my tests I am just setting inj=False to get the injection behaviour.

jonpovey commented 2 years ago

Reading Tutorials I see that it agrees with the existing behaviour of inj (not the behaviour described in the source code built-in documentation) Which is correct? What should "Injection" being True mean? only match one bin, or match all? Maybe Injection is a common term in other verification languages. If not maybe the parameter could change name to something clearer like multi_match

mciepluc commented 2 years ago

@jonpovey "injective function" is a mathematical term describing function that can only map to distinct elements. The idea was therefore "inj=True" is the default and means only one bin can be matched at once. I agree it is not very clear, so changing it to "multi_match" seems to be a good idea.

glkclass commented 2 years ago

I join the discussion since I also played a little bit with this parameter. Yes, there is a small mess in spec regarding this parameter which can be fixed easily. But I suggest to remove this parameter at all because of considerations below.

It switches coverage tool between two options: 1) inj=False - to process all matched bins 2) inj=True - to process single bin from several matched. But second option doesn't make a lot of sense since it: a) looses information b) makes coverage results dependent on coverage tool implementation: selected bin depends on order of bins inside a list. Which seems not to be a good idea.

How many bins are matched per sample (single or several) can be defined by 'coverage bins structure': a) defining a list of non-overlapped bins - single bin can be matched b) defining a list of overlapped bins - several bins can be matched.

inj=False option provides such functionality and seems we don't need another one.

mciepluc commented 2 years ago

@glkclass The thing is that SystemVerilog does not accept multiple bins matching at once. That's why I wanted to have a multiple matching as an optional non-default possibility. I fixed the documentation and don't see a need to change the code. For the future we can think about more clear attribute name than "inj".