Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Created common log interface for LocalPrint. #81

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

LarsAsplund
Copy link

This PR is a start for using the common log interface in AlertLogPkg as discussed in #80. It serves as a proof of concept and to see if the common log interface could work for OSVVM. I've focused on replacing LocalPrint procedure with a call to WriteToLog since most log entries goes through that procedure.

I think there are additional places in AlertLogPkg that can be considered as logging and should be refactored to use WriteToLog but I leave that for you to complete :-).

I realized that passing the path of the log destination, in case it is a file, to WriteToLog will make things easier on the implementation of that procedure so I added that to the interface.

The changes were made rather quickly and I see that there is potential for some cleaning.

Obviously the code has to be tested in the test suites to see that I didn't make any mistakes. I've only made visual inspections of a few Log and Alert calls.

@LarsAsplund
Copy link
Author

LarsAsplund commented Jul 15, 2023

The updates to the common log interfaces were integrated into VUnit and additional tests of piping logs between VUnit and OSVVM show that it works. Differences such as one transcript file vs many separate files are manageable. OSVVM transcript logs piped into VUnit will continue to be in a common file on VUnit side. If VUnit logs using different files are piped to OSVVM they will all be merged in the transcript file. @JimLewis @Paebbels Do you see any issues preventing this to be integrated into OSVVM?

@JimLewis JimLewis changed the base branch from main to dev July 15, 2023 20:48
@JimLewis
Copy link
Member

JimLewis commented Jul 15, 2023

Hi Lars,
I am not sold on this approach. I see two issues. This make OSVVM printing less efficient and it fails to address the bigger issue - reports and reporting errors.

With OSVVM's LocalPrint, when there is mirroring, the string is created one time and then written to the two separate files. With your proposed change, we have to create the string twice. Not excited about doing a change that makes the code less efficient.

I see reports and reporting errors as the biggest issue. This does not address that recording errors in the active system - at least from the OSVVM side of things.

Printing consistently is a good. However, it is not going to be exact as the two systems have differences in justification.

I think the right place to make the adjustment is an outer layer that abstracts the type (AlertLogIDType for OSVVM) and the basic API, for OSVVM this is NewID, Alert, and Log. Mapping things like these would then allow people to report through one system.

Jim

@LarsAsplund
Copy link
Author

With OSVVM's LocalPrint, when there is mirroring, the string is created one time and then written to the two separate files. With your proposed change, we have to create the string twice. Not excited about doing a change that makes the code less efficient.

When it comes to performance there are three things to consider:

  1. I consider this a quick prototype PR that will need adjustments before release. For example, I simply copy and pasted the actuals to the two WriteToLogs calls. An optimized approach would use a local variable/constant and only compute it once.
  2. OSVVM only needs one call with its mirroring approach. That is why I added the IsOriginalPkg to CommonLogPkg such that the native implementation can make optimizations. In the suggested implementation the branch calling WriteToLog with a file is disabled by an if statement (if (not IsOriginalPkg) and IsTranscriptOpen then) in case the native OSVVM implementation is used.
  3. When adding this to VUnit I noticed a slight performance degradation of slightly below 10%. I haven't investigated why but I guess the extra procedure call is part of that (in the OSVVM case there is no extra call since WriteToLog replaces LocalPrint). There are optimizations that can be made but so far I haven't bothered. 1 million log entries take a few seconds so I don't feel this is a practical performance issue. That is when nothing is fed to the display. Most users have logs on the display and in that case the performance is I/O limited and drops a few magnitudes.

I see reports and reporting errors as the biggest issue. This does not address that recording errors in the active system - at least from the OSVVM side of things.

I may have sold this idea poorly. While the lowest level of logging alignment is on the log message style/formatting, the input parameters to WriteToLog allow for much more alignment than that. In both my prototypes for piping logs to VUnit and OSVVM I've used the public API of the frameworks I pipe to. This means that the target framework doesn't see any difference in these log entries and log entries coming directly from the user. This means that things like counting works as expected. For example, here is the result of two errors, one from VUnit and one from OSVVM, when the logging systems are separated:

image

Now, the same code when piping VUnit logs to OSVVM:

image

As you can see OSVVM is fully aware of VUnit logs so it's not just about printing.

This is how it works with the latest VUnit release and I think it is a very useful start which comes at no cost. I agree that we can have higher levels of abstractions but that brings us back to where we got stuck 5 years ago. This is a simple first step. If that turns out well we can start considering the next step.

@LarsAsplund
Copy link
Author

I updated such that LocalPrint is used when IsOriginalPkg is set true. This means that the OSVVM-native implementation is exactly as before. Since IsOriginalPkg is a constant the if statement will be optimized away and there cannot be any performance issues in that case.

In addition, OSVVM does not have to implement their own WriteToLog. The compiler may require its presence but the body can be empty. This is what I provided.

The non-impact of this PR is also very easy to analyze if you look at the diff.

Since the common log interface isn't used for the OSVVM-native implementation you would need some extra tests though to verify that the interface is properly invoked when IsOriginalPkg is set false. That should be easy though.

@LarsAsplund
Copy link
Author

Added some missing pieces. From my point of view this PR is starting to look complete.

The only thing I'm considering is to remove the "no-name" string, integer and boolean parameters. If there is going to be interfaces that differ in casing you cannot create a single reusable implementation to convert from any framework to framework X anyway. There need to be at least two, one for each casing style. Only the named parameters are mandatory. The casing can be whatever you prefer but types and default values must be the same. If you need additional parameters for your native implementation they can have any name and and casing but I would like to restrict types to standard IEEE VHDL-93 types. We want to keep things lightweight with few dependencies. No custom record types etc.

@LarsAsplund
Copy link
Author

@JimLewis @Paebbels Any comments on this one?

1 similar comment
@LarsAsplund
Copy link
Author

@JimLewis @Paebbels Any comments on this one?

@JimLewis
Copy link
Member

JimLewis commented Jul 31, 2023

Hi Lars,
Your example seems to show that OSVVM is some how able to count VUnit errors. How is this happening? Is there something in the pull request that does this?

From an OSVVM perspective, if we do not get error tracking in the OSVVM alert log data structure, then we don't get the information in the reports. As a result, I think the implementation of this sort of capability is done at the wrong level. I think it needs to be done at an outer layer instead of an inner layer. IE: have common log package provide the interface for NewID, Alert, and Log - and have the rest of the capability call those features through them.

Jim

@LarsAsplund
Copy link
Author

No, the PR is just a refactoring to expose the common log interface that allow someone to create an alternative implementation that pipes OSVVM logs elsewhere.

If you want to pipe VUnit logs into OSVVM, then you need to provide an alternative implementation on the VUnit side.

I've not "formally" released such an implementation and generally speaking it doesn't make sense for projects like ours to create and maintain alternative implementations for other frameworks.

However, OSVVM is a bit special since we ship it with VUnit. I'm thinking that I will eventually add an argument to our add_osvvm method that allow users to redirect VUnit logs to OSVVM. In the examples I've provided a prototype for that: https://github.com/VUnit/vunit/blob/master/examples/vhdl/osvvm_log_integration/osvvm_integration/vunit_to_osvvm_common_log_pkg-body.vhd. That is the source of the examples I provided. I think it needs a bit more work before making it more official though.

I don't think the interface should be seen as an inner layer. It's a low-level interface in order to achieve something without the more extensive integration we failed with in the past. In fact, the interface consists of parameters that are representations of the public APIs. As such it can be seen as the outermost layer. When I pipe VUnit logs to OSVVM I simply use the public API for OSVVM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants