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

add support for ModelM USB board #9846

Merged
merged 27 commits into from
Aug 22, 2020
Merged

add support for ModelM USB board #9846

merged 27 commits into from
Aug 22, 2020

Conversation

mschwingen
Copy link
Contributor

Add new keyboard: IBM Model M replacement USB board:
https://github.com/mschwingen/hardware/tree/master/modelm-usb

Description

Add new keyboard: IBM Model M replacement USB board

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

  • [X ] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • [ X] I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • [X ] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@zvecr zvecr added the keyboard label Jul 30, 2020
keyboards/mschwingen/modelm/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/rules.mk Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team August 4, 2020 06:52
keyboards/mschwingen/modelm/config.h Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/config.h Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/config.h Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/matrix.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/matrix.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/matrix.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/matrix.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/README.md Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/README.md Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/README.md Outdated Show resolved Hide resolved
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Also, for the LEDs, when using ws2812, you could use rgblight layers, actually.

https://docs.qmk.fm/#/feature_rgblight?id=lighting-layers

keyboards/mschwingen/modelm/config.h Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/led_ffc/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/led_wired/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/led_ws2812/rules.mk Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/modelm.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/modelm.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/modelm.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/modelm.c Outdated Show resolved Hide resolved
keyboards/mschwingen/modelm/rules.mk Outdated Show resolved Hide resolved
mschwingen and others added 2 commits August 21, 2020 15:25
use automatic variant defines from makefile instead of defining our own

Co-authored-by: Drashna Jaelre <[email protected]>
@mschwingen
Copy link
Contributor Author

Also, for the LEDs, when using ws2812, you could use rgblight layers, actually.

https://docs.qmk.fm/#/feature_rgblight?id=lighting-layers

I think I already replied to that - I tried, but it seems to me the rgblight layer system can't really do what I need. I want indicator LEDs that change color depending on layer, but the primary function is still num/scroll/capslock. The best I came up with is defining one layer per LED, and the turning on layers instead of LEDs - this adds unnecessary code without using any of the features from the rgblight layer.

@mschwingen mschwingen closed this Aug 21, 2020
@mschwingen mschwingen reopened this Aug 21, 2020
@mschwingen
Copy link
Contributor Author

Oops - sorry for the close/re-open, I meant to close the one request only.

@drashna
Copy link
Member

drashna commented Aug 22, 2020

Well, I think that you're basically doing the same thing, to be honest. Just in a slightly different way.

If you'd like, I can see converting it, if you'd like.

But for now, I'm going to merge it.

@drashna drashna merged commit 42eeb31 into qmk:master Aug 22, 2020
@mschwingen
Copy link
Contributor Author

Well, I think that you're basically doing the same thing, to be honest. Just in a slightly different way.

If you'd like, I can see converting it, if you'd like.

Unless the rgb_layers code can provide an advantage (and reduce my own code), I'd rather leave the code as is - while I understand the urge to use common library code instead of private implementations, in my attempts, it only added code size with no gain.

But for now, I'm going to merge it.

Thanks - also for the other suggestions!

nicocesar pushed a commit to nicocesar/qmk_firmware that referenced this pull request Sep 6, 2020
* add support for ModelM USB board

* EMI improvement: remove unnecessary toggling of MOSI pin

* address review comments

* Update keyboards/mschwingen/modelm/rules.mk

Co-authored-by: James Young <[email protected]>

* Update keyboards/mschwingen/modelm/rules.mk

Co-authored-by: James Young <[email protected]>

* Update keyboards/mschwingen/modelm/config.h

Co-authored-by: James Young <[email protected]>

* Update keyboards/mschwingen/modelm/config.h

Co-authored-by: James Young <[email protected]>

* Update keyboards/mschwingen/modelm/rules.mk

Co-authored-by: Drashna Jaelre <[email protected]>

* Update keyboards/mschwingen/modelm/keymaps/default/keymap.c

Co-authored-by: Drashna Jaelre <[email protected]>

* update printf usage

* add comment

* EMI improvement: remove unnecessary toggling of MOSI signal

* remove trailing space

* use shorter macros as suggested in review by noroadsleft, re-format table to line up columns

* Update keyboards/mschwingen/modelm/config.h

Co-authored-by: Ryan <[email protected]>

* Update keyboards/mschwingen/modelm/rules.mk

Co-authored-by: Ryan <[email protected]>

* Update keyboards/mschwingen/modelm/rules.mk

Co-authored-by: Ryan <[email protected]>

* Update keyboards/mschwingen/modelm/rules.mk

Co-authored-by: Ryan <[email protected]>

* Update keyboards/mschwingen/modelm/README.md

Co-authored-by: Ryan <[email protected]>

* Update keyboards/mschwingen/modelm/README.md

Co-authored-by: Ryan <[email protected]>

* Apply suggestions from code review

use spi_read from core insteads of our own copy

Co-authored-by: Ryan <[email protected]>

* include spi_master.c to use spi_read()

* Update keyboards/mschwingen/modelm/README.md

Co-authored-by: James Young <[email protected]>

* Apply suggestions from code review: correct indenting in keymap

Co-authored-by: James Young <[email protected]>

* Apply suggestions from code review

use automatic variant defines from makefile instead of defining our own

Co-authored-by: Drashna Jaelre <[email protected]>

* Update keyboards/mschwingen/modelm/rules.mk: use QUANTUM_LIB_SRC for uart.c

Co-authored-by: Drashna Jaelre <[email protected]>

Co-authored-by: Michael Schwingen <[email protected]>
Co-authored-by: James Young <[email protected]>
Co-authored-by: Drashna Jaelre <[email protected]>
Co-authored-by: Ryan <[email protected]>
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.

5 participants