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

VIA support for Planck Rev6 #9144

Merged
merged 7 commits into from
Apr 22, 2021
Merged

VIA support for Planck Rev6 #9144

merged 7 commits into from
Apr 22, 2021

Conversation

mixedfeelings
Copy link
Contributor

Description

Adds simple VIA support for Planck Rev6 with a default QWERTY layout

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

@mixedfeelings
Copy link
Contributor Author

@jackhumbert wanted me to tag him for approval of the vendor / product IDs before merging.

keyboards/planck/keymaps/via/keymap.c Show resolved Hide resolved
keyboards/planck/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/planck/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/planck/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/planck/config.h Outdated Show resolved Hide resolved
@zvecr zvecr requested review from jackhumbert and a team May 20, 2020 17:56
@zvecr zvecr added keyboard keymap via Adds via keymap and/or updates keyboard for via support labels May 20, 2020
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.

LGTM pending approval of the Vendor and Product IDs.

@noroadsleft noroadsleft requested a review from a team May 21, 2020 20:27
Copy link
Member

@Erovia Erovia left a comment

Choose a reason for hiding this comment

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

There is going to be a problem with this.
Rev6 uses a different matrix from all the others (rev1-rev5, ez).
Since the PID is defined at the keyboard level, it will be used for all revisions, VIA will recognize them but will not work.

My proposal is to:

  • either move the PID definition to the revision level for each board
  • or use one PID for rev6 (defined at the rev level) and another one for all the other boards. (since they have the same matrix layout, one VIA json should work for them)

@mixedfeelings
Copy link
Contributor Author

@Erovia it seems like the latter option would be the easiest to implement as you would only need two JSONs (for now). Perhaps the PID could refer to matrix revision rather than actual product.

@jackhumbert did you have any further thoughts on VID / PID?

Thanks for looking at this, everyone!

@Erovia
Copy link
Member

Erovia commented Jun 15, 2020

In any case, 2 json should be enough.
In the first case, the rev1-rev5,ez boards could share the same PID.

I kinda hate the idea of repeating the same thing 6 different places, but also hate the idea of having 1 exception. ¯\(ツ)

@drashna
Copy link
Member

drashna commented Jun 18, 2020

Does VIA not read the DEVICE_VER?

@noroadsleft
Copy link
Member

Does VIA not read the DEVICE_VER?

It does not.

@mixedfeelings
Copy link
Contributor Author

Thanks everyone, I guess that leaves me in a holding pattern until it is decided how to handle PIDs for the multiple Planck matrices.

@fauxpark
Copy link
Member

Personally I'd like to see VIA take into account the DEVICE_VER value. This seems like the perfect use case for it.

@Erovia
Copy link
Member

Erovia commented Jun 28, 2020

@Wilba6582 @olivia @yiancar
Could you please weigh in on this topic?

@fauxpark fauxpark mentioned this pull request Jun 30, 2020
13 tasks
@tzarc tzarc added the on hold label Jul 22, 2020
@jackhumbert
Copy link
Member

The split between the versions happens at the microcontroller level (rev1-5 vs rev6 & ez), so I think we can use these values (all have the Vendor ID of 0xFEED):

  • rev1-5: 0xAE01
  • rev6 & ez 0xA4F9

These were generated with the strings "Planck AVR" and "Planck ARM" using the (somewhat) standard command:

 echo -n "<PRODUCT>" | shasum | cut -c1-4 |tr '[:lower:]' '[:upper:]'

@mixedfeelings
Copy link
Contributor Author

mixedfeelings commented Aug 19, 2020

Good info, @jackhumbert, thank you. Do you have any thoughts on VendorID? I don't think we can use 0xFEED for VIA.

For @noroadsleft, @Erovia, @fauxpark: Does the microcontroller level distinction get us around this particular block for the purpose of officially supporting VIA for planck?

@fauxpark
Copy link
Member

I think we should probably also make a distinction between the rev6 (ARM) and EZ, which would be 0xC6CF.

@drashna
Copy link
Member

drashna commented Sep 11, 2020

For @noroadsleft, @Erovia, @fauxpark: Does the microcontroller level distinction get us around this particular block for the purpose of officially supporting VIA for planck?

It should, I think. The AVR versions have no real differences, other than backlight/rgb support, and bootloader, IIRC. Everything else is the same.

I think we should probably also make a distinction between the rev6 (ARM) and EZ, which would be 0xC6CF.

For the Planck EZ, would prefer this, specifically:

#define VENDOR_ID       0x3297
#define PRODUCT_ID      0xC6CF

@mixedfeelings
Copy link
Contributor Author

Would the olkb pcb's have the same VID as the EZ, or is that vendor ID unique to ZSA boards?

@drashna
Copy link
Member

drashna commented Sep 20, 2020

Would the olkb pcb's have the same VID as the EZ, or is that vendor ID unique to ZSA boards?

The Vender ID is unique and belongs to ZSA. It shouldn't be used for the OLKB boards (the EZ is a collaboration board between ZSA and OLKB/jack, but is sold and maintained by ZSA)

https://www.the-sz.com/products/usbid/index.php?v=0x3297&p=&n=

@fauxpark fauxpark mentioned this pull request Sep 21, 2020
14 tasks
@rstacruz
Copy link
Contributor

rstacruz commented Nov 23, 2020

Hey all, I'd like to give this a try. Can I compile a firmware from this branch, and expect it to work with the VIA app?

I assume this requires a .json file for Via with a keymap, any hints on how I can generate one?

@mixedfeelings
Copy link
Contributor Author

I assume this requires a .json file for Via with a keymap, any hints on how I can generate one?

Here you go: https://github.com/mixedfeelings/keyboards/tree/planck-rev6.1/src/olkb/planck/rev6

I haven't submitted a PR to VIA yet, as I am waiting for the blockers here to get resolved.

@mixedfeelings mixedfeelings mentioned this pull request Dec 29, 2020
14 tasks
@jackhumbert
Copy link
Member

jackhumbert commented Apr 19, 2021

Just to recap, the Planck EZ should remain untouched (it already has its VID/PID set) and these settings for config.h for the Planck Rev 1-5 will work for me:

#undef PRODUCT_ID
#define PRODUCT_ID 0xAE01

the Planck Rev 6:

#undef PRODUCT_ID
#define PRODUCT_ID 0xA4F9

and the Planck Light:

#undef PRODUCT_ID
#define PRODUCT_ID 0xBEA2

I'd like to keep 0xFEED as the Vendor ID for the above, if that's an option.

@mixedfeelings
Copy link
Contributor Author

I'd like to keep 0xFEED as the Vendor ID for the above, if that's an option.

Thanks, @jackhumbert! Understood on all the above points, but unfortunately 0xFEED can't be used for VIA. They require the Vendor ID to be unique and specific.

@jackhumbert
Copy link
Member

jackhumbert commented Apr 19, 2021

Right! Some of that is coming back to me - let's use 0x03A8 for the VID. This was going to be the QMK-wide one at some point (with PIDs generated via command I mentioned earlier), but those plans haven't really been fleshed out yet.

@mixedfeelings
Copy link
Contributor Author

mixedfeelings commented Apr 21, 2021

@jackhumbert You beat me too it, thank you so much!!

@drashna
Copy link
Member

drashna commented Apr 21, 2021

For reference, is there a Product ID to use for the Planck THK (#12597)?

@jackhumbert
Copy link
Member

For reference, is there a Product ID to use for the Planck THK (#12597)?

Just added!

@drashna drashna merged commit ffa1507 into qmk:master Apr 22, 2021
makenova pushed a commit to makenova/qmk_firmware that referenced this pull request Apr 26, 2021
Co-authored-by: Joel Challis <[email protected]>
Co-authored-by: George Wietor <[email protected]>
Co-authored-by: Jack Humbert <[email protected]>
rizo pushed a commit to rizo/qmk_firmware that referenced this pull request May 10, 2021
Co-authored-by: Joel Challis <[email protected]>
Co-authored-by: George Wietor <[email protected]>
Co-authored-by: Jack Humbert <[email protected]>
toddyamakawa pushed a commit to toddyamakawa/qmk_firmware that referenced this pull request May 19, 2021
Co-authored-by: Joel Challis <[email protected]>
Co-authored-by: George Wietor <[email protected]>
Co-authored-by: Jack Humbert <[email protected]>
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Jul 11, 2021
Co-authored-by: Joel Challis <[email protected]>
Co-authored-by: George Wietor <[email protected]>
Co-authored-by: Jack Humbert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keyboard keymap on hold via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants