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.

Sign up for GitHub

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

Jump to bottom

Add support for Teensy-modified Datahand keyboard #4847

Merged
merged 1 commit into from
Jan 22, 2019
Merged

Add support for Teensy-modified Datahand keyboard #4847

merged 1 commit into from
Jan 22, 2019

Conversation

adzenith
Copy link
Contributor

@adzenith adzenith commented Jan 14, 2019 •
edited
Loading

Description

I have a DataHand keyboard in which I've replaced the stock processor with a Teensy++ 2.0 in a similar manner to that described here. This PR adds a new keyboard to support this setup.

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.
  • I have read the CONTRIBUTING document. (https://docs.qmk.fm/#/contributing)
  • 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).

noroadsleft
noroadsleft requested changes Jan 14, 2019
View reviewed changes
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.

@adzenith My understanding is that changes to core code should be made in pull requests separately from changes or additions to keyboards, but I'll let someone with more knowledge than I make the decision on that.

keyboards/datahand/config.h Outdated Show resolved Hide resolved
keyboards/datahand/config.h Outdated Show resolved Hide resolved
keyboards/datahand/config.h Outdated Show resolved Hide resolved
keyboards/datahand/datahand.h Outdated Show resolved Hide resolved
keyboards/datahand/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/datahand/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/datahand/keymaps/default/keymap.c Outdated Show resolved Hide resolved
@adzenith
Copy link
Contributor Author

Thanks for the quick review! I've updated the PR as per your suggestions: adding KC_ everywhere, fixing header guards, updating to process_user_record, etc.

drashna
drashna requested changes Jan 15, 2019
View reviewed changes
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.

It looks like this may be better in the "converter" folder, given what it is.

That or, because this isn't a mass produced board/replacement controller, it should probably be moved to "handwired".

quantum/matrix.c Outdated Show resolved Hide resolved
tmk_core/common/action_layer.c Outdated Show resolved Hide resolved
@adzenith
Copy link
Contributor Author

I'll move it to handwired. Thanks for the review!

@adzenith
Copy link
Contributor Author

Updated!

@adzenith adzenith force-pushed the datahand branch 3 times, most recently from 363c102 to e8ebbf1 Compare January 16, 2019 15:41
@adzenith
Copy link
Contributor Author

CI is showing a strange error: Compiling: keyboards/handwired/datahand/matrix.c avr-gcc: error: device-specs/specs-AT90USB1286: No such file or directory
Does anyone know what this might mean? It builds fine locally.

drashna
drashna requested changes Jan 18, 2019
View reviewed changes
keyboards/handwired/datahand/rules.mk Outdated Show resolved Hide resolved
@adzenith
Copy link
Contributor Author

Looks like CI is working now! Let me know if there's anything else I need to do. Thanks again for the help!

@drashna
Copy link
Member

drashna commented Jan 22, 2019

This is more of a nit pick, but could you update the code (especially the default keymap) to conform to the QMK Coding conventions?
https://docs.qmk.fm/#/contributing?id=coding-conventions

@adzenith
Copy link
Contributor Author

Sure! I can change the comments to use C-style comments (which it says is "strongly encouraged") – other than that, I think I already am complying with coding standards. Is there anything else I'm missing?
Thanks!

@adzenith
Copy link
Contributor Author

Updated with C-style comments.

@drashna
Copy link
Member

drashna commented Jan 22, 2019 •
edited
Loading

The comments are fine.

Mostly: https://github.com/qmk/qmk_firmware/blob/df34c46d1f939ff37fcf24daf94800c65fdeceff/keyboards/handwired/datahand/keymaps/default/keymap.c#L137-L238

And:

https://github.com/qmk/qmk_firmware/blob/df34c46d1f939ff37fcf24daf94800c65fdeceff/keyboards/handwired/datahand/matrix.c

@adzenith
Copy link
Contributor Author

Hm, I'm not sure what you mean. I'm looking at the coding convention and I see

  • We indent using two spaces (soft tabs)
  • We use a modified One True Brace Style
  • We encourage use of C style comments: /* */
  • In general we don't wrap lines
  • We use #pragma once

I do see a couple of misplaced braces in keymap.c, which I can fix, but I'm not sure what else is being pointing out in those sections.
Are you referring to the function opening braces? I can move those onto the same lines as the function declarations if so. The coding standard says block braces have to be on the same line, but function braces I wasn't sure about because a different style is used in e.g. quantum/matrix.c and quantum/quantum.c

@adzenith
Copy link
Contributor Author

I've moved the function braces onto the function lines - let me know if that's what you meant!

@drashna
Copy link
Member

drashna commented Jan 22, 2019

Sorry for not being clear, yeah that's what I meant!

@drashna
Copy link
Member

drashna commented Jan 22, 2019

Thanks!

@drashna drashna merged commit 6b1009b into qmk:master Jan 22, 2019
@adzenith
Copy link
Contributor Author

Thanks for the reviews and the merge!

@adzenith adzenith deleted the datahand branch January 23, 2019 04:25
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Jan 24, 2019
@Shinichi-Ohki
Merge branch 'master' of https://github.com/qmk/qmk_firmware
11284ba 
* 'master' of https://github.com/qmk/qmk_firmware: (87 commits)
  [Keyboard] KBD67: enable bootmagic lite by default (qmk#4931)
  [Keymap] Adding keymap for Nyquist (qmk#4918)
  Optimize/Update the new_project script (qmk#4920)
  Remove lfkeyboards parent rules.mk as its only required for mini1800
  [Keyboard] Add BDN9 (qmk#4919)
  [Keyboard] Add KBD67 Hotswap Support (qmk#4916)
  Fixup the clueboard 66 info.json
  Clueboard refresh (qmk#4902)
  Give the keymap folder the proper name
  Fix layouts/default/66_iso keymap
  [Keyboard] Add DataHand keyboard support (qmk#4847)
  [Keymap] Add a compile-time provided macro and assign to _FL (qmk#4908)
  Added info.json for TGR Alice
  Always read two bytes from the endpoint if we have two bytes to read
  Rename i2c_slave functions so it can coexist with i2c_master (qmk#4875)
  Fix for ISO layout in tada68:rys (qmk#4906)
  [Keyboard] Added TGR Alice keyboard support (qmk#4896)
  handwired/retro_refit: refactor, Configurator support and readme update (qmk#4899)
  Initial fixes for vagrant (qmk#4900)
  Tidy up IS_{,HOST_}LED_{ON,OFF} macros (qmk#4894)
  ...
tenderlove added a commit to tenderlove/qmk_firmware that referenced this pull request Jan 24, 2019
@tenderlove
Merge branch 'master' into merging
e5886c5 
* master: (643 commits)
  [Keyboard] Remove hadron ver0 as it is no longer required (qmk#4921)
  Remove unused fn_actions[] and action_function() in default keymaps (qmk#4829)
  [Keyboard] KBD67: enable bootmagic lite by default (qmk#4931)
  [Keymap] Adding keymap for Nyquist (qmk#4918)
  Optimize/Update the new_project script (qmk#4920)
  Remove lfkeyboards parent rules.mk as its only required for mini1800
  [Keyboard] Add BDN9 (qmk#4919)
  [Keyboard] Add KBD67 Hotswap Support (qmk#4916)
  Fixup the clueboard 66 info.json
  Clueboard refresh (qmk#4902)
  Give the keymap folder the proper name
  Fix layouts/default/66_iso keymap
  [Keyboard] Add DataHand keyboard support (qmk#4847)
  [Keymap] Add a compile-time provided macro and assign to _FL (qmk#4908)
  Added info.json for TGR Alice
  Always read two bytes from the endpoint if we have two bytes to read
  Rename i2c_slave functions so it can coexist with i2c_master (qmk#4875)
  Fix for ISO layout in tada68:rys (qmk#4906)
  [Keyboard] Added TGR Alice keyboard support (qmk#4896)
  handwired/retro_refit: refactor, Configurator support and readme update (qmk#4899)
  ...
loadedsith added a commit to loadedsith/qmk_firmware that referenced this pull request Jan 25, 2019
@loadedsith
Merge branch 'master' into grahampheath
e372e24 
* master: (60 commits)
  [Keymap] New kbd67/hotswap keymap for writing both code and math (qmk#4933)
  Add support for Clueboard 66% rev4
  Fix a typo in link to the Pro Micro ISP firmware
  [Keyboard] Remove hadron ver0 as it is no longer required (qmk#4921)
  Remove unused fn_actions[] and action_function() in default keymaps (qmk#4829)
  [Keyboard] KBD67: enable bootmagic lite by default (qmk#4931)
  [Keymap] Adding keymap for Nyquist (qmk#4918)
  Optimize/Update the new_project script (qmk#4920)
  Remove lfkeyboards parent rules.mk as its only required for mini1800
  [Keyboard] Add BDN9 (qmk#4919)
  [Keyboard] Add KBD67 Hotswap Support (qmk#4916)
  Fixup the clueboard 66 info.json
  Clueboard refresh (qmk#4902)
  Give the keymap folder the proper name
  Fix layouts/default/66_iso keymap
  [Keyboard] Add DataHand keyboard support (qmk#4847)
  [Keymap] Add a compile-time provided macro and assign to _FL (qmk#4908)
  Added info.json for TGR Alice
  Always read two bytes from the endpoint if we have two bytes to read
  Rename i2c_slave functions so it can coexist with i2c_master (qmk#4875)
  ...
zer09 pushed a commit to zer09/qmk_firmware that referenced this pull request Feb 2, 2019
@adzenith @zer09
[Keyboard] Add DataHand keyboard support (qmk#4847)
e3d373c
dlhextall pushed a commit to dlhextall/qmk_firmware that referenced this pull request May 24, 2019
@adzenith
[Keyboard] Add DataHand keyboard support (qmk#4847)
4e69ffc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noroadsleft noroadsleft noroadsleft requested changes

@drashna drashna drashna approved these changes

Assignees
No one assigned
Labels
keyboard
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adzenith @drashna @noroadsleft