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

Adapt downsteam Ghost Squid support to latest QMK #14607

Merged
merged 3 commits into from
Nov 29, 2021

Conversation

fenuks
Copy link

@fenuks fenuks commented Sep 25, 2021

Description

This changeset adapts code from https://github.com/BathroomEpiphanies/epiphanies_qmk_keyboard/tree/master/keyboards/ghost_squid_20140518 to add upstream support for Ghost Squid controller for CM Storm QuickFire XT. I didn't write that code, I just adapted it to compiler under the latest QMK as mentioned in #3952.

This is still a draft, as documentation is lacking. I will add it, but I thought it would be useful to get prior feedback before I create info.json and KLE definition. Note that I'm new to QMK, so probably there is much room for improvement. I mostly changed keymap code, as FN keys didn't work at all. I removed completely blowrak keymap found in original repo to make things simpler for me (it's, citing creator, Swedish take on Dvorak). I expect that more code can be deleted/cleaned.

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

keyboards/bpiphany/ghost_squid/config.h Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/ghost_squid.h Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/ghost_squid.c Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/keymaps/default/rules.mk Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/matrix.c Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/rules.mk Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/rules.mk Outdated Show resolved Hide resolved
@drashna drashna requested a review from a team September 28, 2021 04:29
@fenuks
Copy link
Author

fenuks commented Sep 28, 2021

Thank you for your suggestions. I applied almost all, I still read about matrix lite. I had to leave matrix_init_kb and matrix_scan_kb, otherwise code wouldn't link. Besides that, I added another layer to disable super/gui keys (keyboard allows to disable those when Fn + F9 is pressed).

@fenuks
Copy link
Author

fenuks commented Sep 28, 2021

I've been trying to convert code to matrix lite, but it doesn't work, i.e. I cannot type after I flash new firmware. My apologies if I'm asking about something obvious, but after browsing over many other matrix.c files I didn't find hints how to solve it.

I think that problem might lie in fact currently custom matrix_print and matrix_key_count act as if MATRIX_COLS <= 16, but in config.h it is defines as #define MATRIX_COLS 18, thus default implementation in matrix_common.c will print different header. Same for key count, bitpop32 will be used instead of bitpop16. I don't understand codebase well yet, so I'm not certain if that's the root of problem. Other than that, it seems matrix_is_modified also needs to be rewritten, because it works differently in matrix_common.c implementation.

@fauxpark
Copy link
Member

For Custom Matrix Lite, a lot of that stuff is not necessary as it's already implemented in core. At minimum you only need to implement the init and scan functions.

@fenuks
Copy link
Author

fenuks commented Sep 28, 2021

I tried to do that, I removed all the functions that have provided default implementation, and renamed appropriately init and scan functions, but it doesn't work. Key presses are not detected at all. I don't know how important are matrix_print and matrix_key_count, but it seems to me that in default definition they work differently because custom definitions act as if MATRIX_COLUNS was <= 16.

keyboards/bpiphany/ghost_squid/ghost_squid.c Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/keymaps/default/config.h Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/keymaps/default/keymap.c Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/readme.md Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/readme.md Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/rules.mk Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/rules.mk Outdated Show resolved Hide resolved
keyboards/bpiphany/ghost_squid/matrix.c Outdated Show resolved Hide resolved
@fenuks
Copy link
Author

fenuks commented Sep 28, 2021

@fauxpark Thank you very much for your help. Your matrix code works perfectly well. I applied other suggestions as well.

Beside more code cleaning, is there anything else you would like to be changed? I will get now to documentation, specifically diagram as requested in documentation template. Please let me know if I forgot about something.

@fenuks
Copy link
Author

fenuks commented Sep 28, 2021

I created image to visualize layers. https://imgur.com/a/zYIGf7L

If it's okay, I will update readme with short description.

@stale
Copy link

stale bot commented Nov 16, 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.

@stale stale bot removed the awaiting changes label Nov 19, 2021
@drashna drashna requested a review from a team November 28, 2021 06:52
@fenuks fenuks changed the title Draft: adapt downsteam Ghost Squid support to lastest QMK Adapt downsteam Ghost Squid support to latest QMK Nov 28, 2021
@drashna drashna merged commit 507dd18 into qmk:master Nov 29, 2021
rudism pushed a commit to rudism/qmk_firmware that referenced this pull request Jan 16, 2022
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.

3 participants