logging, avoiding exit, i2c mock fallback

[WayneKeenan]

Hiya, this is more of a query and perhaps start of a discussion.

I've embedded a modified version of the explorer library in PiCraftZero.

I've included a diff below to highlight the details, although it's not intended as a PR or patch as it is.

In summary the tweaks are:

• use standard logging
• avoid using exit() on errors - a requirement to co-exist with other code and to do graceful fallback
• fallback to a SMBus mock - for dev&testing on non-Pi platforms.
• a i2c detection failure catch

It would be nice to get all these into explorer hat library in some way, perhaps as injectable/configurable overrides in some cases, instead of having to import my bootlegged version in user scripts for example.

The diff:

diff -r explorerhat/__init__.py /Users/wayne/local.projects/PiCraft/python/src/picraftzero/thirdparty/pimoroni/explorerhat/__init__.py
8a9,10
> import logging
> logger = logging.getLogger(__name__)
14c16
<         exit("This library requires python-smbus\nInstall with: sudo apt-get install python-smbus")
---
>         logger.warning("This library requires python-smbus. Install with: sudo apt-get install python-smbus")
16c18
<         exit("This library requires python3-smbus\nInstall with: sudo apt-get install python3-smbus")
---
>         logger.warning("This library requires python3-smbus. Install with: sudo apt-get install python3-smbus")
21,22c23,24
<     exit("This library requires the RPi.GPIO module\nInstall with: sudo pip install RPi.GPIO")
< 
---
>     logger.warning("This library requires the RPi.GPIO module. Install with: sudo pip install RPi.GPIO")
>     from picraftzero.thirdparty.mocks.raspiberrypi.rpidevmocks import MockGPIO as GPIO
26c28
<     exit("This library requires the cap1xxx module\nInstall with: sudo pip install cap1xxx")
---
>     logger.warning("This library requires the cap1xxx module. Install with: sudo pip install cap1xxx")
683c685
<     print("\nExplorer HAT exiting cleanly, please wait...")
---
>     logger.info("\nExplorer HAT exiting cleanly, please wait...")
685c687
<     print("Stopping flashy things...")
---
>     logger.debug("Stopping flashy things...")
691c693
<     print("Stopping user tasks...")
---
>     logger.debug("Stopping user tasks...")
694c696
<     print("Cleaning up...")
---
>     logger.debug("Cleaning up...")
697c699
<     print("Goodbye!")
---
>     #print("Goodbye!")
705c707
< except IOError:
---
> except (IOError, NameError):
715c717
<     print("Explorer HAT Pro detected...")
---
>     logger.info("Explorer HAT Pro detected...")
719c721
<     print("Explorer HAT Basic detected...")
---
>     logger.info("Explorer HAT Basic detected...")
722c724
<     print("Explorer pHAT detected...")
---
>     logger.info("Explorer pHAT detected...")
726c728
<     exit("Warning, could not find Analog or Touch...\nPlease check your i2c settings!")
---
>     logger.warn("Warning, could not find Analog or Touch...\nPlease check your i2c settings!")
Only in /Users/wayne/local.projects/PiCraft/python/src/picraftzero/thirdparty/pimoroni/explorerhat: __pycache__
diff -r explorerhat/ads1015.py /Users/wayne/local.projects/PiCraft/python/src/picraftzero/thirdparty/pimoroni/explorerhat/ads1015.py
3a4,7
> import logging
> logger = logging.getLogger(__name__)
> 
> 
8c12
<         exit("This library requires python-smbus\nInstall with: sudo apt-get install python-smbus")
---
>         logger.warning("Falling back to mock SMBus. This library requires python-smbus\nInstall with: sudo apt-get install python-smbus")
10c14
<         exit("This library requires python3-smbus\nInstall with: sudo apt-get install python3-smbus")
---
>         logger.warning("Falling back to mock SMBus. This library requires python3-smbus\nInstall with: sudo apt-get install python3-smbus")
11a16,17
>         from picraftzero.thirdparty.mocks.raspiberrypi.rpidevmocks import Mock_smbusModule
>         SMBus = Mock_smbusModule.SMBus
17,18c23,28
<     revision = ([l[12:-1] for l in open('/proc/cpuinfo', 'r').readlines() if l[:8] == "Revision"] + ['0000'])[0]
<     return 1 if int(revision, 16) >= 4 else 0
---
>     try:
>         revision = ([l[12:-1] for l in open('/proc/cpuinfo', 'r').readlines() if l[:8] == "Revision"] + ['0000'])[0]
>         return 1 if int(revision, 16) >= 4 else 0
>     except FileNotFoundError:
>         logger.warning("Attempt to autodetect i2c bus failed, defaulting to 1")
>         return 1
diff -r explorerhat/pins.py /Users/wayne/local.projects/PiCraft/python/src/picraftzero/thirdparty/pimoroni/explorerhat/pins.py
10c10
<         threading.Thread.__init__(self)
---
>         threading.Thread.__init__(self, name= __class__)

[WayneKeenan]

I just replaced the exit's with warnings, but raising an Exception might be a better design.

[WayneKeenan]

For the i2c fallback nobody would want the library to depend on picraftzero.thirdparty.mocks.raspiberrypi.rpidevmocks or the rpidevmocks PiCraftZero uses.

Perhaps this library could do it's default behaviour unless an optional i2c_provider option was passed to the constructors/init or a module global?

[Gadgetoid]

Raising an Exception is something I've considered, instead of printing out an explicit message, since it's technically the right way to handle these cases.

Unfortunately, beginners simply do not read Exception messages, or are incapable of parsing out the bit that matters - the exception text - from the backtrace that Python vomits up. Explicitly printing out a concise error is a compromise and an attempt to produce more friendly error output for beginners.

It's a shame there's no way to pass a module some state before importing it, to switch into "expert mode" for more complex projects relying on our libraries.

All of our libraries use an instantiate-on-import singleton pattern which is fantastic for beginners but horrible for building them into more complex projects. So far I haven't found a satisfactory way to serve both needs.

[WayneKeenan]

How about reading an environment var at import time at the top of the the explorer module which sets an internal flag for:

1. conditionally executing the instantiate-on-import singleton pattern
2. determining if errors should be printed or raised
3. determining if a logger or print should be used

There could be 2 new internal functions, e.g. log and error in place of print and exit respectively that did the flag check in single place, otherwise it's most messy.

This could be a single 'master switch' env var and without the env var set by default the library would do the existing behaviour.
We could call this env var 'WAYNES_WORLD', but that's not precious to me. :)

If I put a PR together and you provided an ENV var name, would you kindly take a look?

EDIT: I'm also looking at the same for the pan/tilt module too. (To start with :) )

[WayneKeenan]

the i2c part is another matter, thats a bit more involved and potentially need to look at i2c factories with at least 2 different implementations (one being as it is now), also set by env var. Akin to how gpiozero uses pin factories.

[WayneKeenan]

Perhaps split the library internally into a core api for 'advanced' usage and use the core to (re)create the existing 'beginner' api.

[Gadgetoid]

Note: I've created PRs against most of our libraries (including this one: https://github.com/pimoroni/explorer-hat/pull/36) that- along with not doing anything silly at import time- change them to simply raising an exception in an import error condition. I made a lot of concessions to hand-hold beginners with our libraries, and may have pushed a bit too far- breaking some Python fundamentals in the process. I'm keen to rectify that.

Hopefully this will make your life easier!