Skip to content

Split the core driver from the board specific subclasses #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tannewt opened this issue Apr 7, 2020 · 8 comments
Closed

Split the core driver from the board specific subclasses #27

tannewt opened this issue Apr 7, 2020 · 8 comments
Assignees

Comments

@tannewt
Copy link
Member

tannewt commented Apr 7, 2020

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
Copy link
Contributor

dglaude commented Aug 16, 2020

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
Copy link
Contributor

dglaude commented Mar 7, 2021

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:

  • Should they be added to this library?
  • Should they put their board into separate library? Maybe in the community bundle?

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
Copy link
Member

ladyada commented Mar 7, 2021

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

@dglaude
Copy link
Contributor

dglaude commented Mar 7, 2021

@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 => Add Pimoroni 11x7 LED Matrix Breakout #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
Copy link
Contributor

dglaude commented Mar 8, 2021

To be discussed "In the Weed".

@ladyada
Copy link
Member

ladyada commented Mar 8, 2021

@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
Copy link
Collaborator

@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
Copy link
Collaborator

Fixed via #38.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants