techyian / MMALSharp

C# wrapper to Broadcom's MMAL with an API to the Raspberry Pi camera.
MIT License
195 stars 33 forks source link

Abstract logging to interface #98

Closed techyian closed 4 years ago

techyian commented 4 years ago

MMALSharp should not dictate to using NLog for its logging and should provide an interface to allow users to decide their own logging facility. Will investigate net-commons/common-logging.

OriNachum commented 4 years ago

I guess it relates to this. Had to move from master to dev for timelapse pictures on .NET Core 3.1 console app, and had to locally update dev version to 3.1.0 (From 2.7.0) Also had issues with System.Drawing.Common (4.7.0)

(And also - kudos! Works well on my Raspberry Pi 3 Model B)

techyian commented 4 years ago

Hi!

Yes, the timelapse pictures should be working on the dev branch. Sorry I don't quite understand:

and had to locally update dev version to 3.1.0 (From 2.7.0)

What exactly did you update from 2.7.0?

Regarding System.Drawing.Common, what issues do you get? I've read that GDI functionality can be added with the following libraries but haven't tried myself yet!

libc6-dev 
libgdiplus

Happy to help out with any issues you come across - glad the majority of functionality is working for you. I'm working towards v0.6 (dev branch) and there's a few issues yet to iron out.

OriNachum commented 4 years ago

What exactly did you update from 2.7.0?

I updated Microsoft.Extensions.Logging on MMALSharp & MMALSharp.Common Additionally, I had to update Microsoft.Extensions.Logging.Abstraction on MMALSharp

Regarding System.Drawing.Common, what issues do you get?

It was .NET library mismatch issue. Updating to 4.7.0 opened it for dotnetcore3.1

Happy to help out with any issues you come across - glad the majority of functionality is working for you. I'm working towards v0.6 (dev branch) and there's a few issues yet to iron out.

Looking forward to it! You're doing a great job - let me know if I can help with code (Review/tasks)

kenssamson commented 4 years ago

Having an issue with new logging setup. One issue was that the different projects seem to be associated with different versions of Microsoft.Extensions.Logging. I solved this by adding v 3.1.0 to my project. The other issue I can't get pass is I get an exception when attempting to initialize the camera:

System.MissingMethodException: Method not found: 'Microsoft.Extensions.Logging.ILoggerFactory Microsoft.Extensions.Loggi ng.DebugLoggerFactoryExtensions.AddDebug(Microsoft.Extensions.Logging.ILoggerFactory)'. at MMALSharp.Common.Utility.MMALLog.ConfigureLogger(ILoggerFactory factory) at MMALSharp.Common.Utility.MMALLog.get_LoggerFactory() at MMALSharp.Common.Utility.MMALLog.get_Logger() at MMALSharp.PortExtensions.SetParameter(IPort port, Int32 key, Object value) at MMALSharp.Components.MMALCameraComponent..ctor() at MMALSharp.MMALCamera..ctor() at MMALSharp.MMALCamera.<>c.<.cctor>b__30_0() at System.Lazy'1.ViaFactory(LazyThreadSafetyMode mode) at System.Lazy'1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor) at System.Lazy'1.CreateValue() at System.Lazy'1.get_Value() at MMALSharp.MMALCamera.get_Instance()

I'm running ASP.NET Core 3.0 razor pages site that using SignalR to communicate with the camera. I've added NLog to my application and it appears to be working correctly. I've attached the log file my program creates. Any help would be appreciated. Also, please let me know if you have any questions.

mmal-log-2019-12-27.log

OriNachum commented 4 years ago

@kenssamson , I know it's not a real solution to suggest to change to another framework - but I strongly suggest trying Serilog instead of NLog. It's more modern, robust and easier to use.

About the issue - have you tried using NLog with Microsoft's logging and AddDebug method? It may be unrelated to this project.

techyian commented 4 years ago

Apologies for the issues you're having here. The version of Microsoft.Extensions.Logging.Abstractions which I've added against MMALSharp (2.2) isn't compatible with .NET Core 3.0. When updating it in the library to the latest version (3.1), the way I've configured the logging providers in Logging.cs isn't allowed anymore due to those methods being deprecated. It's opened up a can of worms really! @kenssamson this is why you're receiving the exception you mentioned.

It looks as though Microsoft.Extensions.Logging.Abstractions v3.1 requires you to use Microsoft's Dependency Injection library in order to configure it. There doesn't appear to be any other straight-forward way of doing so. I will have a look into this. I didn't really want to start adding DI just for this, and it feels wrong for me to be doing so in the library, however if that's the only way this is going to be resolved then that's what I'll have to do.

@OriNachum have you managed to get everything configured correctly in your cloned repo? If so, I'd be interested to see how you've approached this.

Please bare with me!

techyian commented 4 years ago

I've just pushed some changes to the dev branch. I've updated Microsoft.Extensions.Logging.Abstractions to 3.1. I've also removed Microsoft.Extensions.Logging. I think it was my understanding of how to configure these packages which was at fault mainly. Not tested against .NET Core 3.0, but I have against my .NET Core 2.2 app and it works OK.

@kenssamson please could you grab latest and test again for me?

OriNachum commented 4 years ago

@techyian following the previous message fixed all issues for me (I wasn't using NLog though, if it has any effect) So additional to your changes, I also updated System.Drawing.Common to 4.7.0

techyian commented 4 years ago

Thanks for confirming @OriNachum

kenssamson commented 4 years ago

I ended up going back to the last version before the change in logging. It's working for now and I'm working on issue in my code. I'll take a look at the changes and let you know.

I agree that it's not good to add DI to a class library. I've attached a work around I use that is supported in .NET Core 3.0. It's similar to your logging code with the addition of a public function for defining the logging factory.

ApplicationLogging.cs.txt

Basically, it has a static function EnableLogging that takes an ILoggerFactory as a parameter. The rest of code uses that ILoggerFactory.

In startup...

public void Configure(ILoggerFactory loggerFactory, ...) {
    ApplicationLogging.EnableLogging(loggerFactory);
    MMALLog.EnableLogging(loggerFactory);
}

When referencing the logger, I then use the NULL conditional operator to prevent any errors if logging has not been enabled....

class MyClass {
    private ILogger<MyClass> logger = ApplicationLogging.CreateLogger<MyClass>();

    void SomeRoutine() {
        logger?.LogInformation("...");   // no error if logging not "enabled"
    }
}

I feel this gives two bonuses:

Let me know what you think.

techyian commented 4 years ago

Thanks @kenssamson. Yes I like what you've done there. What I've settled with is to open up the LoggerFactory property so users can override the factory being used, that way you can decide whether you want to use NLog/Serilog or simply use Microsoft's own Logging system (just bring down Microsoft.Extensions.Logging into your client app). I've tested it in a .NET Core 3.1 app and it seems to work well.

Essentially what you can do is:

// Taken from https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-3.1#create-logs
var loggerFactory = LoggerFactory.Create(builder =>
{
    builder.AddConsole();
});

MMALLog.LoggerFactory = loggerFactory;

// Take picture/video etc...

I'm quite happy with how this is working now so I'll close this ticket off.

techyian commented 4 years ago

I raised a ticket over at the NLog abstraction project and have received some helpful comments about how I've implemented this. I think there's still some work to do before this is closed off.

techyian commented 4 years ago

Working as expected. Closing this ticket now.