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

Some features/fixes #6

Merged
merged 10 commits into from
Aug 3, 2018
Merged

Some features/fixes #6

merged 10 commits into from
Aug 3, 2018

Conversation

Fothsid
Copy link
Contributor

@Fothsid Fothsid commented Jul 31, 2018

P.S. Added feature to disable/enable logging because size of log after around 4 hours of playing was 400 MBs lol

if ((!DEV9Header.config.EnableLogging.Error && eType == TraceEventType.Error) ||
(!DEV9Header.config.EnableLogging.Verbose && eType == TraceEventType.Verbose) ||
(!DEV9Header.config.EnableLogging.Information && eType == TraceEventType.Information))
return;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, logging is configured with LogInit() in DEV9.cs

private static void LogInit()
Which currently does so based on build type (Debug or Release)

Note that LogInit() is called before the configuration file have been loaded, I'm not sure what the best option to resolve this is.

You can call SetStdOutLevel(), SetStdErrLevel() & SetFileLevel() to configure global restrictions for each output. Any log levels set in SetStdOutLevel() must be excluded inSetStdErrLevel() or else you will get duplicate messages.

Also you must allow TraceEventType.Critical to log to stderr and the log file, this level is only used here in the event of a fatal error.

public static void MsgBoxErrorTrapper(Exception e)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure how I should handle this, because as you said, LogInit() is called before the configuration file have been loaded.
When I made this, I was thinking only about how to stop these logs, I was not even thinking about build types, maybe because I can't find a way to select Release or Debug type in VS Code 0_o

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used VSCode much myself, but I've made build tasks that should allow you to create release builds. (You will need to open the solution folder instead of the project folder is vscode to use the tasks)

You could either move any calls to ConfigFile.LoadConf() before any LogInit() (by moving ConfigFile.LoadConf() from Open() to Init() and reordering the calls in Config()) or by splitting LogInit() into two functions with Initialise logging with reasonable defaults before a second function is called to set logging levels as desired.

Ideally I would like to be able to enable Verbose logging on a per source level as just one source can produce a large amount entries, although the need for verbose logs doesn't come up often.

[DataMember]
public bool Error = true;
[DataMember]
public bool Verbose = true;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verbose should default to false to match release build behaviour.

@@ -47,6 +51,7 @@ private void Init()
HddEnable = false;
DirectConnectionSettings = new ConfigDirectIP();
SocketConnectionSettings = new ConfigSocketIP();
EnableLogging = new ConfigLogging();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do ConfigLogging's defaults apply correctly when loading an older config file (that lacks the entry)?

I remember having issues relating to that in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If config file lacks the entry, it applies default values. (at least on my machine)
However, if you manually add required entries, it still applies default values until you let it recreate config file. (maybe I just did it in wrong order?)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the entries in the file have to be in a specific order for settings to be read. I'm only concerned about updating an old file, and that seems to work.

if ( homedir == NULL ) {
homedir = getpwuid(getuid())->pw_dir;
}
coreClrFolder = string(homedir) + string("/.config/PCSX2/coreclr");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLR_DEV9_CORE.dll is currently found bases on the path passed to DEV9setSettingsDir while this assumes it's always in the home directory (it can be configured otherwise) Both should probably be consistent to avoid any confusion if a non-default settings path is encountered.

@TheLastRar
Copy link
Owner

A Release build should disable most logging that ends up in the log file, although having this configurable would be nice

Since SDL2 is one of the PCSX2's dependencies, I think this is a pretty good solution, at least for Linux
systems.
Should work on Windows too.
… loaded.

 - Moved all RELEASE build logging configurations after config file is loaded.
 - RELEASE build logging got enable/disable feature for each source source level through
config file
@@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using CLRDEV9;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the reference to CLRDEV9 as it's not needed.

return SDL_ShowSimpleMessageBox((uint)flags, title, message, new IntPtr(0));
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Newline at end of file

@@ -7,6 +7,7 @@
#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <pwd.h>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove reference if this is no-longer required.

Copy link
Owner

@TheLastRar TheLastRar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Unless there is something else you want to add I will squash and merge this.

@Fothsid
Copy link
Contributor Author

Fothsid commented Aug 3, 2018

That's all I wanted to add xD

@TheLastRar TheLastRar merged commit b85c61c into TheLastRar:master Aug 3, 2018
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.

2 participants