qmk_firmware
qmk_firmware copied to clipboard
Userspace: muppetjones (#1)
Description
This PR adds:
- My userspace
- Planck keymap
- Lily58 keymap
- Kyria keymap
My userspace and keymaps incorporate code from the following users:
- @splitkb (optimizations)
- @drashna (
wrapper.h
pattern) - @andrewjrae (casemodes)
- @j-inc (bongo cat)
I've added the following features in my userspace and keymaps. As I clean up and optimize these features, I'll add them into the main code base.
- Etch-a-Mouse: Encoder-based pointing device movement with acceleration.
- Dynamic, Layer-based RGB: RGB underglow that changes with your layers and respects your current RGB settings. (Need to move this from my kyria into my userspace).
This PR only includes one change outside of my user-specific code: Updates to keyboards/lily58/lib/layer_state_reader.c
. The default Lily58 keymap did not compile without these updates.
Types of Changes
- [ ] Core
- [ ] Bugfix
- [ ] New feature
- [ ] Enhancement/optimization
- [x] Keyboard (addition or update)
- [x] Keymap/layout/userspace (addition or update)
- [ ] Documentation
Issues Fixed or Closed by This PR
Checklist
- [x] My code follows the code style of this project: C, Python
- [x] 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.
- [x] I have read the CONTRIBUTING document.
- [ ] I have added tests to cover my changes.
- [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).
@drashna @zvecr Made suggested changes. Thanks for the review!
@drashna Thank you for the second review. All of the comments have been addressed.
I updated and changed some things while trying to debug a ghosting problem. I saw the behaviour on my kyria initially, but also when I flashed my lily with the original updates I posted here. The keys ghosting were different on the lily; however, it was still multiple keys at once. The issue seems to be in QMK or chibios as the WPM display was always high -- hanging around 128-255. More likely, there was a change in either that isn't compatible with the WPM interface that the included bongo cat code is using.
I was originally using 0.11.* with no issue, but I had pulled in the latest master (0.13.*) to get my fork up to speed. Rolling back the QMK version and the chibios version to the most recent 0.11 fixed both the ghosting and the WPM.
All of my changes actually line up with the second round of requested changes. I haven't retested on 0.13 yet. I'm going to get my testing and dev setup a little more stable and see if I can't track down the source of the above issue. If I find anything, I'll open a new PR.
Yeah, there is an issue with the WPM code right now. IIRC, there is a proposed fix for that.
Also, develop seems to handle the split code better.
Not going to lie, I'm not a fan of requested changes that have zero effect on the code base (and one that actually affects compile negatively). All but one (the colors in the planck) are sections from the default keymap for that board that I had left for one reason or another. Had these been changes submitted that hit the actual codebase, I certainly would have cleaned those up beforehand, and could understand the requested changes if I had missed them.
I'm all for constructive criticism, but removing inconsequential comments and "requiring changes"...it just slows down and frustrates the review process. I've been on the reviewer side in that situation several times. A word of advice: Just "comment" instead of "request changes". If it really is critical to have the extra comments removed, please say so.
@muppetjones it's preferable to get things like this done before the PR is merged and is potentially copy-pasted into other users' code, bloating the repo. It's then harder for us to clean it up afterwards, because we generally require signoff from the affected users (or at least provide a heads up) when modifying their code.
@fauxpark That's good reasoning. I appreciate the response. I'll clean it up, and I'll see about another PR to clean up the defaults that I copied from.
@fauxpark @zvecr I've made the requested changes -- thanks for the patience and the feedback.
Bump for re-review when you get a chance. Thanks! @fauxpark @zvecr
@fauxpark Fixed.
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.
Bump @fauxpark @zvecr
@drashna Thanks for the re-review and changes. I'd kinda given up on this PR =]
I'll take a look and try to get this updated and cleaned up this weekend.
@drashna Incorporated your suggestions. Thank you very much for the review!
@drashna g2g
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.
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 bug
, awaiting review
, breaking_change
, in progress
, or on hold
to prevent the issue from being re-flagged.
mis-requested review sorry drashna!
Thanks!!