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] update bm40hsrgb with RGB Matrix support #10044

Merged
merged 16 commits into from
Aug 18, 2020

Conversation

rgoulter
Copy link
Contributor

@rgoulter rgoulter commented Aug 15, 2020

Updates the firmware for KPRepublic's BM40RGB keyboard that #9860 added.
This PR includes support for RGB Matrix.

Description

This PR updates the bm40hsrgb keyboard, adding RGB Matrix support.

I've tested this bm40rgb's RGB matrix functionality with my BM40RGB keyboard.

Types of Changes

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

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • 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 and don't break anything (as well as I can manage).

keyboards/bm40rgb/bm40rgb.c Outdated Show resolved Hide resolved
keyboards/bm40rgb/config.h Outdated Show resolved Hide resolved
keyboards/bm40rgb/info.json Outdated Show resolved Hide resolved
keyboards/bm40rgb/info.json Outdated Show resolved Hide resolved
keyboards/bm40rgb/readme.md Outdated Show resolved Hide resolved
keyboards/bm40rgb/rules.mk Outdated Show resolved Hide resolved
keyboards/bm40rgb/config.h Outdated Show resolved Hide resolved
keyboards/bm40rgb/config.h Outdated Show resolved Hide resolved
keyboards/bm40rgb/bm40rgb.h Outdated Show resolved Hide resolved
Co-authored-by: Ryan <fauxpark@gmail.com>

- Fix alignment in keyboards/bm40rgb/bm40rgb.c
- Remove redundant DESCRIPTION in keyboards/bm40rgb/config.h
- Tidy keyboards/bm40rgb/readme.md
- Tidy MANUFACTURER, PRODUCT in keyboards/bm40rgb/config.h
- Tidy rules.mk keyboards/bm40rgb/rules.mk
- Use PRODUCT_ID computed from SHASUM of keyboard name in keyboards/bm40rgb/config.h
- Define LAYOUT_all in keyboards/bm40rgb/bm40rgb.h
@rgoulter rgoulter force-pushed the add-keyboard-bm40rgb branch from c9b735b to c491c62 Compare August 15, 2020 05:40
rgoulter and others added 2 commits August 15, 2020 13:02
Co-authored-by: Ryan <fauxpark@gmail.com>
@rgoulter rgoulter marked this pull request as ready for review August 15, 2020 06:07
@rgoulter
Copy link
Contributor Author

@fauxpark Thanks for helping tidy up the code per the PR checklist. I wanted to make a WIP Draft Pull Request before going through the PR checklist.

I do see that some community layouts don't build with this keyboard. Looks like most of the failures are due to BACKLIGHT_PIN not being defined; but my rules.mk already has BACKLIGHT_ENABLE = no.
Are those build failures tolerable? It'd be nice to keep the support for community layouts with this keyboard.

Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

All references to ortho_4x12 need to be removed, as the board cannot physically support that layout.
image

keyboards/bm40rgb/rules.mk Outdated Show resolved Hide resolved
keyboards/bm40rgb/rules.mk Outdated Show resolved Hide resolved
keyboards/bm40rgb/info.json Outdated Show resolved Hide resolved
keyboards/bm40rgb/bm40rgb.h Outdated Show resolved Hide resolved
Co-authored-by: Joel Challis <git@zvecr.com>
@fauxpark fauxpark requested a review from a team August 15, 2020 15:21
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

Also the make target changing would usually mean it has to go via https://docs.qmk.fm/#/breaking_changes and target develop.

keyboards/bm40rgb/readme.md Outdated Show resolved Hide resolved
keyboards/bm40rgb/config.h Outdated Show resolved Hide resolved
keyboards/bm40rgb/info.json Outdated Show resolved Hide resolved
@zvecr zvecr added breaking_change Changes that need to wait for a version increment keyboard labels Aug 16, 2020
@rgoulter
Copy link
Contributor Author

Ah, it would be a breaking change. Hmm. I'll repurpose this PR/branch to just apply the changes that add RGB Matrix support, then.

@rgoulter rgoulter changed the title [keyboard] bm40rgb with RGB Matrix support [keyboard] update bm40hsrgb with RGB Matrix support Aug 16, 2020
keyboards/bm40hsrgb/bm40hsrgb.c Show resolved Hide resolved
keyboards/bm40hsrgb/bm40hsrgb.c Show resolved Hide resolved
keyboards/bm40hsrgb/info.json Outdated Show resolved Hide resolved
@noroadsleft noroadsleft removed the breaking_change Changes that need to wait for a version increment label Aug 18, 2020
Co-authored-by: James Young <18669334+noroadsleft@users.noreply.github.com>
@noroadsleft noroadsleft merged commit c59a81b into qmk:master Aug 18, 2020
@noroadsleft
Copy link
Member

Thanks!

@rgoulter
Copy link
Contributor Author

And I thank you code reviewers. I appreciate the patience to help iron out things that didn't fit well. 👍

nicocesar pushed a commit to nicocesar/qmk_firmware that referenced this pull request Sep 6, 2020
* bm40rgb: copy from kbfirmware.com

* bm40rgb: align default keymap

* bm40rgb: rename kb.c/h to bm40rgb.c/h

* bm40rgb: adjust with changes from the noroadsleft converter

* bm40rgb: adjust keyboard to support plant_mit and ortho_4x12 layouts

* bm40rgb: Add keyboard readme.md

* bm40rgb: add RGB Matrix support

* bm40rgb: remove bm40hsrgb layout

* Apply suggestions from PR

- Fix alignment in keyboards/bm40rgb/bm40rgb.c
- Remove redundant DESCRIPTION in keyboards/bm40rgb/config.h
- Tidy keyboards/bm40rgb/readme.md
- Tidy MANUFACTURER, PRODUCT in keyboards/bm40rgb/config.h
- Tidy rules.mk keyboards/bm40rgb/rules.mk
- Use PRODUCT_ID computed from SHASUM of keyboard name in keyboards/bm40rgb/config.h
- Define LAYOUT_all in keyboards/bm40rgb/bm40rgb.h

* Update keyboards/bm40rgb/info.json

* Tidy comment

* Apply suggestions from code review

* Apply suggestions from code review

* Revert "bm40rgb: remove bm40hsrgb layout"

This reverts commit 1f69a03.

* Move the rgbmatrix change over to bm40hsrgb

* Wrap g_led_config declaration with
@rgoulter rgoulter deleted the add-keyboard-bm40rgb branch November 4, 2021 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BM40HSRGB Support[Feature Request]
4 participants