CoveragePkg: Add "getNextCoverPoint" function

[SkydiverTricky]

sometimes it can be useful to get coverage points sequentially or randomly, depending on the test. Maybe it would be useful to add a function something like:

impure function nextCoverPoint(isRandom : boolean := true) return integer/integer_vector is
begin
  if isRandom then return randCoverPoint;
  else return getNextCoverPointSequence;
  end if;
end function;

Thoughts?

[JimLewis]

I am not against this, however, can you provide an example of where it is good to hide the intent of what you are doing like this.

Let me give an example of my concern from RandomPkg. RandomPkg has the capability to select which of uniform, normal, favorbig, favorsmall, and poisson (just for fun) distributions are used by RandInt and friends. However, as I read the tests, it results in something like:

RV.SetRandomParm(NORMAL, <mean>,<stddeviation>); 
....
IntVal := RV.RandInt(0,15) ;

The problem I see is as the distance between the SetRandParm and the RandInt increases the less readable the code is. I think we need to take care with our abstractions and be sure to clarify the intent of the code and not obfusicate it, hence, later the following was added:

RV.Normal(<mean>, <stddeviation>, 0, 15) ; 

I find myself challenged to find a use model for it, and hence, have actually thought of removing it.

Maybe if you could provide a good use model for this, I could both justify adding it and decide the current capability in RandomPkg is actually a good thing.

[SkydiverTricky]

I dont really have a specific test case - it was more for ease of debug. For example, when generating packet data, where the payload is inconsequential other than the length, I can currently generate it either as fully random payload, or as a count sequence. The count sequence makes waveform debug much easier as I can always tell from the payload where in the packet I am. I thought it might be useful to have a similar switch when using coverage. I currently use coverage to generate specific headers in a packet.

If I could use the coverage in a similar way, I could add the bins in the order that would make debugging easier, and have a simple switch to enable it, rather than have a more complicated setup that has several if..then statements to individually add bins based on config.

[JimLewis]

Ok. You feedback helps.

[SkydiverTricky]

I have an example from a real testbench

I have a design sending IP packets over AXI. These packets are formed in a packet gerator that has multiple flows (in my case ~30). I use the covPType to create a index to fetch a packet from a specific flow (as the IP address, UDP port etc have to be consistent when sending multiple packets on the same flow).

idx           := cov.randCovPoint;
pkt_ptr       := new byte_array_t'( stream_gen.get_next_packet(idx) );
send_axis_packet( pkt_ptr.all, pkt_drive_q );
....

cov.ICover(idx);

The UUT has a filter that is supposed to only send specific packets to a specific end point, but some packets are being incorrectly dropped. With the randomised indexing, it is harder to track down which packets are being incorrectly dropped so a pattern can be detected. The flows are configured so that the first half should go through and the 2nd half should be blocked.

I can currently work around this problem in this tesbench with:

idx           := cov.randCovPoint when G_RANDOM_COV else (pkt_cnt rem stream_gen.count_flows);

But this wont then obey any coverage limits set for each index.

[JimLewis]

Currently we have:

    impure function RandCovPoint return integer ;
    impure function RandCovPoint return integer_vector ;
    impure function GetPoint ( BinIndex : integer ) return integer ;
    impure function GetPoint ( BinIndex : integer ) return integer_vector ;
    impure function GetMinPoint return integer ;
    impure function GetMinPoint return integer_vector ;

As a first step we could add GetIncPoint which updates LastIndex to (LastIndex mod NumBins) + 1 and then does a GetPoint(LastIndex). Note the funny increment of LastIndex is because bins are numbered from 1 to NumBins.

    impure function GetIncPoint return integer ;
    impure function GetIncPoint return integer_vector ;

Then we could add a GetNextPoint(Mode) which does:

impure function GetNextPoint(Mode : ) return integer/integer_vector is
begin
  case Mode is 
    when Random => return RandCovPoint ; 
    when Increment => return GetIncPoint ;
    when Minimum => return GetMinPoint ; 
  end case ;
end function GetNextPoint;

GetIncPoint has the same flaws you pointed out with your current methodology, it ignores the current coverage of the bins in a trade for simplicity. GetMinPoint returns the bin with the minimum coverage which returns the first bin with the minimum percent coverage - which is one way to get an incrementing pattern when the coverage goals are different.

What this also points out is that the naming for RandCovPoint is inconsistent. Likelyhood in the future there will be a GetRandPoint that duplicates it and with the intent of slowly deprecating RandCovPoint. I use the word deprecate lightly as unless there is a compelling reason, there is no reason to break old code.

If we do this does it solve the problem?

[JimLewis]

Released in 2020.05