qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

Userspace: muppetjones (#1)

Open muppetjones opened this issue 3 years ago • 16 comments

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

muppetjones avatar Jul 06 '21 03:07 muppetjones

@drashna @zvecr Made suggested changes. Thanks for the review!

muppetjones avatar Jul 07 '21 14:07 muppetjones

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

muppetjones avatar Jul 11 '21 16:07 muppetjones

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.

drashna avatar Jul 11 '21 16:07 drashna

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 avatar Jul 17 '21 20:07 muppetjones

@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 avatar Jul 17 '21 20:07 fauxpark

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

muppetjones avatar Jul 23 '21 13:07 muppetjones

@fauxpark @zvecr I've made the requested changes -- thanks for the patience and the feedback.

muppetjones avatar Jul 31 '21 17:07 muppetjones

Bump for re-review when you get a chance. Thanks! @fauxpark @zvecr

muppetjones avatar Aug 15 '21 20:08 muppetjones

@fauxpark Fixed.

muppetjones avatar Aug 24 '21 21:08 muppetjones

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[bot] avatar Oct 30 '21 06:10 stale[bot]

Bump @fauxpark @zvecr

muppetjones avatar Nov 01 '21 14:11 muppetjones

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

muppetjones avatar Feb 17 '22 02:02 muppetjones

@drashna Incorporated your suggestions. Thank you very much for the review!

muppetjones avatar Mar 20 '22 17:03 muppetjones

@drashna g2g

muppetjones avatar Apr 05 '22 04:04 muppetjones

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[bot] avatar Jun 12 '22 14:06 stale[bot]

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.

github-actions[bot] avatar Jul 29 '22 02:07 github-actions[bot]

mis-requested review sorry drashna!

keyboard-magpie avatar Aug 03 '22 17:08 keyboard-magpie

Thanks!!

muppetjones avatar Aug 03 '22 17:08 muppetjones