Consistent naming, minor additions

[riedel-ferringer]

suggestion to make naming more clear and consistent: always use actual/expected instead of L/R
minor hopefully useful addions to the ScoreBoard and TbUtilPkg

[Paebbels]

Thanks for you proposals.

@JimLewis please have a look.

[JimLewis]

Parameters named L and R are consistent with names in the LRM for built in operators.

I believe Actual and Expected within the package will improve the package. In the printed messages, I think people reading reports relate better to received and expected. So if we are going to make a change, I think it should print " Received = Expected, Received = " ... Expected =. Not ok with abbreviating.

From a regression stand point, changes like this are significant and disruptive. For example, in the OSVVM regression of the AlertLogPkg, we have to check that specific outputs are generated. All of those checks then will need to be reverified. Alot of that is manual work.

It may cause similar regression failures elsewhere.

Not saying we should not do it. Saying we should do it once and only once.

[JimLewis]

It may be appropriate to use JoinStr here to add the space before the regular printing so that the regular printing does not have the extra space unless they are necessary.

[JimLewis]

I think maybe we need to discuss this.

Actual and Expected are the names used by the scoreboard package. Many users think in terms of received and expected. Hence, that is a lesson learned over time and AlertLog came along long after, so it used different naming.

WRT, AlertIf vs AffirmIf. AffirmIf is intended for self checking. There is a received/actual and a expected.

AlertIf started out as general purpose - ie: used for parameter and self-checking, but with the addition of AffirmIf, its intent is parameter checking. We might be checking that A'length matches B'length. This is not a received/actual vs expected type of check.

So I think we need to pause and maybe talk about the proposed changes. GTM?

[riedel-ferringer]

Whether 'actual' or 'received' does not matter to me. I was always confused by L/R because I just didn't know which is which. While it doesn't really matter for practical reasons, it's a question of consistency.

However, I understand that there are implications here, probably also for users who rely on specific output strings.

[riedel-ferringer]

After reading your last comment, is it agreed to use "received" / "expected" everywhere? If so, I will update the PR accordingly.

[Paebbels]

I'm ok with both variants as long as it's not R/L.
Received is a special case of actual.
Personally I would prefer actual/expected as it's also used in other frameworks or in OSVVM's scoreboard.

But the final decision is up to @JimLewis :).

[JimLewis]

@Paebbels
The intended use model for AffirmIf is to cover all uses cases of self checking. This is where actual/expected or received/expected make sense.

The intended use model for AlertIfEqual/AlertIfNotEqual is for parameter checking. It is not clear to me how actual/expected or received/expected make any sense in the above parameter check example above. Let me be honest, I don't particularly like L and R, however, give me something that works. You can see that they do not work for my example above right?

[JimLewis]

For AlertIfEqual we could say,
Parameters expected to be not equal, Param1 = ... Param2 = ....

For AlertIfNotEqual we could say,
Parameters expected to be equal, Param1 = ... Param2 = ....

[JimLewis]

From an OSVVM verification suite perspective, the impact of changing AlertIfEqual/AlertIfNotEqual is relatively minor.

OTOH, changing AffirmIfEqual will require significant more work in updating the checking. So it would be a low priority update relative to other tasks that need to be done.

[riedel-ferringer]

I agree that actual/expected (or whatever) does not make sense for certain use-models of AlertIfEqual. However, the same would be true for AffirmIfEqual. There might be situation where you don't compare against an expected value, but against another 'actual'. Even if this is not the intended use model, it will be used (well, I certainly do). And vice versa, I also use AlertIfEqual to check actuals vs. expected values sometimes. Using param1, param2 does not, imho, solve the problem of being ambigous. I think that more often than not, you compare something against an expected value. And if not, it doesn't matter. I don't think that in your MUX example, using 'actual'/'expected' would make the results less clear.

[JimLewis]

Purpose? Use model?

Not for or against, just want to understand. Also note that the internal Error and Check counts are there because the implementation pre-dated the AlertLogPkg. I suspect that a future implementation of the ScoreboardPkg that is just a singleton implementation will use only the AlertLogPkg capability, so we need to understand why here so we understand the design considerations for the singleton.

You only added it to the protected type API. I don't see anything that adds it to the Singleton API.

[riedel-ferringer]

I missed the singleton API, because when I started I think there only was the protected type, so I simply didn't realize it.

I have a testbench for a Verification Unit of the Avalon Bus, and I am testing the VU with different parameters (readLatency, readWaitTime, etc). I always send a couple of data and use a scoreboard to check them. When done, I've written a procedure that prints a summary (checked vs failed). I then clear the counters and reuse the scoreboard for the next run with a different avalon-setting.

I think the internal check count is very important, because if we only count error, you wouldn't notice if nothing had been checked at all. I guess the AlertLogPkgs CheckCount is not that well suited for such a check.

[JimLewis]

One of the next planned additions to reporting is scoreboard reporting. For the predefined scoreboards this will be automatic. However, it will only be automatic for the Singleton API.

One of the things you would be able to do then is for each phase of your test, get a new scoreboardID:

SB := NewID("SB:Phase1") ; 
. . . 
SB := NewID("SB:Phase2") ; 

As long as you finish each phase with the scoreboard empty, it is ok to just leave the old one behind.
The reporting will be automatic for each ID in the structure.

For a limited set of scoreboards, this would be ok. For a large number of scoreboards, I would need
to check to see what the nominal amount of storage a scoreboard ID uses. It may be very little -
ie: just the counters and this would be ok.

Each new scoreboard ID has its own AlertLogID. Requirements Bins currently setting a Passed Goal.
If this is not allowed for a regular AlertLogID, it needs to be extended to do so. And then all the
checking would be automatic.

[riedel-ferringer]

As long as you finish each phase with the scoreboard empty, it is ok to just leave the old one behind.
The reporting will be automatic for each ID in the structure.

Just to clarify things (and I understand this is not the actual topic here): You will automatically create a summary report whenever a new ID is created and the last scoreboard before the newly created one is empty? Or will you always create the report once the scorebord becomes empty? What then, do you deallocate the just-reported scoreboard to free up memory and make the slot available again (which is probably a bad idea because of duplicate IDs)?

[JimLewis]

The plan is to do the same thing that is done in the Coverage data structure. When the test completes and the test calls EndOfTestReports (instead of ReportAlerts). If you look at ReportPkg.EndOfTestReports for coverage it does. So if there is coverage, then walk the Coverage singleton, find each coverage model and produce a report for it. Then there is TCL code that converts the coverage model to HTML that runs when simulate completes.

    if GotCoverage then 
      WriteCovYaml (
        FileName     => "./reports/" & GetAlertLogName & "_cov.yml"
      ) ;
    end if ; 

So if you look below the above code, you will see the following commented out. So what we need is a GotScoreboard (which basically checks to see if there are any scoreboards in the singleton), and a WriteScoreboardYaml, which writes out elements of the Scoreboard that we need for reporting - all the count stuff and such, and then the corresponding TCL code to convert the Yaml to HTML. It will not be too hard, mostly templating off the code already done in the CoveragePkg. The only hard part is working out the Yaml format and what to report. Should not take too long. Just need to work it into the schedule.

--    if work.ScoreboardPkg_slv.GotScoreboard then 
--       work.ScoreboardPkg_slv.WriteScoreboardYaml ;
--    end if ; 
--
--    if work.ScoreboardPkg_int.GotScoreboard then 
--       work.ScoreboardPkg_int.WriteScoreboardYaml ;
--    end if ; 

[JimLewis]

Currently we do not delete/deallocate items in the singletons. That is something I am currently working on and expect to be in one of the next two releases.

[JimLewis]

I am still seeing AckTime in the new WaitForTransaction Procedures. Is it really there or is it just a feature of GitHub?

[JimLewis]

I think maybe we need to discuss this.

Actual and Expected are the names used by the scoreboard package. Many users think in terms of received and expected. Hence, that is a lesson learned over time and AlertLog came along long after, so it used different naming.

WRT, AlertIf vs AffirmIf. AffirmIf is intended for self checking. There is a received/actual and a expected.

AlertIf started out as general purpose - ie: used for parameter and self-checking, but with the addition of AffirmIf, its intent is parameter checking. We might be checking that A'length matches B'length. This is not a received/actual vs expected type of check.

So I think we need to pause and maybe talk about the proposed changes. GTM?

[JimLewis]

@riedel-ferringer
First, thanks for the pull request.

Can you submit the changes to ScoreboardGenericPkg and TbUtilPkg separately from AlertLogPkg. I can merge those. We need to talk about the changes to AlertLogPkg.

Also can you do your pull requests against Dev rather than Master. I need to test them against my test cases before releasing it to Master.

[JimLewis]

I have accepted your changes for TbUtilPkg and ScoreboardGenericPkg.

I accepted the intent of your change for AlertLogPkg, PathTail. In its current form it crashed GHDL, so I rewrote it using a variable instead.

WRT, ScoreboardGenericPkg, at this time, neither the other pull request nor I added SetCheckCountZero to the singleton. I did make it available to indexed scoreboards that use ScoreboardPType.

WRT, AlertLogPkg, and printing for AlertIfEqual and AlertIfNotEqual and using types that are arrays of std_ulogic/std_logic, I changed the to_string to to_hstring to be consistent. In the future, maybe we want to add a format specifier?

Currently only ScoreboardGenericPkg uses the word Actual and it only uses it for the type. When printing, it prints "Received". So have is currently printed by AffirmIfEqual is consistent with what the scoreboard prints. If we do a consistency update, it would be to change the Scoreboard parameter to ReceivedType. Unlikely it will change for ScoreboardPType, however, ...

I have been pondering refactoring a new Singleton Scoreboard that has the subprogram generic AffirmIfEqual instead of Match, to_expected_string and to_actual_string. That will allow people to control whether Received and Expected are only printed on FAILURE or also during PASSED (requested by the DO-254 folks). If this happens, having local counts for Error and Passed will not be possible, however, we will be able to fetch them and emulating clearing them (by logging their previous value and subtracting). The question would be at that time, would we need API commands to get the count for the entire test as well as the count for one segment of the test (after clearing). This is the reason I did not make those values visible in the Singleton API just yet. If this one does get created, it would be appropriate to change its ActualType to ReceivedType.

[Paebbels]

@riedel-ferringer Jim and I did some branch cleanups like master → main. We also renamed Dev → dev. This by accident closed this PR.

@JimLewis can you try with maintainer rights if you can pickup this branch/PR and change the source branch name?

[Paebbels]

@riedel-ferringer is it correct that after your last force-push the branch has no diff?

[riedel-ferringer]

I am not sure if we yet reached a conclusion here? I can provide an updated version of AlertLogPkg.vhd (as plenty has changed and there are merge conflicts) according to whatever naming scheme you prefer.

The change requests are fixed, at least as far as I can tell.

[riedel-ferringer]

@riedel-ferringer is it correct that after your last force-push the branch has no diff?

Yes it is correct. I accidentially merged into the wrong branch and did not realize that it was the one connected to the PR.

[JimLewis]

AlertIfEqual uses L and R because the target of AleritIf is for protocol checkers which only report errors and are not part of self checking. So while L and R are not that descriptive, using Actual and Expected or Received and Expected is not an exact thing either.

AffirmIfEqual uses received and expected. At least in English, what is meant by received is better understood than actual.

It is noted that this is inconsistent with what the Scoreboard uses - actual and expected. However, I expect the Scoreboard to address this in the future (due to other planned changes). The current Scoreboard falls short of the communities need in flexibility of the message printing. I have plans to address this with a future addition to the ScoreboardGenericPkg family.