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

Userspace: muppetjones (#1) #13461

Merged
merged 25 commits into from
Aug 3, 2022

Conversation

muppetjones
Copy link

Description

This PR adds:

  • My userspace
  • Planck keymap
  • Lily58 keymap
  • Kyria keymap

My userspace and keymaps incorporate code from the following users:

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

Add and update lily58 to work with userspace
Add and update kyria keymap to work with userspace
Add and update planck keymap with userspace
Add etchamouse code and docs to userpace
Add userspace
Update mouse encoder for smoother movement.
Encoder + mouse
Added casemodes by andrewjrae
keyboards/lily58/lib/layer_state_reader.c Outdated Show resolved Hide resolved
users/muppetjones/rules.mk Outdated Show resolved Hide resolved
@zvecr zvecr requested a review from a team July 6, 2021 17:11
keyboards/lily58/keymaps/muppetjones/features/bongo_cat.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/features/bongo_cat.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/rules.mk Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/config.h Outdated Show resolved Hide resolved
users/muppetjones/config.h Outdated Show resolved Hide resolved
users/muppetjones/features/etchamouse.h Outdated Show resolved Hide resolved
users/muppetjones/features/etchamouse.h Outdated Show resolved Hide resolved

return layer_state_str;
return layer_state_str;
Copy link
Author

@muppetjones muppetjones Jul 7, 2021

Choose a reason for hiding this comment

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

Whitespace changes due to clang-format.
Rolled back the layer definitions, but kept "duo".

Copy link
Member

Choose a reason for hiding this comment

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

If anything, this should be copied to your keymap, rather than altered.

Also, snprintf should be avoid whenever possible, as it adds ~1.5k to the compiled size, even with LTO enabled.

Copy link
Author

Choose a reason for hiding this comment

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

Restored the original.

Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Joel Challis <git@zvecr.com>
@muppetjones
Copy link
Author

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

Updated parameters for etchamouse for smoother mouse movement. Updated lily keymap and userspace to actually work together.
@drashna drashna requested a review from a team July 11, 2021 16:01
@muppetjones
Copy link
Author

muppetjones commented Jul 11, 2021

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

@drashna
Copy link
Member

drashna commented Jul 11, 2021

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.

Co-authored-by: Drashna Jaelre <drashna@live.com>
keyboards/lily58/keymaps/muppetjones/keymap.c Outdated Show resolved Hide resolved
keyboards/planck/keymaps/muppetjones/config.h Outdated Show resolved Hide resolved
keyboards/planck/keymaps/muppetjones/config.h Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/rules.mk Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/rules.mk Outdated Show resolved Hide resolved
keyboards/planck/keymaps/muppetjones/keymap.c Outdated Show resolved Hide resolved
@muppetjones
Copy link
Author

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.

@fauxpark
Copy link
Member

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

@muppetjones
Copy link
Author

@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
Copy link
Author

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

@muppetjones
Copy link
Author

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

keyboards/lily58/keymaps/muppetjones/README.md Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/keymap.c Outdated Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/keymap.c Show resolved Hide resolved
keyboards/lily58/keymaps/muppetjones/keymap.c Outdated Show resolved Hide resolved
return true;
}

void dip_switch_update_user(uint8_t index, bool active) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void dip_switch_update_user(uint8_t index, bool active) {
bool dip_switch_update_user(uint8_t index, bool active) {

Copy link
Member

Choose a reason for hiding this comment

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

Still needs to be addressed

muse_mode = false;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
return true;
}

users/muppetjones/README.md Outdated Show resolved Hide resolved
users/muppetjones/config.h Outdated Show resolved Hide resolved
Co-authored-by: Drashna Jaelre <drashna@live.com>
@muppetjones
Copy link
Author

@drashna g2g

@muppetjones muppetjones requested a review from drashna April 5, 2022 04:05
@drashna drashna requested review from a team and drashna April 13, 2022 05:11
@stale
Copy link

stale bot commented Jun 12, 2022

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 Jun 13, 2022
@drashna drashna requested a review from a team June 13, 2022 06:11
@github-actions
Copy link

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 github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jul 29, 2022
@drashna drashna removed the stale Issues or pull requests that have become inactive without resolution. label Jul 30, 2022
@keyboard-magpie
Copy link
Contributor

mis-requested review sorry drashna!

@keyboard-magpie keyboard-magpie merged commit df8a538 into qmk:master Aug 3, 2022
@muppetjones
Copy link
Author

Thanks!!

imhoffman pushed a commit to imhoffman/qmk_firmware that referenced this pull request Aug 20, 2022
* Userspace: muppetjones (#1)

Add and update lily58 to work with userspace
Add and update kyria keymap to work with userspace
Add and update planck keymap with userspace
Add etchamouse code and docs to userpace
Add userspace
Update mouse encoder for smoother movement.
Encoder + mouse
Added casemodes by andrewjrae

* Rollback lily58 state reader and add missing GPL

* Apply suggestions from code review

Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Joel Challis <git@zvecr.com>

* fix lily58 keymap

* Updates to user and lily for muppetjones.

Updated parameters for etchamouse for smoother mouse movement. Updated lily keymap and userspace to actually work together.

* Update keyboards/lily58/keymaps/muppetjones/config.h

Co-authored-by: Drashna Jaelre <drashna@live.com>

* Updated keymaps and userspace

* Little more cleanup.

* Update keyboards/lily58/keymaps/muppetjones/rules.mk

Co-authored-by: Ryan <fauxpark@gmail.com>

* Rollback accidental libchibios update

* Apply suggestions from code review

Co-authored-by: Drashna Jaelre <drashna@live.com>

* Update kyria keymap

* Move kyria keymap to splitkb/kyria

* Update planck keymap

* Remove all changes to keyboards/lily58/lib/layer_state_reader.c

* Update lily58 keymap

* Recommended change

* Update keymap readme

* Update kyria keymap and userspace

* Apply suggestions from code review

Co-authored-by: Drashna Jaelre <drashna@live.com>

* Renamed users/muppetjones/README.md to lc

* Update keyboards/lily58/keymaps/muppetjones/config.h

Co-authored-by: Drashna Jaelre <drashna@live.com>

Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
* Userspace: muppetjones (#1)

Add and update lily58 to work with userspace
Add and update kyria keymap to work with userspace
Add and update planck keymap with userspace
Add etchamouse code and docs to userpace
Add userspace
Update mouse encoder for smoother movement.
Encoder + mouse
Added casemodes by andrewjrae

* Rollback lily58 state reader and add missing GPL

* Apply suggestions from code review

Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Joel Challis <git@zvecr.com>

* fix lily58 keymap

* Updates to user and lily for muppetjones.

Updated parameters for etchamouse for smoother mouse movement. Updated lily keymap and userspace to actually work together.

* Update keyboards/lily58/keymaps/muppetjones/config.h

Co-authored-by: Drashna Jaelre <drashna@live.com>

* Updated keymaps and userspace

* Little more cleanup.

* Update keyboards/lily58/keymaps/muppetjones/rules.mk

Co-authored-by: Ryan <fauxpark@gmail.com>

* Rollback accidental libchibios update

* Apply suggestions from code review

Co-authored-by: Drashna Jaelre <drashna@live.com>

* Update kyria keymap

* Move kyria keymap to splitkb/kyria

* Update planck keymap

* Remove all changes to keyboards/lily58/lib/layer_state_reader.c

* Update lily58 keymap

* Recommended change

* Update keymap readme

* Update kyria keymap and userspace

* Apply suggestions from code review

Co-authored-by: Drashna Jaelre <drashna@live.com>

* Renamed users/muppetjones/README.md to lc

* Update keyboards/lily58/keymaps/muppetjones/config.h

Co-authored-by: Drashna Jaelre <drashna@live.com>

Co-authored-by: Drashna Jaelre <drashna@live.com>
Co-authored-by: Joel Challis <git@zvecr.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants