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

Adding encoder-enabled Lily58 keymap #9382

Closed
wants to merge 3 commits into from

Conversation

filoxo
Copy link
Contributor

@filoxo filoxo commented Jun 11, 2020

There is an unofficial hardware variant of the Lily58 that supports a rotary encoder. Keyhive is one vendor that supplies them: https://keyhive.xyz/shop/lily58

Description

This change adds a keymap based on the default one that enables encoder support. I initially wanted to make it a separate rev (as recommended in #8900) but couldn't make any sense of the build errors. I'd be happy to move stuff around but I'm more interested in just making it work.

I tested this with the current master of qmk on a Lily58, with the rotary encoder on the left side.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

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

Comment on lines +11 to +14
#ifdef RGBLIGHT_ENABLE
//Following line allows macro to read current RGB settings
extern rgblight_config_t rgblight_config;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed anymore, as there are functions to check all of the rgb config settings, and to write to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never responded to this. I don't think I'll be making that change since it would require touching all the keymaps and changing how the value is read in lib/rgb_state_reader.c, and that is enough effort to warrant a separate PR.

@zvecr zvecr added the keymap label Jun 20, 2020
@stale
Copy link

stale bot commented Aug 4, 2020

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

stale bot commented Sep 19, 2020

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.

@drashna drashna requested a review from a team September 20, 2020 02:04
@stale stale bot removed the awaiting changes label Sep 20, 2020
Comment on lines +3 to +10
#ifdef PROTOCOL_LUFA
#include "lufa.h"
#include "split_util.h"
#endif
#ifdef SSD1306OLED
#include "ssd1306.h"
#endif

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
#ifdef PROTOCOL_LUFA
#include "lufa.h"
#include "split_util.h"
#endif
#ifdef SSD1306OLED
#include "ssd1306.h"
#endif

Fairly sure this block isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in the original default keymap. If it should be removed from there, then I feel like that should be batched into a separate PR instead of in this one.

Copy link
Member

Choose a reason for hiding this comment

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

It should be removed there, as well, but that would be a separate PR, ideally.

As in, for all the default keymaps that do this.

However, replicating the "bad code" should be avoided, as well.

keyboards/lily58/keymaps/encoder/keymap.c Outdated Show resolved Hide resolved
@filoxo
Copy link
Contributor Author

filoxo commented Oct 14, 2020

I'm going to go ahead and close this since I keep getting feedback for changes that aren't directly related to this keymap, and I'm frustrated with the gatekeeping needed for just a layout with additional options enabled.

@filoxo filoxo closed this Oct 14, 2020
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.

4 participants