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 gBoards GergoPlex firmware #10406

Closed
wants to merge 14 commits into from
Closed

Add gBoards GergoPlex firmware #10406

wants to merge 14 commits into from

Conversation

pirj
Copy link

@pirj pirj commented Sep 23, 2020

Description

Add firmware for gBoards GergoPlex.

This PR differ from the original GergoPlex pull request:

  • rebased on master (in hopes for breaking changes to be removed, since it doesn't affect any existing firmware, but introduces a new one)
  • [...] code review notes addressed
  • code layout fixed
  • prettified layouts (at the cost of using LAYOUT_kc instead of LAYOUT_split_3x5_3, asked a question how to benefit from both)
  • removed Via layout (it doesn't support chords/combos and is useless on a 35% keyboard)

All @germ's original commits are left intact.

Not done

Is a custom matrix really needed?

According to custom_matrix.md:

The reasons to use [custom matrix] ... I/O multiplexer

And MCP23018, I/O port expander, is used for the left half of the split keyboard.

feature_split_keyboard.md suggests:

assumes that you're using two Pro Micro-compatible controllers

And GergoPlex is only using one AVR, and MCP is used for the other half (just like ErgoDox EZ, torn, spiderisland/split78, moonlander, ymdk/sp64, wheatfield/split75, sx60, net_type_a, hotdox, handwired/ferris, ergotaco, other gBoards keyboards, and a fair number of keyboards using PORT_EXPANDER_ADDRESS in their firmwares).

I hope to get rid of at least a part of the custom matrix code using MATRIX_ROW_PINS, MATRIX_COL_PINS, DIODE_DIRECTION.
Also to only keep a fraction of MCP23018 code with SPLIT_KEYBOARD=true and SPLIT_TRANSPORT=custom, SOFT_SERIAL_PIN, USE_I2C etc.
Will do this in a follow-up, since it's not too straightforward.

Types of Changes

  • Keyboard (addition or update)

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

@pirj pirj marked this pull request as draft September 23, 2020 15:12
@pirj pirj mentioned this pull request Sep 23, 2020
15 tasks
@drashna drashna requested a review from a team October 4, 2020 20:09

// unused pins - C7, D4, D5, D7, E6
// set as input with internal pull-up enabled
DDRB &= ~(1 << 0 | 1 << 4 | 1 << 5 | 1 << 6 | 1 << 7);
Copy link
Author

Choose a reason for hiding this comment

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

I intend to replace this according to this comment:

We rather use the GPIO control functions: https://docs.qmk.fm/#/internals_gpio_control?id=functions

@baylessj
Copy link

baylessj commented Oct 6, 2020

Hi @pirj! I just received my Gergoplex and I'm looking to start personalizing my keymap. I don't see the Gergoplex layout in master yet, should I create a fork off of yours to start my modifications? I also see a couple other PRs opened by Germ for this board, not sure which one is the best base to work off of.

@pirj
Copy link
Author

pirj commented Oct 6, 2020

Hey @baylessj . Got ahead, check out this branch and test it. It's currently driving my GergoPlex with no issues. It's a cleanup and minor refactoring of the original pull request. I plan to work on this even further, but that shouldn't prevent you from hacking in your layout - default/keymap.c is very unlikely to undergo any changes.

@baylessj
Copy link

baylessj commented Oct 8, 2020

Thanks for working on this @pirj, currently typing on my fork of your fork. Took a bit to get used to the organization of the key combo code when modifying that but everything else was smooth sailing with making my keymap.

@caksoylar
Copy link

FWIW I also have been running this branch (with @drashna's suggested changes on the layout definition applied) with my keymap for a week or so and haven't had any issues.

@pirj pirj mentioned this pull request Nov 28, 2020
2 tasks
@manna-harbour
Copy link
Contributor

Here is a commit to add split_3x5_3 LAYOUT support to this branch: manna-harbour@bba7a50

That branch can be used to build miryoku for the gergoplex by merging in the miryoku development branch.

@ghost
Copy link

ghost commented Nov 30, 2020

Here is a commit to add split_3x5_3 LAYOUT support to this branch: manna-harbour@bba7a50

That branch can be used to build miryoku for the gergoplex by merging in the miryoku development branch.

I can confirm merging manna-harbour@bba7a50 and manna-harbour:miryoku into pirj:prettify-gergoplex builds and is functional on my GergoPlex with the command make gboards/gergoplex:manna-harbour_miryoku:flash
Thanks for the help @manna-harbour!

@stale
Copy link

stale bot commented Jan 15, 2021

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@bttnns
Copy link

bttnns commented Jan 25, 2021

What else is left todo here?

sorry, new around here and have a gergoplex =)

@yorickpeterse
Copy link

Same here: I recently got a Gergoplex as well and have been using this PR, which thus far works great. It would be nice to see this merged into QMK at some point 😄

@pirj
Copy link
Author

pirj commented Jan 26, 2021

Sorry, it is taking me longer to get back to this.
I'm going to:

  • make changes requested during the review
  • try my chances with reusing existing code from custom_matrix=lite
    and re-request a review.

@yorickpeterse
Copy link

Any progress on this pull request? I'd love to help out in whatever way I can, but my knowledge of QMK internals is pretty much non-existing. If anything needs testing, let me know 😄

@stale
Copy link

stale bot commented Apr 7, 2021

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.

@stale stale bot closed this Apr 7, 2021
@baylessj
Copy link

baylessj commented Apr 7, 2021

This really should still get merged in... @pirj are you able to get back to those action items you listed in your comment above? Or would you prefer if someone else takes over the finishing touches on the PR?

@pirj
Copy link
Author

pirj commented Apr 7, 2021

@baylessj Not yet. There are a few cosmetic things that need to be done. I appreciate if someone takes this over. Or I can do it, but I'd love to take a chance with reusing the code from ergodox, and this will take longer than just cosmetic changes.

@pirj
Copy link
Author

pirj commented May 28, 2021

PR updated. @drashna, please have a look.
I think I've addressed your concerns. Reopening the PR did not happen automatically, and I don't have an option to do so.

What has been done:

  • rebased on fresh master
  • got rid of LAYOUT_kc favoring LAYOUT_split_3x5_3
  • fixed matrix custom C code to build with updated external dependencies (gcc) (phex -> print_hex8, pbin_reverse16 -> print_bin_reverse16)
  • changed VENDOR_ID to 0x6B0A(rds) to match gBoards and not be suspiciously low (0x0007)
  • fixed combo layout docs
  • reworked custom matrix code not to use PORTx/DDRx directly
  • removed redundant #define statements
  • slightly updated docs

What has NOT been done:

  • in the custom matrix code rows and cols seem to be mixed up
  • the code is reading directly using PINF
  • KC_RMB/KC_LMB don't seem to work fixed in gBoards GergoPlex #13027
  • SPLIT_KEYBOARD=yes

@germ do you mind having a look, and test it out, too?
@yorickpeterse, @baylessj, @stjoni, @caksoylar, @btannous appreciate if you can test this, too.

// unused pins - C7, D4, D5, D7, E6
// set as input with internal pull-up enabled
DDRB &= ~(1 << 0 | 1 << 4 | 1 << 5 | 1 << 6 | 1 << 7);
PORTB |= (1 << 0 | 1 << 4 | 1 << 5 | 1 << 6 | 1 << 7);
Copy link
Author

Choose a reason for hiding this comment

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

I don't understand what's going on, it's updated in my branch that is linked to this PR https://github.com/pirj/qmk_firmware/blob/prettify-gergoplex/keyboards/gboards/gergoplex/gergoplex.c, but not here.

Copy link
Member

Choose a reason for hiding this comment

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

GitHub probably doesn't update closed PRs.

@pirj pirj mentioned this pull request May 28, 2021
8 tasks
@pirj
Copy link
Author

pirj commented May 28, 2021

This PR doesn't seem to be updated with my branch updates for some reason.
I've opened up #13027. Sorry for the trouble.

@bttnns
Copy link

bttnns commented May 28, 2021

cloned pirj's branch, built, flashed and am running this latest update on my gergoplex, things seem OK - will update if something seems broken as I use the keyboard today

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