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

New Keyboard: XD68 65% ATMega32U4 based #7395

Merged
merged 15 commits into from
Dec 16, 2019
Merged

Conversation

randlor
Copy link
Contributor

@randlor randlor commented Nov 17, 2019

Adding support for XD68 PCB.

Description

Standard ATMega32U4 spec board with a default ANSI and ISO UK layout. Backlight and RGB support.

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.
  • 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/xd68/config.h Outdated Show resolved Hide resolved
keyboards/xd68/config.h Outdated Show resolved Hide resolved
keyboards/xd68/config.h Outdated Show resolved Hide resolved
keyboards/xd68/keymaps/default/config.h Outdated Show resolved Hide resolved
keyboards/xd68/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/xd68/xd68.h Outdated Show resolved Hide resolved
keyboards/xd68/xd68.h Outdated Show resolved Hide resolved
keyboards/xd68/xd68.c Outdated Show resolved Hide resolved
keyboards/xd68/xd68.c Outdated Show resolved Hide resolved
keyboards/xd68/xd68.c Outdated Show resolved Hide resolved
@fauxpark fauxpark requested a review from a team November 18, 2019 00:56
@randlor
Copy link
Contributor Author

randlor commented Nov 18, 2019

Thanks for the review @fauxpark, changes made as per your recommendations.

keyboards/xd68/xd68.h Outdated Show resolved Hide resolved
keyboards/xd68/rules.mk Outdated Show resolved Hide resolved
keyboards/xd68/keymaps/default_iso/config.h Outdated Show resolved Hide resolved
@randlor
Copy link
Contributor Author

randlor commented Nov 18, 2019

Thanks again @fauxpark, apologies - missed those last few bits.

keyboards/xd68/readme.md Outdated Show resolved Hide resolved
keyboards/xd68/rules.mk Outdated Show resolved Hide resolved
@noroadsleft
Copy link
Member

@fauxpark's initial review has a couple unresolved issues:

  • keyboards/xd68/config.h should use #pragma once instead of #ifndef/#define/#endif
  • He wanted the readme's make line regarding the ISO keymap deleted, but I think I have a slight preference for leaving it in. I won't be too upset if it gets removed though. Basically, whatever you two decide for this is fine by me.

@noroadsleft
Copy link
Member

Also, for future reference, we recommend against committing to your master branch as you've done here, because pull requests from modified master branches can make it more difficult to keep your QMK fork updated. It is highly recommended for QMK development – regardless of what is being done or where – to keep your master updated, but NEVER commit to it. Instead, do all your changes in a branch (branches are basically free in Git) and issue PRs from your branches when you're developing.

There are instructions on how to keep your fork updated here:

Best Practices: Your Fork's Master: Update Often, Commit Never

If you need any help with this just ask. Until your pull request is merged, please continue to commit relevant changes to your master branch.

@randlor
Copy link
Contributor Author

randlor commented Nov 18, 2019

Hi @noroadsleft,

Amended config.h, sure I did that on the first tranche of updates but it was early and not enough coffee.

I'm happy with whatever you guys recommend on the make statements in readme, whatever is most consistent. We could change to a single make statement, and then reference the other available maps as those familiar with QMK should know how to invoke them at build.

Thanks for the heads up on the fork/branch best practice, I'll keep that in mind for any future commits.

All the best

@randlor
Copy link
Contributor Author

randlor commented Nov 18, 2019

I've created a new branch btw, would you like me to resubmit my PR from that one and close this out out?

@noroadsleft
Copy link
Member

noroadsleft commented Nov 18, 2019

I'm happy with whatever you guys recommend on the make statements in readme, whatever is most consistent. We could change to a single make statement, and then reference the other available maps as those familiar with QMK should know how to invoke them at build.

I've had this kicking around in my head for a while and have yet to come up with a good solution to my mind. We require a keyboard to have a default keymap. Our keyboard guidelines dictate that a keyboard that supports multiple physical layouts should have a keymap for each layout to be used as a reference, but we don't strictly enforce this (and with some keyboards it can be quite a chore to do that).

As a result, some boards that support (for instance) ANSI and ISO layouts only include an ANSI-compatible keymap. I think discoverability is an issue here – who can say whether a user is going to actually look in the keymap folder, especially when our own script for starting new user keymaps explicitly copies the default. (This functionality will likely move to our Python CLI sooner or later.) Being able to point out "Hey, there's an ISO keymap here" is a good thing IMO. I just haven't figured out the best way to convey this.


EDITED TO ADD:

As for the new branch, you can and should go ahead and continue using your master branch until we merge this PR. You will need to resynchronize your master branch afterward. Your xd68 branch may not be necessary (though if you need to make further changes after this PR is merged, you should use a new branch).

|------------------------------------------------------- -----|
|CAPS | A| S| D| F| G| H| J| K| L| ;| '| #|Entr|PgDn|
|----------------------------------------------------------------|
|Shift| \ | Z| X| C| V| B| N| M| ,| .| /|Rshift|Up|End |
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
|Shift| \ | Z| X| C| V| B| N| M| ,| .| /|Rshift|Up|End |
|Shift| \ | Z| X| C| V| B| N| M| ,| .| /|Rshift| Up|End |

@randlor
Copy link
Contributor Author

randlor commented Nov 18, 2019

I've had this kicking around in my head for a while and have yet to come up with a good solution to my mind. We require a keyboard to have a default keymap. Our keyboard guidelines dictate that a keyboard that supports multiple physical layouts should have a keymap for each layout to be used as a reference, but we don't strictly enforce this (and with some keyboards it can be quite a chore to do that).

As a result, some boards that support (for instance) ANSI and ISO layouts only include an ANSI-compatible keymap.

It's an interesting challenge, sure it could be solved with enforcement of guidelines but that I suppose that would hamper development and adoption.

Maybe a base minimum of layouts for new boards would be a possible approach, say ANSI and ISO?

Then for existing boards, add "currently supported/missing layouts" columns to the All Supported Keyboards list?

I think discoverability is an issue here – who can say whether a user is going to actually look in the keymap folder, especially when our own script for starting new user keymaps explicitly copies the default. (This functionality will likely move to our Python CLI sooner or later.) Being able to point out "Hey, there's an ISO keymap here" is a good thing IMO. I just haven't figured out the best way to convey this.

I guess another question here is "if a layout in QMK for a particular board didn't exist, would that stop me using it?" To the end, if you're at a point where you're building a board and compiling custom firmware you're probably over half way to making your own layout should you need one.
Say someone was willing to roll their own, then the key part for me is making sure that layout is captured for others to use.

If the Python CLI had a means to create additional maps, sanity check them and then somehow push to Git (with the author's consent of course) and update the available layouts that would in theory serve as a means to organically "fill in the blanks" over time. Then you just need to get people (like me, sorry!) to use that tooling.

@noroadsleft
Copy link
Member

It's an interesting challenge, sure it could be solved with enforcement of guidelines but that I suppose that would hamper development and adoption.

Maybe a base minimum of layouts for new boards would be a possible approach, say ANSI and ISO?

Then for existing boards, add "currently supported/missing layouts" columns to the All Supported Keyboards list?

A decently sized part of our lax approach to enforcing this guideline is that as QMK's userbase has grown, we've gained more users who don't have the necessary knowledge or a full grasp of how everything works and how the different bit interconnect. Sometimes, we get users who know C very well, but can't make heads or tails of using Git. Sometimes it's people who know Git but have no experience with C and just copied an existing board then fiddled with the different bits until the hardware worked. We're trying to lower the barrier of entry – both in effort and knowledge required – not raise it.

Additionally, each of us on the core team – those of us with merge permissions – has varying experience and knowledge. My C skills are very low level, such that I only understand QMK Firmware as implemented on AVR hardware. My specialty and area of expertise is QMK Configurator implementations. My work on that side of things has exposed me to most of the QMK bits that I do understand. @fauxpark's C skills are far beyond my own, and in his relatively short time with us he's made a number of improvements that have made QMK's source easier to understand and use. This is nothing to say of the amounts of available time each of us have for working on QMK. Skullydazed in particular has been doing a lot of work that's been relatively under the radar. There are QMK projects under way that the general public knows nothing about.

I'd be hesitant to make any changes to the Supported Keyboards list, as that page actually gets regenerated every time we merge a PR to qmk_firmware or push an update, and having it list the available layouts is a difficult problem that may be best solved elsewhere. I see this similarly to the same reason QMK Configurator doesn't use the default keymaps that are in qmk_firmware, even though its supporting API knows they exist. Parsing C into text is hard. It gets easier if you strictly enforce coding standards, but I don't think that's an avenue we're interested in pursuing at this time. Everyone writes their code differently.

I guess another question here is "if a layout in QMK for a particular board didn't exist, would that stop me using it?" To the end, if you're at a point where you're building a board and compiling custom firmware you're probably over half way to making your own layout should you need one.
Say someone was willing to roll their own, then the key part for me is making sure that layout is captured for others to use.

If the Python CLI had a means to create additional maps, sanity check them and then somehow push to Git (with the author's consent of course) and update the available layouts that would in theory serve as a means to organically "fill in the blanks" over time. Then you just need to get people (like me, sorry!) to use that tooling.

We do get pull requests to add new layouts to existing hardware, and that's perfectly fine by us. I recently merged this PR which adds the physical layout used in Brazil to the DZ60. A lot of these, the user has added the source C needed to support the layout, but not made the changes to expose it accurately in QMK Configurator. Sometimes our efforts to get the user to do this work has led to their confusion – they understand the C, but the way the keyboard's files work together bewilders them. In those cases I usually go through and fix it myself (again, my area of expertise) after it's merged.

Getting users to use the available tooling is another thing, as you've said. At this point, I'm not actually sure if the new keymap Python tooling is ready to replace the shell script, and as I don't know Python I can't really help in that regard. A lot of things in QMK I understand conceptually – my background is front-end web development – so I may understand what a block of code does, but not how it does it.

In any case, we're always grateful to our users for their contributions. QMK wouldn't be where it is today without our passionate and dedicated community. We're pretty much always looking for ways to improve the user experience.

keyboards/xd68/rules.mk 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! Sorry for losing track of this!

@noroadsleft noroadsleft merged commit 74dc65a into qmk:master Dec 16, 2019
benjaminmikiten added a commit to benjaminmikiten/qmk_firmware that referenced this pull request Dec 18, 2019
* master: (99 commits)
  [Keymap] Added userspace for d4mation. Included their keymap for the Atreus62 (qmk#7483)
  [Keymap] Custom user keymap for Think6.5 with LED range control (qmk#7603)
  [Keymap] CRKBD Custom Keymap - KidBrazil (qmk#7630)
  [Keymap] Add pico 70 keys keymap (qmk#7654)
  Tidy up dztech default keymaps and info.json (qmk#7608)
  Heisenberg handwired keyboard added (qmk#7643)
  [Keyboard] Added Filco Majestouch TKL Pegasus Hoof ISO Layout (qmk#7647)
  Ported J80 to QMK (qmk#7488)
  [Keyboard] Magnavox Videowriter conversion with Pro Micro (qmk#7634)
  [Docs] add japanese translation (basic part) (qmk#7461)
  Tidy up dztech rules.mk
  Relocate RGB keycode processing (qmk#7508)
  Move kwerdenker's personal keymap from RGB (qmk#7645)
  Remove QMK_KEYBOARD_CONFIG_H from boards (qmk#7635)
  Disable usb on slave half to resolve random 'lockup' (qmk#7649)
  [Core] Optimize matrix processing (qmk#7621)
  [Keymap] boy_314's satisfaction75 layout (qmk#7638)
  [Keyboard] XD68 65% ATMega32U4 based (qmk#7395)
  [keyboard] Plain60 cleanups (qmk#7644)
  update default h88 keymap (qmk#7646)
  ...
patrl pushed a commit to patrl/qmk_firmware that referenced this pull request Dec 29, 2019
* First working draft

* Updated readme.md

* Fixed url

* Typo fix

* RGB + Backlight working

* Fixed matrix for ISO NUHS

* ISO matrix working

* Adding ANSI default layout

* First release commit

* Removed reference to deprecated layout

* Changes from PR qmk#7395 review

* Additional changes as requested for PR qmk#7395

* Additional changes from @noroadsleft review

* Replaced ifndef/endif with pragma

* Adding yanfali's recommended changes
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020
* First working draft

* Updated readme.md

* Fixed url

* Typo fix

* RGB + Backlight working

* Fixed matrix for ISO NUHS

* ISO matrix working

* Adding ANSI default layout

* First release commit

* Removed reference to deprecated layout

* Changes from PR qmk#7395 review

* Additional changes as requested for PR qmk#7395

* Additional changes from @noroadsleft review

* Replaced ifndef/endif with pragma

* Adding yanfali's recommended changes
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