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 Canoe Gen2 #10344

Merged
merged 6 commits into from
Sep 26, 2020
Merged

Add Canoe Gen2 #10344

merged 6 commits into from
Sep 26, 2020

Conversation

evyd13
Copy link
Contributor

@evyd13 evyd13 commented Sep 17, 2020

Description

Adds the Canoe Gen2. It has per key RGB using the ws2812 protocol.

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).

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.

I'm not really a fan of the folder name. /percent/canoe/gen2 or rev2 would be better, I think.

@drashna drashna requested a review from a team September 20, 2020 23:18
keyboards/percent/canoegen2/keymaps/via/rules.mk Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/canoegen2.h Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/canoegen2.c Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/canoegen2.c Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team September 22, 2020 05:30
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.

I'm not really a fan of the folder name. /percent/canoe/gen2 or rev2 would be better, I think.

I agree with @drashna about this. The folder name should definitely be percent/canoe/gen2 or percent/canoe/rev2.


Summary of Requested Changes

  • rename LAYOUT_65_ansi to LAYOUT_65_ansi_blocker_split_bs
  • rename LAYOUT_65_iso to LAYOUT_65_iso_blocker_split_bs
  • update Community Layout support per the above (though based on keyboard renders 65_(ansi|iso) layouts are almost undoubtedly supported)

keyboards/percent/canoegen2/canoegen2.h Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/canoegen2.h Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/canoegen2.h Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/info.json Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/info.json Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/percent/canoegen2/rules.mk Outdated Show resolved Hide resolved
@str-dst
Copy link
Contributor

str-dst commented Sep 25, 2020

I'm not really a fan of the folder name. /percent/canoe/gen2 or rev2 would be better, I think.

I agree with @drashna about this. The folder name should definitely be percent/canoe/gen2 or percent/canoe/rev2.

Imho no because that would give the impression that the boards are in some way related to each other design or pcb wise which is entirely not the case.
The Gen 2 is a complete redesign with an entirely different PCB (the gen 1 used a ps2avr PCB).
The only similarity between these boards is that they are called Canoe.

The Gen2 is not just a revision of the Gen1

@fauxpark
Copy link
Member

The same is technically true of the Planck and Preonic...

@str-dst
Copy link
Contributor

str-dst commented Sep 25, 2020

Putting the boards in one folder would be like putting Keycult boards in one folder named TKL with subfolders named No1 and No2 because "technically" they are just another version of the Keycult TKL.

It doesn't make sense because it's not just some revision of the board.
The first board was called Canoe. The second Canoe Gen2.
So it makes sense to put it in a folder called canoegen2.

@drashna drashna self-requested a review September 25, 2020 19:01
@drashna
Copy link
Member

drashna commented Sep 25, 2020

The first board was called Canoe. The second Canoe Gen2.

Both boards use the same layout, correct? If so, then revision works fine here.
That's fauxparks point with the planck and preonic. Specifically, there are 4 different planck boards, the Rev1-5, the Rev6, the EZ, and the Light. They're all "planck" but the matrix and features and controller are all different. Rev1-5 uses atmega32u4, the rev6 and EZ use stm32f303, and the light uses at90usb1286. The EZ and Light use per key RGB, using different ISSI controllers), etc.

So having a different controller, a different matrix and different features really isn't a good reason to have a completely different name and folder.

And there are plenty of other boards that follow this pattern too (for instance, the helix).

So it makes sense to put it in a folder called canoegen2.

IMO, no it doesn't, given the precedent already in the repo.

However canoe/gen2 would also be fine. and looks better than canoegen2.

@Erovia
Copy link
Member

Erovia commented Sep 25, 2020

So it makes sense to put it in a folder called canoegen2.

Not just looks better, but is what the users familiar with QMK are expecting.

@jackhumbert
Copy link
Member

It doesn't make sense because it's not just some revision of the board.

I think "gen" is functionally the same as "rev" here, so should get the same treatment.

Usually when introducing another revision, what code was previously just keyboard/ gets moved to keyboard/rev1/, and any shared code can be moved to back to keyboard/. This allows folks to easily use the same keymap between both revisions without having to copy it into the new keyboard's folder.

@str-dst
Copy link
Contributor

str-dst commented Sep 25, 2020

It doesn't make sense because it's not just some revision of the board.

I think "gen" is functionally the same as "rev" here, so should get the same treatment.

No it is not. Again: this is not a "revision" of the board.
It's an entirely new design and should be treated as such.

Just because they are the same layout doesn't make them the same board.
The boards do not share anything but the layout and a part of the name.

Moving the old board in a folder named gen1 makes even less sense since there is no Canoe Gen1. It's only "Canoe".

Putting them in the same folder would most likely cause confusion with the users since it would give the impression that the boards are in some way compatible with each other which is not the case.

@str-dst
Copy link
Contributor

str-dst commented Sep 25, 2020

The first board was called Canoe. The second Canoe Gen2.

Both boards use the same layout, correct? If so, then revision works fine here.

This doesn't make any sense at all.
Just because a board has the same layout doesn't mean that it's in any way a similar keyboard.
By that logic we could just place all boards by the same designer in folders named after the layouts.

The planck example does not apply here because the planck with all it's PCB revisions is still the same board and all the PCBs are compatible with the Planck case.
This is simply not the case with the Canoe Gen2.

Again: This is an entirely new design and the boards are in no way compatible with each other.
The Gen2 PCB does not fit the original Canoe case and vice versa.
Placing the two boards in the same folder would imply that the boards are compatible with each other.

@fauxpark
Copy link
Member

it would give the impression that the boards are in some way compatible with each other which is not the case.

The only compatibility that really matters here is the layout support, as we aim to abstract away the hardware-specific stuff for exactly this reason.

The planck example does not apply here because the planck with all it's PCB revisions is still the same board

As Drashna mentioned above, the different revisions of the Planck use different MCUs (and I believe the rev6 was designed by someone else whereas most/all of the earlier revs were designed by Wilba). So I think it very much applies here.

The very fact that this board is named "Gen2" kind of implies that a gen1 also exists. I'd argue that is more confusing to users.

@str-dst
Copy link
Contributor

str-dst commented Sep 25, 2020

it would give the impression that the boards are in some way compatible with each other which is not the case.

The only compatibility that really matters here is the layout support, as we aim to abstract away the hardware-specific stuff for exactly this reason.

The planck example does not apply here because the planck with all it's PCB revisions is still the same board

As Drashna mentioned above, the different revisions of the Planck use different MCUs (and I believe the rev6 was designed by someone else whereas most/all of the earlier revs were designed by Wilba). So I think it very much applies here.

The very fact that this board is named "Gen2" kind of implies that a gen1 also exists. I'd argue that is more confusing to users.

"Canoe Gen2" is the name of the board. It's not "Canoe" generation 2 or something. Yes technically this could be seen like this but it is meant as the name of the board here.

And again it is not just the MCU or matrix that is different on the new PCB. It does not fit the original Canoe case.

@fauxpark
Copy link
Member

So why does the website say "revision"?

As for it not fitting the case - AFAICT it doesn't seem like you can buy the PCB on its own, so that doesn't matter anyway.

@str-dst
Copy link
Contributor

str-dst commented Sep 25, 2020

So why does the website say "revision"?

As for it not fitting the case - AFAICT it doesn't seem like you can buy the PCB on its own, so that doesn't matter anyway.

You can't right now but there are more PCBs planned. A hotswap PCB should become available later this year for the board as well as a separately available soldered PCB.

@jackhumbert
Copy link
Member

jackhumbert commented Sep 25, 2020

After talking to kingnestea on the Percent Studio discord, they're ok with /percent/canoe_gen2 - if you can update the keyboard name to that, that'd be great!

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.

#10344 (comment) still needs changing; the via keymap won't compile without this change (LAYOUT_65_iso doesn't exist).

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.

@noroadsleft noroadsleft merged commit 1b7101f into qmk:master Sep 26, 2020
@noroadsleft
Copy link
Member

Thanks!

@evyd13
Copy link
Contributor Author

evyd13 commented Sep 26, 2020

Thank you!

@evyd13 evyd13 mentioned this pull request Sep 26, 2020
6 tasks
@evyd13 evyd13 deleted the canoegen2 branch September 26, 2020 05:50
tomohisa pushed a commit to tomohisa/qmk_firmware that referenced this pull request Sep 28, 2020
* 'master' of https://github.com/qmk/qmk_firmware: (3643 commits)
  Swapparoo of bottom row keys (qmk#10277)
  [Keyboard] Add PloopyCo devices (qmk#7935)
  Keyboard update: SL40 (qmk#10445)
  [keyboard] Add SP-111 support (qmk#10193)
  Add logic for AT90USBxx7 where needed (qmk#10203)
  Trifecta Keymap
  [Docs] Wording & formatting changes in the Tapping Force Hold section of the Tap Hold page + removed trailing whitespaces (qmk#10391)
  `setrgb()`: Use arrow operator (qmk#10451)
  [Keyboard] add Percent Canoe Gen2 (qmk#10344)
  [Keyboard] add hannah65 by Team Mechlovin (qmk#10284)
  Fix Belgian sendstring properly (qmk#10444)
  Add VIA support for lazydesigners/the30 (qmk#10374)
  [Keyboard] add duckboard by doodboard (qmk#10318)
  Fix Belgian sendstring file (qmk#10443)
  [Keyboard] added Bolsa65 keyboard by FJLabs (qmk#10394)
  CLI/Doctor: Print QMK_HOME (qmk#10398)
  Add Wyvern Keyboard (qmk#10378)
  Adding Bear 65 Ergo Keyboard To QMK (qmk#10384)
  VIA Support: Gray Studio HB85 (qmk#10329)
  format code according to conventions [skip ci]
  ...
rgoulter pushed a commit to rgoulter/qmk_firmware that referenced this pull request Oct 4, 2020
* Add Canoe Gen2

* Fix info.json

* Update info.json

* Changes

* Move canoegen2 to canoe_gen2

* Update canoe_gen2.h
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
* Add Canoe Gen2

* Fix info.json

* Update info.json

* Changes

* Move canoegen2 to canoe_gen2

* Update canoe_gen2.h
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.

7 participants