-
Notifications
You must be signed in to change notification settings - Fork 56
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
base: dev
Are you sure you want to change the base?
Conversation
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? |
Hi Lars, 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 |
When it comes to performance there are three things to consider:
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 Now, the same code when piping VUnit logs to OSVVM: 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. |
I updated such that In addition, OSVVM does not have to implement their own 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 |
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. |
1 similar comment
Hi Lars, 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 |
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 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. |
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 replacingLocalPrint
procedure with a call toWriteToLog
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 useWriteToLog
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
andAlert
calls.