Refactor into a Class

[kizniche]

This is the PR for issue #51 where I discuss how I've refactored the code into a class and added the ability to install via pip. This allows easier integration into software as a library. This also brings the repo closer to having a package in pypi.org to allow an even easier install via pip. I haven't thoroughly tested every aspect of the code, but wanted to create a PR for review since @David00 showed an interest in potentially merging the changes.

Note: currently the README install instructions include:

./venv/bin/python -m pip install git+https://github.com/David00/rpi-power-monitor.git

However, to test, you should change to my repo:

./venv/bin/python -m pip install git+https://github.com/kizniche/rpi-power-monitor.git

Cheers, Kyle

[David00]

Thank you @kizniche. I should be able to review this in depth next week.

[David00]

I am reviewing this now.

[kizniche]

I can also work on any changes you think should be made. Just let me know what you think and if you see any issues, incompatibilities, etc.

[David00]

I just pushed a commit on top of your PR containing a few small changes from my review.

See: https://github.com/David00/rpi-power-monitor/pull/62/commits/134a253b49ba9164f539413a1db0f38c33ca3d17

The commit message at the top shows a summary of changes, but it's all minor stuff and general code quality improvements like completed docstrings (you know, the stuff that I probably should have had in there a long time ago :) )

There was one minor issue I noticed, which appeared to be a duplicate instantiation of the RPiPowerMonitor class. See this line in your last commit: https://github.com/kizniche/rpi-power-monitor/blob/62eeedad6f38e1ed0acd110d3caa2e72225d6ef5/rpi_power_monitor/power_monitor.py#L780

Was there a specific reason for making another instance here? I couldn't see any particular reason, but I wanted to ask just in case. I changed it in my PR Review commit.

I will still have some more work to do with going through the Wiki to update the manual installation instructions and also comb through the rest of the Wiki to make sure there aren't any references to the old filename (power-monitor.py vs power_monitor.py).

I'll also need to build a new CustomPiOS image with these changes and test it out. I have tested the PR on my Pi with my latest PCB and sensor hardware, and everything is good there, so I don't have any significant concerns about updating the CustomPiOS image build (other than the SPI speed issue on the v5 kernels which I'm working on in #64).

Aside from that, there are a couple other bigger changes that I want to make, but will save for another time. Primarily, I want to move the config values out of config.py and into a SQLite database so that updating the software with git pull doesn't overwrite people's config settings. Also, I might need to drop the py-spidev driver and build my own C extension to read the ADC, which would resolve #64. I also need to add retention policies and continuous queries to the InfluxDB database to allow for long-term data storage and comparisons. I'm just bringing them up here in case they have any impacts on working with your project (which I still need to check out, by the way!!)

Thanks again for contributing, and excellent work on the refactor!

[David00]

@kizniche, I'm updating my Wiki docs, and I think running this from a virtual env will add unnecessary complexity to the setup process (specifically, the calibration part which requires command-line interaction). Most folks that deploy my project use the Pi exclusively for this project, so I'm thinking of leaving the setup of a virtual environment to other projects that want to import it as a module.

[kizniche]

Was there a specific reason for making another instance here?

No, I think I just forgot to change that.

I think running this from a virtual env will add unnecessary complexity to the setup... I'm thinking of leaving the setup of a virtual environment to other projects that want to import it as a module.

I'm just used to always using virtual environments because they're reproducible and don't cause conflicts with whatever has been previously installed. Those that are familiar with them will use them regardless if they're in the docs or not, so so writing the docs for a basic setup is reasonable.

[David00]

I'm just used to always using virtual environments because they're reproducible and don't cause conflicts with whatever has been previously installed. Those that are familiar with them will use them regardless if they're in the docs or not, so so writing the docs for a basic setup is reasonable.

Oh, I don't disagree one bit. I use virtual environments extensively for all my projects too. But since I provide a customized Pi OS image, I'm basically controlling the whole environment anyways. :)

[David00]

Hey @kizniche, just wanted to bump this and let you know I'm still working on integrating and testing with my automated build process. I had some issues with dependencies changing along with the critical sampling rate issue in #64 that's taken a lot of time so far, but I haven't forgotten about this and should be able to merge this PR in a week or two.

[kizniche]

Sounds good. Thanks for the update. Let me know if I can be of any assistance.

[kizniche]

That sounds reasonable. How do you build your OS image? I'd like to do something like that for my software Mycodo, but don't have any experience.

[David00]

I use CustomPiOS (via the Docker method)

[David00]

Hi @kizniche, I merged this into master earlier, and am building a new image with the updates based on my repo instead of yours. I've also made a bunch of changes to the Wiki docs and readme to support the changing directories and filenames with this merge. The new image will be uploaded to the v0.2.0 tag assets once I'm finished building and testing it.

[David00]

Ok, the image is built and tested... and GitHub has apparently changed the file upload limit to 25MB (via the web UI, 50MB when using the CLI).

So, I had to setup Git LFS and add the image to a new branch to get it in my repo. You can find it on the release-v0.2.0 branch, or, as a direct link in the Wiki on step 1 here.

[David00]

@kizniche, would you like to have a section in my docs to highlight Mycodo? I am refactoring all my docs in preparation for a new release, and moving to a more polished GitHub Pages site. I am adding a page for Integrations with the intent to include things like Home Assistant and Mycodo, and others in the future.

[kizniche]

Sure, that would be great. You can find the Input module doc for Mycodo at https://kizniche.github.io/Mycodo/Supported-Inputs/#power-monitor-rpi-power-monitor-6-channels and I see I need to update the library location from my fork to your repo since you've merged the changes. I should also improve the description of how to use the Input and include possibly more links.

[David00]

Awesome, thanks! Would you like to write anything about your project to include in my docs? If not, I can just grab the summary from your home page:

Mycodo is open source software designed to run on the Raspberry Pi and other single-board computers (SBCs). It couples inputs and outputs in interesting ways to sense and manipulate the environment.

Also, a lot of the input values listed on your documentation will be changing in my upcoming release, and so is the overall setup process. I will let you know here when I create and push to develop-v0.3.0.

[kizniche]

That summary is good. You can mention measurements are logged and history can be visualized on graphs, but that is easily discovered on the Mycodo pages.

Have you explored adding your library to pypi.org? It makes getting the code very easy using pip. Also, having versions for pip would make it easy to set which version of your library to use with the specific Mycodo Input module version (or other software). Using a git repo with pip can sometimes cause issues if an update breaks functionality if it's not backwards compatible (if merely set to use the master branch). There's probably a way to specify a specific commit to avoid this issue, so I'll have to investigate that.

[David00]

Thanks. I have not looked into utilizing PyPi, but I think I am one step closer to being able to add my project there.

I'm still mainly invested in providing customized / prebuilt OS images, leaving manual installation as a secondary method for those that don't want to or can't use my image.