drivers: add driver for IS31FL3731 matrix LED driver #370
Conversation
antonfisher
force-pushed
the
feature-is31fl3731-led-matrix-driver
branch
from
fbde6dc
to
50ecb0c
Compare
is31fl3731/is31fl3731.go
Outdated
| board: boardRaw, | ||
| } | ||
|
|
||
| err = d.configure() |
There was a problem hiding this comment.
In other drivers, we don't touch devices in New() methods. Configure() method must be called separately, usually with configuration parameters.
This means configure() must be changed to public (->Configure())
There was a problem hiding this comment.
Changed to Configure() 👍
is31fl3731/is31fl3731.go
Outdated
|
|
||
| // NewAdafruitCharlieWing15x7 creates a new driver with Adafruit 15x7 | ||
| // CharliePlex LED Matrix FeatherWing (CharlieWing) layout | ||
| func NewAdafruitCharlieWing15x7(bus drivers.I2C, address uint8) (d Device, err error) { |
There was a problem hiding this comment.
This seem to be very specific, I'd extract all code related to CharlieWing to its own file, like is31fl3731_acw15x7.go
There was a problem hiding this comment.
Moved all CharlieWing code to a separate file (struct) 👍
examples/is31fl3731/main.go
Outdated
| ) | ||
|
|
||
| // I2CAddress -- address of led matrix | ||
| var I2CAddress uint8 = 0x74 |
There was a problem hiding this comment.
How random is this address? Can it be moved to the driver itself?
There was a problem hiding this comment.
For standalone is31fl3731 the addresses can be:
- 0x74 (AD pin connected to GND)
- 0x75 (AD pin connected to SCL)
- 0x76 (AD pin connected to SDA)
- 0x77 (AD pin connected to VCC)
For Adafruit Charlie Wing 15x7 its either 0x74 or 0x77. I guess I can make four constants, but it's up to the particular board to define address. WDYT?
There was a problem hiding this comment.
I'd say have them 4 constants in the driver "registers.go" as per praxis and seen in other drivers.
Preferably with comments (exactly as you did here in comment).
And let user of the driver choose which address to use.
There was a problem hiding this comment.
Disclaimer, I'm not a maintainer, but submitted several drivers already to this repo and sort of know the drill.
I like your PR and just want to help it :)
There was a problem hiding this comment.
got it, thanks for the help! :)
There was a problem hiding this comment.
Added address constants 👍
…harlieWing code to a separate file
…tCharlieWing15x7 to the bottom for consistency
|
Thank you @antonfisher for working on this, and thanks @ysoldak for proving all of the feedback/reviews. I am going to squash/merge now. |
IS31FL3731: add driver for IS31FL3731 matrix LED driver
This PR adds a driver for IS31FL3731 matrix LED driver.
Datasheet: https://www.lumissil.com/assets/pdf/core/IS31FL3731_DS.pdf
The first version is basic, but functional.
What's implemented:
Supported boards:
DrawPixelIndex(...)functionDrawPixelXY(...)orDrawPixelIndex(...)function (I've tested on this one with NRF52840 controller).Boards can be added in the future versions:
Tests:
fmt-checkpassunit-testpasssmoke-testpass for the driver:Except the AVR part fails because I do not have
avr-gccinstalled.Example code works like this:
