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

added timber wolf keyboard #9353

Merged
merged 8 commits into from
Jul 17, 2020
Merged

added timber wolf keyboard #9353

merged 8 commits into from
Jul 17, 2020

Conversation

Croktopus
Copy link
Contributor

added firmware for timber wolf. some elements such as via keymaps and info.json arent confirmed to be working, but I think they should be good and don't know how to test without merging into QMK proper.

Description

added timber wolf keyboard, with subdirectories for the 3 versions (they differ physically in layout but share a pcb).

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

  • none

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

@zvecr zvecr added the keyboard label Jun 10, 2020
@zvecr zvecr requested a review from a team June 10, 2020 16:09
keyboards/metamechs/timberwolf/timberwolf.c Show resolved Hide resolved
keyboards/metamechs/timberwolf/timberwolf.h Outdated Show resolved Hide resolved
keyboards/metamechs/timberwolf/readme.md Outdated Show resolved Hide resolved
keyboards/metamechs/timberwolf/readme.md Outdated Show resolved Hide resolved
@zvecr zvecr requested a review from a team June 10, 2020 16:22
@zvecr
Copy link
Member

zvecr commented Jun 10, 2020

they differ physically in layout but share a pcb

If this is the case, the a, b, prime board revisions dont add a ton of value. To confirm, its exactly the same PCB between those 3 examples above?

@Croktopus
Copy link
Contributor Author

they differ physically in layout but share a pcb

If this is the case, the a, b, prime board revisions dont add a ton of value. To confirm, its exactly the same PCB between those 3 examples above?

it is exactly the same pcb, but the layout files for a TBR-Prime will not work for a TBR-A or a TBR-B, etc. which kills the idea of a default layout

@zvecr
Copy link
Member

zvecr commented Jun 11, 2020

If its one PCB, then it should be a single keyboard with no revisions and then LAYOUT macros for the available options. This means ditching the a,b,prime subfolders.

@Croktopus
Copy link
Contributor Author

Croktopus commented Jun 11, 2020

but it's a really weird pcb. https://geekhack.org/index.php?action=dlattach;topic=102520.0;attach=226645;image you see how things like the arrows are in a different place from one another and stuff?

If you let me do this I'll promise to make no rev2 pcbs, but the alternative isnt combining them and doing stuff in the layout files to fix it (because that simply won't work), it's splitting it into 3 keyboards entirely. which i can do.

@zvecr
Copy link
Member

zvecr commented Jun 11, 2020

the alternative isnt combining them and doing stuff in the layout files to fix it (because that simply won't work)

Layout macros is exactly the way to manage this, and is working perfectly in many other use cases within the repo. We dont use board revisions to do different bottom rows on a 60% keyboard. We have stuff like keebio/quefrency with its snap off sections, boardwalk with its ortho and non ortho layouts. All this is done with layout macros.

@Croktopus
Copy link
Contributor Author

Croktopus commented Jun 11, 2020

but if I'm then relying on layout macros, the default keymap & layout become useless. which seems pretty non-ideal from a user perspective. the reason i set it up as multiple boards in the first place is because in the original PR i was told i need to add a default keymap so this was my way to do that in a sensible way.

this is not just a difference in bottom rows - for example, the switch pins for right control on Prime are the same as the switch pins for left arrow on A, and they are physically separated as you can see in that image. its a similar story for the other arrows, nav cluster area, etc. its more like there are multiple physical keyboards crammed onto a single pcb.

can you explain to me why we cant do this as multiple folders? i understand that the intent was to support multiple revisions of a pcb, but it seems like it still makes sense to use that feature for this application.

Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

As this is one pcb with multiple layouts available, it should not use board revisions to separate case compatibility and should rely on layout macros to do so. Apart from providing a default keymap, any other layout macros can be represented with something like default_prime.

@drashna drashna requested a review from a team July 2, 2020 03:54
@Croktopus
Copy link
Contributor Author

im working on merging the boards into a single directory. plan to commit those changes tonight.

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.

Incompatible versions should be handled as revisions.
Common code should be at the keyboards/metamechs/timberwolf/ level, version-specific code below that in appropriately named directories.

keyboards/metamechs/timberwolf/config.h Outdated Show resolved Hide resolved
keyboards/metamechs/timberwolf/timberwolf.c Show resolved Hide resolved
keyboards/metamechs/timberwolf/timberwolf.h Show resolved Hide resolved
keyboards/metamechs/timberwolf/timberwolf.h Show resolved Hide resolved
keyboards/metamechs/timberwolf/timberwolf.h Show resolved Hide resolved
@Erovia Erovia requested review from zvecr and a team July 8, 2020 20:27
@drashna
Copy link
Member

drashna commented Jul 13, 2020

Could the docs changes listed above be applied?

@Croktopus
Copy link
Contributor Author

I've updated the readme to reflect the changes to the file structure.

HD44780_ENABLE = no # Enable support for HD44780 based LCDs
ENCODER_ENABLE = yes # Enable encoder support

BACKLIGHT_DRIVER = software
Copy link
Member

Choose a reason for hiding this comment

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

As a note, C6 should be able to do HARDWARE_PWM using timers rather than the super crude task emulation.

I wont mark it as a blocking requirement, more a "you should try it and see if its better" kinda thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing this out - that was a holdover from the last revision that used c7 or something instead

@zvecr zvecr merged commit 3c84157 into qmk:master Jul 17, 2020
@zvecr
Copy link
Member

zvecr commented Jul 17, 2020

Thanks!

Also as a little bit too late note, info.json looks to be spitting out some errors but we can easily patch that up.

@Croktopus
Copy link
Contributor Author

Croktopus commented Jul 17, 2020

looks like the errors are only occurring on layout_all right? that's strange - when i preview the info.json using ctrl+shift+i it works fine.

edit: oh wait the keycodes arent showing up either....

since it looks like there will be more changes to this, should i delete branch and start a new one or continue making changes in this branch?

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.

4 participants