Python (and micropython) contribution guidelines on GitHub

Hi Everyone,

I am starting to look at the PiicoDev python libraries, mostly from the following location:

However, I note that you also have a repo for just the i2c unified driver and possibly some of the local scripts.

I tend to review libraries before I use them and can see some low-hanging fruit where I would be happy to put in a pull request (docstrings, type hinting, etc…).

Is the PyPI repo the main repo that I should send PRs to, or is it a consolidated repo for PyPI and instead I should send PRs elsewhere? I am assuming PRs are welcome, otherwise I am happy to write my own libs from scratch (I might need to, I want to use asyncio for a little framework I am building so that my Son can do some tinkering without having to worry about the nitty gritty).

I did notice an Open PR from October 2023 here: PiicoDev device presence tests added by mjtlx · Pull Request #13 · CoreElectronics/CE-PiicoDev-Unified · GitHub, I suspect that was entering busy season which is why it hasn’t been reviewed / merged yet.

Thanks for your feedback.

Kind regards,

Gary

2 Likes

Hi @Gary126657 Gary - I’m stoked that you’re considering contributing to our little open source project :smiley: I agree our guidelines could use some work to clear up the repo structure and guidelines.

CE-PiicoDev-Unified is the main unifying library

Device modules are found on their respective product pages / guides eg. the TMP117 temperature sensor has the following repo.
image

The PyPi package is purely for deployment/distribution - it’s mostly used by Raspberry Pi users since it downloads every PiicoDev driver file which is unnecessary for Pico/Microbit users.

Contributions should be made either to the Unified repo or to device repos.

The PyPi repo is a collection of the entire ecosystem at a moment in time and pushed to PyPi. We actually scrape all the individual repos to build this repo before deploying.

A side note about PR#13 - it’s unlikely we can merge the PR into the main branch because it introduces too much new code for Unified to cope with. You can see my discussion and some strategies here.

Given the delicate nature of keeping Unified… unifying, some user-changes are not viable to merge into the main branch (microbit restrictions), but represent a lot of value for the community. I’ve recently started curating outstanding contributions.

If you’d like to discuss your ideas before getting too far down the rabbit hole I’d love to! Good alignment will help us arrive at a solution that can be merged, or at the very least curated for other curious makers.

2 Likes

Hi @Michael, thanks for the links. I totally get you need to maintain QA for your side. And thanks for the forum link to, I will trawl through it.

My current PRs are likely going to be small updates such as docstring comments, type hinting (micropython supports basic typing hints up to Python 3.8) and adding functions for getting device id data where available (in my own project I have been inheriting your classes and modifying functionality, not all is appropriate to refactor upstream). I’ll send a couple of trial ones through. I am so used to these days relying on my IDE to chase documentation while coding, that is my main motivation.

I am currently focussing on the RPI side of things, kind of getting my head around what software patterns are appropriate while still being mindful that someday I want to integrate some PI Pico.

FYI, I have a project here: maker / bildah1 · GitLab, which includes hardware checks, configuration wizard, and discovery. I am about to post a significant commit for our robot, also called Bildah1 as well as some guide videos on deploying the project. My goal is to get my son independent with PyCharms as a remote console to the robot using videos that he can re-reference when he gets rusty or wants to try something new.

1 Like

That’s a great project @Gary126657 :slight_smile: I’ve checked out the repo and it’s beautifully documented :smiley:

Whatever probing PRs you create, it’s helpful to know that they’re as vanilla as possible for MicroPython - to remain compatible with micro:bit + Pico. There are loads of vanilla Python functions that are not implemented in MicroPython - so beware :stuck_out_tongue:

In any case I think that’s our sustainable way to go - if something has good value then we can curate it when we can, and PRs are for very stable contributions that can be tested on all platforms.

(we even support ESP32, though we’re less loud about that because there’s no guides!)

1 Like

Hi @Michael,

Thanks for the tips, I have been paying close attention to the micropython libraries and ports on their github. Reading between the lines, I am guessing micropython environments can be very NV storage limited. I can see in the RGB project that there a minified section. To me, this means that if I wanted to add some docstring or type hinting, it needs to be minified successfully.

While I am very rusty on continuous integration tech in the git world, at a minimum I am quite used to manually running some automated sanity checks prior to committing as part of QA. For my own workflow I have created a simple minification check (using python_minifier):

Is that the tool you use for minification?

I also want to run the mpy-cross compiler on the code against different targets. At the moment I am assuming that if the compiler works then all is well. I will need to check that assertion.

Are you aware of existing tools for checking if a python file is micropython compliant (syntax wise)?

I don’t think the traditional linters/hinters work are set up for it (I could be wrong, I’ll research those as well). I don’t plan to do anything significant or crazy with your repos, especially since I am not set up to run tests on all the functions. It is a pity that the stdlib package mock isn’t available, but I might find a way to run an automated test suite onboard my RPI Pico using some monkey patching, something I’ll think about. Anyway, I am thinking aloud, but if I’m going down a path that has already been visited feel free just to say “search ‘this term’ in the forums”.

1 Like

Extremely. The microbit v2 can support Unified and one “larger” device like the OLED. introducing a second large device is problematic. “simple” devices like a temperature or light sensor are much more forgiving.

I’m sure the package we use python-minifier would handle standard docstrings and/or multiline comments.
The automation is set up on the github side by specifying a .workflows directory. The main.yml describes the actions to take; in this case: on a commit to main, run the minifier.
It looks like you’ve already acquired a copy of the minifier to use locally.

Sure is!

I’m not! Though I’d be curious about your results.
As we’ve developed PiicoDev progressively, I’ve just run into each difference on the fly as I develop for eg. Pico and then move to Raspberry Pi. Now we have a framework in the form of PiicoDev_Unified that is pretty mature and handles the hardware differences well. From here we only run into trouble if we want to import exotic packages or invoke packages/methods that are native to Python but not MicroPython - not a bid deal usually.

Yeah, and exhaustive testing requires using the hardware, regrettably. I know that various tests can simulate hardware interactions - but my confidence is low that they are valid when we are essentially testing a hardware project.

If I’m correct, you’re one of the first :smiley: People have ported modules here and there to other platforms but major developments on the project has a whole has remained largely in-house.

1 Like

Hi, thanks for your extensive responses :slight_smile:.

I was very-curious about minification. I have taken a fork of python_minifier and am modifying it so that it could remove module-level code blocks based on _SYSNAME. The idea being, you could run minify on PiicoDev_Unified.py and spit out three minified files: one for microbit, one for linux, one for “else” with the non-target platform removed from the file. I seem to be having success (I worked with AST a decade ago, but still remember the gist) and am about half way through. (I will push upsteam if the main author is interested, it is not an existing issue or feature request but I thought I would just rock up with it if we decide to go ahead).

Before I go further, do you think this would be useful for your workflows? I am not sure how the minified files are brought in at the moment and whether specifying in a project a platform specific minified file is worth the effort of reducing the code (based on the current unified min file, 5325 bytes goes down to 2238 bytes for microbit if we put an if block around the platform specific classes as well).

As an aside: I expect the PiicoDev code base to succeed in being very stable since it only needs to be a thin interface to a hardware device. It is easy to manually test (with a workbench) with 100% coverage and move on. My thoughts on pre-testing were probably more with my own projects as I’d like to have a sophisticated library that is able to work on both platforms (something I’m experimenting on).

I’ve always been curious about this but never taken the plunge! It was an attractive idea, but difficult to justify when the whole point is to “unify” the user-code and device libraries so that no device-library needs to be device-specific.
But perhaps it’s more scalable to branch Unified in the long term.

That is significant! :open_mouth: A device library like the BME280 is about 2.5k, so it doesn’t buy a whole device. But all that room is freed up for user code. Nice work :ok_hand:

GitHub actions - refer to my previous reply where I link to the .workflows/main.yml file. Essentially, everything happens on GitHub after a commit to PiicoDev_Unified.py. A virtual machine is instantiated, pip install python_minifier and then executes the minifier on the target, generating a new /min/PiicoDev_Unified.py file.
That does mean that the VM needs to download+install minifier every time from a public pip repo.

IMO we’re using Actions in a pretty low-level and naive way. They can include very sophisticated tests for CICD - but i don’t think PiicoDev is quite there yet :stuck_out_tongue:

image

I am guessing that the intention, which PiicoDev_Unified has achieved, is having a unified interface to i2c and hence PiicoDev.

I guess the potential tension is that a unified interface does not have to be a unified code base (especially with the HW constraints you face). However, it starts with a common code base (which you have with your base class).

I will experiment with a few ideas while I am still on School Holidays and send PRs through. I’ll keep them small and happy for them to be rejected, none of it is personal, it just a way to have a conversation :-).

Thanks again for the github actions prompt, you had mentioned it before, but I forgot it.

2 Likes

Hi @Michael, a quick update/FYI.

I have the following PR going to python-minifier:

I plan to experiment with different workflows, cross-compile, etc… here:

Anyone is welcome to have a look if they are curious.

2 Likes