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 support for "The Ave." keyboard #10042

Merged
merged 39 commits into from
Nov 10, 2020
Merged

Conversation

The-Royal
Copy link
Contributor

@The-Royal The-Royal commented Aug 15, 2020

Description

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

@The-Royal
Copy link
Contributor Author

-___-

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 its 2 physically different PCBs, it should be split into 2 sub revisions keyboards/kingly_keys/ave/staggered and keyboards/kingly_keys/ave/ortho.

@zvecr zvecr added the keyboard label Aug 16, 2020
@The-Royal
Copy link
Contributor Author

As its 2 physically different PCBs, it should be split into 2 sub revisions keyboards/kingly_keys/ave/staggered and keyboards/kingly_keys/ave/ortho.

That was the original plan. I wasn't sure on exactly if that was the right idea cause they are two different PCBs...but they use the exact same pin-outs, config and mapping. It could have been a single PCB but I don't like swiss cheese PCBs lol.

@The-Royal
Copy link
Contributor Author

@zvecr How do I structure the split? Two different keyboard folders or staggered subfolder (keymap, json, info, and config) within the main?

@The-Royal
Copy link
Contributor Author

Future revisions/sales of the pcb might actually be just combined (staggered and ortho) onto the single pcb

@LostQuasar
Copy link
Contributor

@zvecr How do I structure the split? Two different keyboard folders or staggered subfolder (keymap, json, info, and config) within the main?

look into pancake pro micro and pancake BLE those might help you understand to make different versions

@The-Royal
Copy link
Contributor Author

@zvecr Do I still need the initial ave.c and ave.h files if the sub-folders contain the more consolidated configurations?

@LostQuasar
Copy link
Contributor

Since the two variations are widely different I would treat them as 2 separate keyboards and leave the root folder empty

@The-Royal
Copy link
Contributor Author

Alright. Removed the the two unnecessary files. :)

@zvecr zvecr requested a review from a team August 18, 2020 23:06
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.

GitHub tip: You can apply multiple suggestions to a single commit by using the Files Changed tab.

keyboards/kingly_keys/ave/config.h Outdated Show resolved Hide resolved
keyboards/kingly_keys/ave/ortho/info.json Outdated Show resolved Hide resolved
keyboards/kingly_keys/ave/ortho/info.json Outdated Show resolved Hide resolved
keyboards/kingly_keys/ave/readme.md Outdated Show resolved Hide resolved
keyboards/kingly_keys/ave/staggered/info.json Outdated Show resolved Hide resolved
keyboards/kingly_keys/ave/staggered/info.json Outdated Show resolved Hide resolved
keyboards/kingly_keys/ave/readme.md Outdated Show resolved Hide resolved
The-Royal and others added 2 commits August 19, 2020 16:56
Co-authored-by: James Young <18669334+noroadsleft@users.noreply.github.com>
Co-authored-by: James Young <18669334+noroadsleft@users.noreply.github.com>
@The-Royal
Copy link
Contributor Author

Yeah I think every PR I have open has had some issue with the Tapping term specifically.

It was working perfectly before the PR with the same Rev split into two keymaps of Ortho/staggered.

Travis doesn’t seem to have an issue with the build strangely enough.

@ridingqwerty
Copy link
Contributor

Travis doesn’t seem to have an issue with the build strangely enough.

Yeah I did notice that. I would try building yourself anyway, but maybe it's just me. I could definitely make kingly_keys/romac:default and other stuff no problem.

@noroadsleft
Copy link
Member

I feel like this isn't correct...

@fauxpark
Copy link
Member

Yep, rules.mk is missing from the staggered and ortho directories.

@The-Royal
Copy link
Contributor Author

Yep, rules.mk is missing from the staggered and ortho directories.

Do I need one in the parent board directories too?

Or should i just duplicate and move the rules.mk from the master parent to the sub directories?

There is one for each default Keymap currently

@noroadsleft
Copy link
Member

Do I need one in the parent board directories too?

Or should i just duplicate and move the rules.mk from the master parent to the sub directories?

Yeah, Take keyboards/kingly_keys/ave/rules.mk and move it to the ortho directory, and then copy it to the staggered directory. So:

cd keyboards/kingly_keys/ave/
git mv -- rules.mk ortho/rules.mk
cp ortho/rules.mk staggered/rules.mk
git add -- staggered/rules.mk
git commit -m "move keyboard rules.mk to version level"

keyboards/kingly_keys/ave/readme.md Outdated Show resolved Hide resolved
The-Royal and others added 2 commits October 28, 2020 06:52
keyboards/kingly_keys/ave/config.h Outdated Show resolved Hide resolved
Co-authored-by: Ryan <fauxpark@gmail.com>
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 53f1166 into qmk:master Nov 10, 2020
@noroadsleft
Copy link
Member

Thanks!

ChrissiQ pushed a commit to ChrissiQ/qmk_firmware that referenced this pull request Nov 10, 2020
* Add configurator support for "The Ave." keyboard

* Update readme.md

* update misc nomenclature

* add punctuation

* fix miss-placed "0"

* update README.md

* del. duplicate "F11" in visual layout information

* Split PCB configurations into subfolders

* update layer template to new matrix title

* rm primary <keyboard>.c / <keyboard>.h files

* add new end line to both .h subfolder fiels

* Apply suggestions from code review

* tested update to RGB code

* update rgb code

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Update rules.mk

* Update rules.mk

* Delete rules.mk

* Delete rules.mk

* Update rules.mk

* Update config.h

* Update config.h

* Update keymap.c

* Update ortho.c

* Update ortho.h

* Update config.h

* Update keymap.c

* Update staggered.c

* Update staggered.h

* Apply suggestions from code review

* Update config.h

* Update keymap.c

* move and duplicate rules.mk file

* Update keyboards/kingly_keys/ave/readme.md

* Update config.h

* Update keyboards/kingly_keys/ave/config.h
@mtei
Copy link
Contributor

mtei commented Nov 14, 2020

This PR#10042 has a build error, but it has been merged. Why?

@fauxpark
Copy link
Member

fauxpark commented Nov 14, 2020

The build failure is not really anything to do with this keyboard's code itself - it's complaining that TAPPING_TERM is not defined, which is technically true as far as the keymap.c is concerned. TAPPING_TERM, if not defined in config.h, is defaulted to 200 in action_tapping.h, however this header is only included in a handful of .c files so it will not be visible to keymap.c.

It should be moved somewhere both action_tapping.c and quantum.h can see it. Maybe action.h?

@mtei
Copy link
Contributor

mtei commented Nov 14, 2020

workaround:

diff --git a/keyboards/kingly_keys/ave/post_config.h b/keyboards/kingly_keys/ave/post_config.h
new file mode 100644
index 000000000..9a69b420f
--- /dev/null
+++ b/keyboards/kingly_keys/ave/post_config.h
@@ -0,0 +1,4 @@
+#ifndef TAPPING_TERM
+#    define TAPPING_TERM 200
+#endif
+

@drashna
Copy link
Member

drashna commented Dec 5, 2020

This PR#10042 has a build error, but it has been merged. Why?

I have a fix coming for this. That should help detect when tapping term is incorrectly set, either the tapping term itself or the functions.

Which is mostly... just adding action_tapping.h to quantum.h, and adding the function prototypes to action_tapping.h. This should fix this issue, and better handle things everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants