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

Extended space cadet to support ctrl and alt #5226

Closed
wants to merge 19 commits into from

Conversation

XScorpion2
Copy link
Contributor

@XScorpion2 XScorpion2 commented Feb 23, 2019

Description

Extending the space cadet concept to have versions for alt and ctrl.

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

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Feb 23, 2019

As an example, my preonic uses [ ] for shift and - = for ctrl as it already has the number row for ( ).

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Feb 23, 2019

Failures are due to increased firmware size. After last commit, these changes increase firmware by 254 bytes (locally, CI server printed values look higher than local values, not sure why), lily58/rev1 (worst overage) only has 66 bytes free before the changes. I could add a define to enable this so it is disabled by default so the boards now over firmware size can keep working without changes. Thoughts?

Another note: If PR #5151 is merged in, it reduces the firmware size back down to passing levels. (62 bytes free)

@ezuk
Copy link
Contributor

ezuk commented Feb 25, 2019

I only reviewed the documentation (rather than the actual implementation code) and wanted to say it's great. Very nicely updated!

@drashna
Copy link
Member

drashna commented Feb 28, 2019

@ezuk from the looks of it, most of this is pulling out the code from quantum.c, and moving it to it's own process_record files, like most of the other features. And merging in another, similar feature.

So, all in all, I think this is a cleaner implementation, and will save space for those that don't want to use it.

@drashna
Copy link
Member

drashna commented Feb 28, 2019

This PR and #3885 aim to do the same thing. Though, this PR includes some additional functionality.

@XScorpion2
Copy link
Contributor Author

@drashna #3885 last updated 4 months ago, I'll take a look at the source and see if there is anything that needs to be pulled over from that pr into this one to make them identical in the regards of splitting out space cadet

@XScorpion2
Copy link
Contributor Author

XScorpion2 commented Feb 28, 2019

Issue #3701 is fixed by this pr. Root cause was due to using a single timer for both left and right shifts instead of separate timers. Otherwise these pr's are identical in functionality outside of this one supporting ctrl + alt. Though the comment in #3885 about define hell and the logic on how this and shift enter were implemented do make me wonder if this feature hasn't been slightly over complicated. @drashna got time in discord to chat?

@XScorpion2
Copy link
Contributor Author

closing pr, #5277 is cleaner

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.

4 participants