use NullHandler for logging by default

[vladak]

This change makes sure the self.logger is always set to something. Given that enable_logger() has the flexibility of specifying the logging package, the loggers are switched, as opposed to handler juggling which might not be a good idea anyway as multiple log packages can be used simultaneously.

[tekktrik]

This looks good to me, and seems functionally the same. My only concern would be memory issues (e.g., adafruit_logging takes up precious RAM needed by a board). It may be worth seeing if this works on an M0 board. @brentru, was that a concern you had when making the library?

[vladak]

That's interesting and I have not considered that. Is there perhaps some tooling that would help with memory analysis ?

[FoamyGuy]

Can you expand a bit on why the way it's done today with the None check is not a good idea? I don't know what you meant by handler juggling.

This does seem to me like it would be functionally the same, but at the "cost" of extra RAM and the additional dependency needing to be loaded.

I don't quite understand why having the NullHandler pre-set by default like this preferable to having it be None and using the if statements to determine whether it should try to log or not.

I don't think I have an M0 device that I could hook up with airlift so I can't test that. Honestly though I would'nt be too surprised if it was already tough to get networking and minimqtt both imported and working even without expanding the RAM needed here. It's pretty tight already and the networking libraries are on the bigger side I think

[vladak]

It's a readability issue and I'd hope that the code changes make it apparent. Currently in the code there are 32 checks that determine whether self.logger is set to None so that the logger can be actually used. It is also a stability issue - when I was doing some changes to the code recently, I had to be carefull to carry the logger is not None checks around and also make sure that any new logger use is underneath the check. If I was not careful with keeping the check for all the cases where logger is used, then it might not be caught in my cursory testing that had the logger always set to something. It's similar (pattern wise) to the isLoggable() check done in Java logging to avoid unnecessary CPU churn if the log level in effect does not match the log level of the log statement, however in the case of the None check it results in NoneType exception rather than slower code.

As for the handler juggling, it was alternative idea when I considered the possibilities of addressing the readability issue. Basically have a single logger with NullHandler and replace it with handlers from the logger passed to enable_logger() and reverse the operation in disable_logger(). This approach also needs a logging module imported, obviously, is more complicated than necessary and does not play well with multiple distinct logging packages (say adafruit logging with CPython logging).

Another possibility would be to wrap this inside a function.

[dhalbert]

Currently in the code there are 32 checks that determine whether self.logger is set to None

I agree that makes for a mess.

Another possibility would be to wrap this inside a function.

That seems like the simplest solution that doesn't import a logging package when it isn't needed. self.log_debug() and self.log_info() would do the None checks. Maybe you could use a single self.log() function because the levels (INFO, DEBUG, etc.) wouldn't be defined.

Another idea would be to create an adafruit_null_logging package that doesn't do anything except define the levels and use that to initialize the logger. Then the memory overhead is small. If real logging is needed, the real package would be imported. That makes for double definitions of the levels, so that's a little wasted space. I don't know if adafruit_logging could be configured to do lazy importing internally to achieve approximately the same effect.

[dhalbert]

This looks good to me (once it builds). @tekktrik @FoamyGuy What do you think?

[tekktrik]

I think this is good as well. It's then probably worth adding a docstring to the logger attribute in a follow-up PR the logging option is documented.

[dhalbert]

I think this is good as well. It's then probably worth adding a docstring to the logger attribute in a follow-up PR the logging option is documented.

Not merged yet so could be added in this PR.

[tekktrik]

@vladak is that something you mind doing? Basically, for the logger attribute in __init__(), add a docstring beneath it documenting it like so:

    def __init___(self) -> None:
        self.logger = NullLogger()
        """An optional logging attribute that can be set with with a Logger to enable debug logging."""

Feel free to change the description above, just an idea to communicate how.

[vladak]

@vladak is that something you mind doing?

No issue other than my sleep schedule :-)

[tekktrik]

Thanks!