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

Lily58 Add support for RGB MATRIX #8900

Closed
wants to merge 3 commits into from

Conversation

ShadowProgr
Copy link
Contributor

@ShadowProgr ShadowProgr commented Apr 24, 2020

Add support for rgb matrix for folks using Lily58-Glow

Description

There was no g_led_config in the code to support the rgb matrix for Lily58-Glow. I created this PR so other Lily58-Glow users could have the sweet rgb matrix effects without spending time setting it up.

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

Add support for rgb matrix for folks using Lily58-Glow
@zvecr zvecr requested a review from a team April 24, 2020 15:40
@zvecr
Copy link
Member

zvecr commented Apr 24, 2020

Duplicate of #7495 and #8890, this is a lighter touch so maybe gets my vote? (though last time I checked it was still an unofficial hardware fork, so might still get in the way long term)

@ShadowProgr
Copy link
Contributor Author

Duplicate of #7495 and #8890, this is a lighter touch so maybe gets my vote? (though last time I checked it was still an unofficial hardware fork, so might still get in the way long term)

I just took a look at those 2 PRs and I think those 2 and mine are different? #7495 and #8890 seem to only differ from original lily58 by the amount of LEDs. My PR adds g_led_config so that end-users can have working RGB matrix just by defining RGB_MATRIX_ENABLE, the other 2 doesn't have that.

Also #7495 seems to have 128 LEDs (I wonder how much power does it need lol), I don't think it's the Lily58-Glow.

Regarding possible long term issues that might arise, I think it would be better to have a separate glow version as in #8890. And then I can add RGB matrix support to it instead of editing the official rev1, which currently doesn't have per key RGB

@ShadowProgr ShadowProgr reopened this Apr 25, 2020
@luckenbach luckenbach mentioned this pull request Apr 26, 2020
13 tasks
@drashna drashna requested a review from a team April 29, 2020 05:35
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

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

Does the original Lily58 PCB by @kata0510 support RGB Matrix, or is this code for a PCB based on the Lily58? Looking at the OG Lily58 PCB repo, my understanding is no.

If that's the case, I think I'd prefer seeing this added as a variant/revision of the Lily58 (e.g. lily58/glow) rather than glomming on to the codebase for the Lily58 (lily58/rev1).

@ShadowProgr
Copy link
Contributor Author

ShadowProgr commented May 8, 2020

Yes that makes sense, I will move all of this to lily58/glow. But I'll probably do so after qmk:future merges into master, since lily58 code is refactored in it.

@noroadsleft
Copy link
Member

Sounds good @ShadowProgr, I'll mark this as being on hold until then.

@drashna drashna added the awaiting_pr Relies on another PR to be merged first label May 9, 2020
@ShadowProgr
Copy link
Contributor Author

So I started working on this today and found out that since Lily58 uses split common now, it cannot have rgb matrix working (for the slave side) until #5998 is merged.

Now since lily58-glow main changes from official version is rgb per key, it's kinda pointless to implement this PR without rgb matrix working, putting this on hold again.

Am I missing anything or am I basically stuck until rgb matrix support is brought over to split common?

@AnanyaKirti
Copy link
Contributor

is it working now?

@drashna
Copy link
Member

drashna commented Aug 12, 2021

@ShadowProgr RGB Matrix works now, with split common.

Also, merge conflicts.

@ShadowProgr
Copy link
Contributor Author

Woah it's been so long I completely forgot about this.

I'll probably get it done this weekend

@drashna drashna removed the awaiting_pr Relies on another PR to be merged first label Aug 12, 2021
@drashna
Copy link
Member

drashna commented Aug 12, 2021

No worries!

@tzarc tzarc removed the on hold label Nov 5, 2021
@tzarc
Copy link
Member

tzarc commented Nov 5, 2021

Removed the tags preventing stalebot from kicking in.
If it's worth going in, I'd suggest picking it back up.

@stale
Copy link

stale bot commented Jan 3, 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
Copy link

stale bot commented Apr 17, 2022

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.

@stale stale bot closed this Apr 17, 2022
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.

6 participants