adafruit / Adafruit_CircuitPython_IS31FL3731

CircuitPython driver for the IS31FL3731 charlieplex IC
MIT License
22 stars 25 forks source link

Split the core driver from the board specific subclasses #27

Closed tannewt closed 3 years ago

tannewt commented 4 years ago

The board specific wrappers should live in separate files from the core class so that other boards' code isn't loaded into memory. They could even live in separate product level libraries (aka repos) instead of this one which should be IC level.

dglaude commented 4 years ago

I have done the split into separate files. However the core file is also the file for "Adafruit 16x9 Charlieplexed PWM LED Matrix" so it make importing that one different from the other. I did not try to split that into separate repository as this is a bit too much work for me. I did not try the before/after memory consumption yet.

dglaude commented 3 years ago

My PR #32 was going in that direction and was ready since a long time. The learn guide that needed to be adapted were identified. Now it is too late and broken by the addition of the #35.

Adafruit should show the way and make a library that only deal with that chip, and then have separated library for individual piece of hardware, including those made by Adafruit.

There are a lot of other hardware from Pimoroni (and maybe other) that use that chip:

If there is a pattern that need to be followed, then Adafruit should show the way and contributor will mimic that for their addition, but each contributor that only care about a board he want to add should not hold the burden of refactoring everything. I did the refactoring work to split the library into module once, but I cannot cope with this twice.

ladyada commented 3 years ago

@dglaude no problem - if you close the PR, we'll redo the refactor

dglaude commented 3 years ago

@ladyada I just want to point out a missed opportunities and that there is not point for me to work on my free time and have that wasted. There was an issue, I provided a PR for it and nothing was done with it.

Right now someone just need to add the work of Sandy (but he has pending PR to fix what was already accepted). And deal with the next PR in the pipeline (see below).

So whatever I do, there is the risk of having a PR spoil it, and I am not a GIT ninja. This is why I believe this work should be done or continued by Adafruit.

Look at what is coming or potentially coming, just today there are two new usage from this library, both linked to Pico:

  1. https://twitter.com/recantha/status/1368611754947842051?s=20 => https://www.recantha.co.uk/blog/?p=20904 => #37
  2. https://twitter.com/veryalien/status/1368573430656864257 => code not publish, but I can ask

It is up to you to decide if and how you would like to embrace the potential contribution from those author, but it is obvious that a refactoring of the code need to be fast and not stay as a PR for month or this kind of things will keep happening and the library size keep increasing (not a problem for Pico, but a problem for some M0 board).

dglaude commented 3 years ago

To be discussed "In the Weed".

ladyada commented 3 years ago

@dglaude there's no fault - there was nobody formally assigned to merge it and update all the documentation so nobody checked up on it. if you don't want to rebase, which is what it sounds like, close the PR and we'll redo the refactor.

makermelissa commented 3 years ago

@dglaude, after looking over this, I'm going to grab the code from your PR and manually merge #35 into it as well as some other outstanding PRs. So in other words, I'll submit a new PR to supersedes it as well as others, but it will incorporate the initial work you did. #32 should have probably been merged first, but we can only go forward from here.

makermelissa commented 3 years ago

Fixed via #38.