Skip to content
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

[Keyboard] Add Pimoroni Keybow 2040 #21481

Closed
wants to merge 1 commit into from

Conversation

fanf2
Copy link

@fanf2 fanf2 commented Jul 8, 2023

Description

Keybow 2040

The Pimoroni Keybow 2040 is a 16 key mechanical macropad controlled by a Raspberry Pi RP2040, with Kailh hot swap sockets and per key RGB lighting, in a decorative FR4 sandwich.

I am not associated with Pimoroni, I just own a Keybow 2040. I thought installing QMK on it might be a fun way to learn more about QMK. LITTLE DID I KNOW...

As far as I know, all the problems with this PR have been resolved, so it should be ready to go.

The following struck-out sections describe some problems I had when trying to get it to work. They are retained for historical interest only.

VIA

On my M1 Mac, VIA.app is stuck "Searching for devices..."

I have tried loading keybow2040.json (from thte VIA commit in this PR) into VIA's "Design" tab: still no luck.

I ttried adding a little debug logging to raw_hid_receive() in quantum/via.c, but it never prints anything to the console.

I also have a Mechboards / Yiancar HS60 which used to work with VIA, but does not any more.

So I currently suspect something is broken with VIA, but it does not seem possible to get any information out of VIA to explain what it is doing and what is or is not working, and searchengineering has so far not helped me find a solution.

LEDs

Another frustrating failure, I'm afraid

I seem to be suffering from some kind of asynchronous corruption of the global matrix[] array. I tried to track it down with a lot of VERIFY_TRACED_VARIABLES() but I ended up at some code that could not possiblly be blamed, unless the corruption was coming from some kind of interrupt service routine.

Despite that

I have tracked down the relevant source code for the Keybow 2040 default CircuitPython firmware, and together with its published schematic I think most of the configuration setttings are reasonably plausible (tho I have not been able to test things like the orientation of the LED matrix). The readme.md contains links to the more obscure information I used.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project
    nah, I can fix that when the code work :trollface:
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation
    maybe, dunno yet if there are things I learned that could go into the docs
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work
    heh lol not quite 😭

@github-actions github-actions bot added keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Jul 8, 2023
@sigprof
Copy link
Contributor

sigprof commented Jul 8, 2023

On my M1 Mac, VIA.app is stuck "Searching for devices..."

Are you using the latest VIA.app? Although it seems that the preferred way to use VIA now is to open https://usevia.app in a Chrome-based browser (the required WebHID support is currently not present in any other browser engine). Any old version of the local VIA application won't work with recent firmwares (and new versions are just Electron wrappers for https://usevia.app).

@fanf2
Copy link
Author

fanf2 commented Jul 8, 2023

Yes, I installed VIA 2.2 and 3.0, neither worked.

I prefer to avoid Google as much as possible. I wonder if VIA is failing at rawhid because of Apple security restrictions on an unsigned executable, but who can say without debug logging 🤷 Maybe the Chrome dev tools will be more helpful. Yuck.

keyboards/pimoroni/keybow2040/info.json Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/config.h Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/config.h Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/keybow2040.c Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/keybow2040.c Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/keymaps/default/keymap.json Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/keymaps/via/keybow2040.json Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/keymaps/via/keymap.json Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/readme.md Outdated Show resolved Hide resolved
@sigprof
Copy link
Contributor

sigprof commented Jul 8, 2023

I prefer to avoid Google as much as possible.

On Linux ungoogled-chromium can be used with https://usevia.app; no idea whether it would work on MacOS (especially on the M1 platform).

@fanf2 fanf2 marked this pull request as draft July 8, 2023 20:24
@drashna
Copy link
Member

drashna commented Jul 8, 2023

I prefer to avoid Google as much as possible.

On Linux ungoogled-chromium can be used with usevia.app; no idea whether it would work on MacOS (especially on the M1 platform).

For Ventura, I found that I had to explicitly add the via app to the "input monitoring" permissions before via would work. Same with the toolbox for the console/hid bootloaders.

@fanf2 fanf2 force-pushed the pimoroni_keybow_2040 branch 2 times, most recently from 2580a3a to 172954c Compare July 11, 2023 17:19
@fanf2 fanf2 marked this pull request as ready for review July 11, 2023 17:27
@fanf2
Copy link
Author

fanf2 commented Jul 11, 2023

I have pushed round two of this pull request 🎉

Many thanks to @sigprof for the thorough and helpful review, and thanks to @drashna for the extra hints and tips!

I have gone through the review comments and I think I have addressed them all. My apologies if I missed any.

I got the RGB matrix working after much careful checking of each LED's register number, and the corresponding key and LED locations should match. (I understand now how the incorrect LED register numbers could cause out-of-bounds array accesses which would explain the mysterious memory corruption I was struggling with before.)

There were also some problems with inconsistencies in my I2C driver configuration. I cleaned them up to match the board's schematic: it uses GPIOs 4 and 5 for I2C, and those pins are connected to the RP2040's I2C0 peripheral. However, a perfectly consistent I2C0 configuration does not compile because some of QMK's code assumes that I2C1 is configured. If I add a dummy I2C1 configuration that refers to unused RP2040 pins, the RGB matrix remains black. If I tell the driver to use the same GPIO pins for I2C0 and I2C1, it compiles and works. 🧐

I have not yet tested that VIA works if I use Chrome. Maybe after I have had something to eat.

keyboards/pimoroni/keybow2040/mcuconf.h Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/mcuconf.h Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/config.h Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/info.json Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/info.json Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/halconf.h Outdated Show resolved Hide resolved
@fanf2 fanf2 force-pushed the pimoroni_keybow_2040 branch 3 times, most recently from ec6a691 to e228d62 Compare July 12, 2023 09:05
@fanf2
Copy link
Author

fanf2 commented Jul 12, 2023

Round three!

Thanks again to @sigprof for your helpful guidance. I think I've addressed the problems you found. I have added some improved explanatory comments to keybow2040/config.h; I hope they are correct!

@fanf2 fanf2 force-pushed the pimoroni_keybow_2040 branch 2 times, most recently from e18b055 to d67f5e8 Compare July 12, 2023 09:55
@fanf2
Copy link
Author

fanf2 commented Jul 19, 2023

Hello again,

I gave my Keybow 2040 a prod with VIA running in Chrome instead of the Electron version, and it seemed to work OK.

I think all the problems with this PR have been resolved. Please let me know if there's anything else I can do to help get it merged :-)

@mosch
Copy link

mosch commented Jul 26, 2023

@fanf2 thanks for working on this! I can confirm the fw works on my Keybow 2040.

Im also having issues with the via app. Tested the downloadable version and usevia.app; on Win+Mac in Chrome + Edge. @fanf2 how did you get it working?

The Keybow shows up in the browser dialog (shows as paired). However I only see the [Authorized Device +] Button.

@fanf2
Copy link
Author

fanf2 commented Jul 27, 2023

Hi, @mosch,

VIA does not yet know about the Keybow 2040; I have been waiting for this PR to be merged before I open the PR for VIA supprt.

Until it gets official support from VIA, you need to make the following incantations:

  • go to the settings (cogwheel) tab in VIA;
  • turn on the "show design tab" switch
  • go to the design (paintbrush) tab;
  • bop the "load draft definition" button
  • feed it a copy of the following JSON
{
    "name": "Pimoroni Keybow 2040",
    "vendorId": "0x5069",
    "productId": "0x4784",
    "matrix": {
	"rows": 5,
	"cols": 4
    },
    "layouts": {
	"keymap": [
	    [{"x":2.5,"w":0.5,"h":0.5,"w2":1},"4,0"],
	    [{"y":-0.5},"0,0","0,1","0,2","0,3"],
	    ["1,0","1,1","1,2","1,3"],
	    ["2,0","2,1","2,2","2,3"],
	    ["3,0","3,1","3,2","3,3"]
	]
    }
}

@mosch
Copy link

mosch commented Jul 28, 2023

Sweet that works, Thank you!

Copy link

@mosch mosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are missing a comma here. I also found that this JSON uses tailing commas, while the others don't.

keyboards/pimoroni/keybow2040/info.json Outdated Show resolved Hide resolved
@fanf2 fanf2 force-pushed the pimoroni_keybow_2040 branch 2 times, most recently from 9f3d63d to 344aa9c Compare July 28, 2023 09:23
@mosch
Copy link

mosch commented Aug 9, 2023

This looks good now, anything missing before getting it in? @sigprof

keyboards/pimoroni/keybow2040/readme.md Outdated Show resolved Hide resolved
Copy link
Member

@drashna drashna Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyboards/pimoroni/keybow2040/keybow2040.c Outdated Show resolved Hide resolved
keyboards/pimoroni/keybow2040/keybow2040.c Show resolved Hide resolved
keyboards/pimoroni/keybow2040/config.h Outdated Show resolved Hide resolved
Copy link

@itsnelsonmartins itsnelsonmartins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change from IS31FL3731 to is31fl3731 to fix QMK CI Run

keyboards/pimoroni/keybow2040/info.json Outdated Show resolved Hide resolved
@fanf2 fanf2 force-pushed the pimoroni_keybow_2040 branch 2 times, most recently from ba17986 to cd809cb Compare September 3, 2023 17:33
@fanf2
Copy link
Author

fanf2 commented Sep 3, 2023

OK, I have updated the readme.md to follow the template more closely. I don't have an imgur account so I deleted the hero image. I have improved keybow2040.c, config.h, and info.json as suggested.

Thank you for the review and style fixes. I hope it is now good to go!

Comment on lines 1 to 2
Keybow 2040
===========
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Keybow 2040
===========
# Keybow 2040

Comment on lines 30 to 42
bootloader
----------

The procedure to flash the firmware is similar to other RP2040
devices. You can enter the bootloader in two ways:

* **Vulcan nerve pinch**: hold down BOOTSEL while pressing RESET
(not so great if you have configured BOOTSEL to do something)

* **Mr Resetti double-tap**: press RESET twice quickly
(only works when running the QMK firmware)

Find the BOOTSEL and RESET buttons on your Keybow 2040 either side of
the USB socket on the edge of the PCB. When the USB socket is facing
away from you, BOOTSEL is on the right and RESET is on the left.
According to the schematic the reset button is called Mr Resetti.

* _Aside_: The BOOTSEL button disables the QSPI flash that stores
the firmware, which makes the RP2040 use its built-in UF2
bootloader.

A USB drive will appear called `RPI-RP2`. Copy the firmware file
`pimoroni_keybow2040_default.uf2` onto the `RPI-RP2` drive using
whatever software is convenient (e.g. the `cp` command, the macOS
Finder, or Windows Explorer). The `make :flash` command will copy the
firmware for you.

The `RPI-RP2` drive will disappear (which might make your OS complain
that you failed to unmount it first) and a keyboard device will appear
in its place.

The Pimoroni web site has [instructions for reinstalling CircuitPython
on the Keybow 2040](https://learn.pimoroni.com/article/circuitpython-and-keybow-2040)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bootloader
----------
The procedure to flash the firmware is similar to other RP2040
devices. You can enter the bootloader in two ways:
* **Vulcan nerve pinch**: hold down BOOTSEL while pressing RESET
(not so great if you have configured BOOTSEL to do something)
* **Mr Resetti double-tap**: press RESET twice quickly
(only works when running the QMK firmware)
Find the BOOTSEL and RESET buttons on your Keybow 2040 either side of
the USB socket on the edge of the PCB. When the USB socket is facing
away from you, BOOTSEL is on the right and RESET is on the left.
According to the schematic the reset button is called Mr Resetti.
* _Aside_: The BOOTSEL button disables the QSPI flash that stores
the firmware, which makes the RP2040 use its built-in UF2
bootloader.
A USB drive will appear called `RPI-RP2`. Copy the firmware file
`pimoroni_keybow2040_default.uf2` onto the `RPI-RP2` drive using
whatever software is convenient (e.g. the `cp` command, the macOS
Finder, or Windows Explorer). The `make :flash` command will copy the
firmware for you.
The `RPI-RP2` drive will disappear (which might make your OS complain
that you failed to unmount it first) and a keyboard device will appear
in its place.
The Pimoroni web site has [instructions for reinstalling CircuitPython
on the Keybow 2040](https://learn.pimoroni.com/article/circuitpython-and-keybow-2040)
## Bootloader
Enter the bootloader in 3 ways:
* **Bootmagic reset**: Hold down the key at (0,0) in the matrix (usually the top left key or Escape) and plug in the keyboard
* **Physical reset button**: Double tap the RESET button, or hold BOOTSEL and tap RESET
The Pimoroni web site has [instructions for reinstalling CircuitPython
on the Keybow 2040](https://learn.pimoroni.com/article/circuitpython-and-keybow-2040)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is wrong, because I have not enabled the bootmagic feature:

  • there's no point since the reset and bootsel buttons are easily accessible on the side of the Keybow2040
  • the RP2040's ROM implements the bootsel feature, so there's no need for QMK to do so
  • the bootsel button is not (0,0) in the matrix because that would mess up the 4x4 layout

I've trimmed the tutorial parts of that section, and added a sentence about bootsel vs bootmagic.

I've left my jokes in because this is supposed to be fun 🤪

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bootmagic doesn't just reset the board and jump to bootloader, it also clears the eeprom. Which can be very useful. And has nothing to do with the bootsel button (it's just an arbitrary spot in the matrix, but defaults to 0,0).

Comment on lines 65 to 58
notes
-----

* The BOOTSEL button doubles as a 17th configurable key.

* The default keymap has F13 - F20 on the top half of the board
(nearest the USB socket), and a navigation cluster (arrows, home,
end, page up/down) on the bottom half. The BOOTSEL button changes
the RGB matrix mode.

* The Keybow 2040 CircuitPython firmware uses VID = 0x16D0, PID = 0x08C6,
but this QMK firmware has different USB IDs to avoid confusing drivers:
VID = 0x5069 ('Pi'), PID = 0x4784 (4 'x' 4)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how useful this is here.

Suggested change
notes
-----
* The BOOTSEL button doubles as a 17th configurable key.
* The default keymap has F13 - F20 on the top half of the board
(nearest the USB socket), and a navigation cluster (arrows, home,
end, page up/down) on the bottom half. The BOOTSEL button changes
the RGB matrix mode.
* The Keybow 2040 CircuitPython firmware uses VID = 0x16D0, PID = 0x08C6,
but this QMK firmware has different USB IDs to avoid confusing drivers:
VID = 0x5069 ('Pi'), PID = 0x4784 (4 'x' 4)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left that in because there ought to be somewhere for users to find out what the default keymap is without reading the JSON, and I need somewhere to explain the info.json configuration choices. JSON isn't supposed to have comments, so 🤷

resources
---------

Here are some links to the information I used when configuring QMK for the Keybow 2040.

* [Piomoroni Keybow 2040 product page](https://shop.pimoroni.com/products/keybow-2040)
* [Keybow 2040 assembly instructions](https://learn.pimoroni.com/article/assembling-keybow-2040)

Places to find information about the Keybow 2040 hardware:

* [schematic](https://cdn.shopify.com/s/files/1/0174/1800/files/keybow_2040_schematic.pdf)
* contains most of the information that can also be gleaned from the CircuitPython support

* Pimoroni user-friendly CircuitPython libraries
* [old](https://github.com/pimoroni/keybow2040-circuitpython) and [new](https://github.com/pimoroni/pmk-circuitpython)
* these are wrappers around the low-level support below

* [CircuitPython hardware support](https://github.com/adafruit/circuitpython/blob/main/ports/raspberrypi/boards/pimoroni_keybow2040/)
* defines map from hardware feature names (e.g. `SW15`) to the way they are accessed (e.g. GPIO6)

* [CircuitPython IS31FL3731 driver](https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731/blob/main/adafruit_is31fl3731/keybow2040.py)
* defines the RGB matrix layout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better linked in the "hardware supported" section above. Though, it looks like most of this is linked in the product page, already.

Suggested change
resources
---------
Here are some links to the information I used when configuring QMK for the Keybow 2040.
* [Piomoroni Keybow 2040 product page](https://shop.pimoroni.com/products/keybow-2040)
* [Keybow 2040 assembly instructions](https://learn.pimoroni.com/article/assembling-keybow-2040)
Places to find information about the Keybow 2040 hardware:
* [schematic](https://cdn.shopify.com/s/files/1/0174/1800/files/keybow_2040_schematic.pdf)
* contains most of the information that can also be gleaned from the CircuitPython support
* Pimoroni user-friendly CircuitPython libraries
* [old](https://github.com/pimoroni/keybow2040-circuitpython) and [new](https://github.com/pimoroni/pmk-circuitpython)
* these are wrappers around the low-level support below
* [CircuitPython hardware support](https://github.com/adafruit/circuitpython/blob/main/ports/raspberrypi/boards/pimoroni_keybow2040/)
* defines map from hardware feature names (e.g. `SW15`) to the way they are accessed (e.g. GPIO6)
* [CircuitPython IS31FL3731 driver](https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731/blob/main/adafruit_is31fl3731/keybow2040.py)
* defines the RGB matrix layout

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left these links in because it was difficult to find the relevant parts of the default Python firmware, so I think it will be helpful when anyone else wants to work on this board.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this isn't the circuit python repo, so has zero relevance here. And most of the links add no value.

Copy link

github-actions bot commented Nov 3, 2023

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Nov 3, 2023
@mosch
Copy link

mosch commented Nov 3, 2023

How can I help getting this in? I think it's a vaulable contribution and brings a great keyboard into QMK. Let's get this in!

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Nov 4, 2023
@willoucom
Copy link
Contributor

Hello,
I have compiled and tested this firmware and it works perfectly.
Is it possible to review this PR again?
Can I help?
Thank you very much

@keyboard-magpie
Copy link
Contributor

@fanf2 will need to accept some of the suggested changes above still.

Additionally, there are some changes required since a round of breaking changes has passed- qmk migrate and/or qmk info -kb pimoroni/keybow2040 -f json may help you find these.

@@ -0,0 +1 @@
# Blankety Blank chequebook and pen
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Blankety Blank chequebook and pen
# This file intentionally left blank

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good grief am I not allowed to express my personality at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as it makes refactoring at a later date more complicated.

Comment on lines +7 to +13
/*
* see config.h for notes on the Keybow 2040 I2C setup
*/
#undef RP_I2C_USE_I2C0
#undef RP_I2C_USE_I2C1
#define RP_I2C_USE_I2C0 TRUE
#define RP_I2C_USE_I2C1 FALSE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
* see config.h for notes on the Keybow 2040 I2C setup
*/
#undef RP_I2C_USE_I2C0
#undef RP_I2C_USE_I2C1
#define RP_I2C_USE_I2C0 TRUE
#define RP_I2C_USE_I2C1 FALSE
#undef RP_I2C_USE_I2C0
#define RP_I2C_USE_I2C0 TRUE
#undef RP_I2C_USE_I2C1
#define RP_I2C_USE_I2C1 FALSE

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, I prefer my version.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can close this PR then if you like.

Comment on lines +5 to +7
/*
* see config.h for notes on the Keybow 2040 I2C setup
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
* see config.h for notes on the Keybow 2040 I2C setup
*/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not removing this because it's an important cross-reference that I wanted (but lacked) when I was working on this code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config.h is a standard file in QMK. There is absolutely no need to tell users about it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how adding that adds any value over what is in the docs.
RE: https://docs.qmk.fm/#/i2c_driver?id=arm-configuration

Comment on lines 6 to 8
/*
* see Keybow 2040 schematic for the LED wiring
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
* see Keybow 2040 schematic for the LED wiring
*/

Copy link
Author

@fanf2 fanf2 Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not removing this because it's an important cross-reference that I failed to find as soon as I would have liked when I was working on this code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? Or rather, why wouldn't every keyboard need this, then? "see schematic" is useless information, as that's always the goto when referencing the design.

It's like "see readme for details". It's pointless.

Comment on lines 6 to 11

#define RGB_MATRIX_LED_COUNT (4 * 4)
#define RGB_MATRIX_KEYPRESSES

/*
* LED driver chip count: one IS31FL3731
*/
#define DRIVER_COUNT 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define RGB_MATRIX_LED_COUNT (4 * 4)
#define RGB_MATRIX_KEYPRESSES
/*
* LED driver chip count: one IS31FL3731
*/
#define DRIVER_COUNT 1

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno why this change is needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three of these defines are no longer necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are no longer required.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I guess the DRIVER_COUNT is obsolete since 84df695

And RGB_MATRIX_LED_COUNT is obsolete since 95681b8

And DRIVER_COUNT since 84df695

Dunno what the issue is with RGB_MATRIX_KEYPRESSES?

Copy link
Member

@fauxpark fauxpark Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's implied when you have at least one reactive effect enabled dfb6d38.

Comment on lines 15 to 26
/*
* I2C address: IS31FL3731 AD pin is connected to ground
*/
#define DRIVER_ADDR_1 0b1110100

/*
* The RP2040 has two I2C peripherals; I2C0 must be used with GPIO 4
* and 5 which on the Keybow 2040 are wired up to the IS31FL3731.
*/
#define I2C_DRIVER I2CD0

/*
* I2C1 here refers to the board's / QMK's first (and only) I2C
* peripheral, not to the RP2040's second I2C peripheral.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
* I2C address: IS31FL3731 AD pin is connected to ground
*/
#define DRIVER_ADDR_1 0b1110100
/*
* The RP2040 has two I2C peripherals; I2C0 must be used with GPIO 4
* and 5 which on the Keybow 2040 are wired up to the IS31FL3731.
*/
#define I2C_DRIVER I2CD0
/*
* I2C1 here refers to the board's / QMK's first (and only) I2C
* peripheral, not to the RP2040's second I2C peripheral.
*/
#define IS31FL3731_I2C_ADDRESS_1 IS31FL3731_I2C_ADDRESS_GND
#define I2C_DRIVER I2CD0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno why this change is needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ISSI addresses should now use their respective defines instead of literals, for consistency and clarity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because things were refactored last develop cycle?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see #define DRIVER_ADDR_1 needs changing since d56ee70

Other than that, it's wrong to remove the comments since they explain where the values come from.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs are a far better place for that kind of explanation, you know...

@fanf2
Copy link
Author

fanf2 commented Dec 12, 2023

OK, I've gone through the comments and worked out where most of the unexplained ones came from, so I think this PR is now up-to-date with the upstream changes in the last three months.

I have reflashed my keybow2040 and it seems to be working ok.

/*
* the LED addresses must match the wiring in the Keybow 2040 schematic
*/
const is31_led PROGMEM g_is31_leds[RGB_MATRIX_LED_COUNT] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const is31_led PROGMEM g_is31_leds[RGB_MATRIX_LED_COUNT] = {
const is31fl3731_led_t PROGMEM g_is31fl3731_leds[IS31FL3731_LED_COUNT] = {

["GP19", "GP15", "GP11", "GP7"],
["GP20", "GP16", "GP12", "GP8"],
["GP21", "GP17", "GP13", "GP9"],
["GP23", "NO_PIN", "NO_PIN", "NO_PIN"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
["GP23", "NO_PIN", "NO_PIN", "NO_PIN"]
["GP23", null, null, null]

Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jan 27, 2024
Copy link

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

@github-actions github-actions bot closed this Feb 26, 2024
@Gadgetoid
Copy link

@fanf2 thank you for all the effort you've put in here.

Could I please implore you to defer to the maintainers and get this PR ship-shaped when you have the time?

I would love to see this PR merged and for us (disclaimer: I'm the software wrangler at Pimoroni) to have a toe into the QMK world.

Thank you! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyboard keymap stale Issues or pull requests that have become inactive without resolution. via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants