-
-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Keebwerk MEGA Initial commit #10777
Keebwerk MEGA Initial commit #10777
Conversation
- Add Keebwerk Mega pcb - Update wilba rgb code
Please help CI is angry:) |
Bump! |
@@ -1197,7 +1198,8 @@ void map_led_to_point( uint8_t index, Point *point ) | |||
point->x = pgm_read_byte(addr); | |||
point->y = pgm_read_byte(addr+1); | |||
|
|||
#if defined(RGB_BACKLIGHT_M6_B) || defined(RGB_BACKLIGHT_M10_C) || defined(RGB_BACKLIGHT_HS60) || defined(RGB_BACKLIGHT_NK65) || defined(RGB_BACKLIGHT_NK87) || defined(RGB_BACKLIGHT_NEBULA68) || defined(RGB_BACKLIGHT_NEBULA12) | |||
#if defined(RGB_BACKLIGHT_M6_B) || defined(RGB_BACKLIGHT_M10_C) || defined(RGB_BACKLIGHT_HS60) || defined(RGB_BACKLIGHT_NK65) || \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Wilba6582 you ok with this split?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yiancar yeah... we're long overdue for a refactor in this file, will get that done after this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yiancar yeah... we're long overdue for a refactor in this file, will get that done after this PR
It's probably best to try and converge this with core rgb functionality, at this point? Two concurrent implementations isn't helping standardisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am refactoring this code so that it's less conditionally-compiled (i.e. less per-keyboard specific code) and more of a plug-in/override behaviour, so that the specifics of the keyboard's RGB features can be put in the keyboard's code. I'm doing this as a consequence of there being more divergent use cases than what this code started with.
The Core RGB matrix code was a refactoring of this original RGB code, that ignored the per-keyboard customization that my original RGB code used and continues to use. At the time, I made the point that I would have to continue using the original RGB matrix implementation since the features that I wanted were not what QMK developers thought essential to keep. I also made the point that there may not be a way to make a one-size-fits-all RGB matrix implementation, especially with regards to performance on Atmel MCUs, which is why this RGB code was written for minimal cycles and minimal size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that it would be better to work in getting the core functionality up to snuff rather than further fragmenting the implementation further.
For instance, a lot of effort has been made to make it more efficient on AVR controllers, such converting the math to 8-bit math, to be more efficient. xulkal/xscorpion has done a lot of work in that regards, which the wilba_tech code misses out on.
But regardless of your reasons for not wanting to standardize it for ALL keyboards, I do not find it acceptable for one keyboard to include code from another keyboard/manufacturer this way. It doubles the work, since there are two differing implementations, that will diverge more and more, over time. .
IMO, it too either needs to be merged with the core code, or no other boards outside of wilba_tech ones should touch or refer to the wt_*
files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excuse me @drashna you are telling us that people cannot use open source code provided with the permission of the author in an open source project? So if I write my own copy of this code in a yiancar folder that would be ok?
There is a reason some people choose to use this code, its because the core code does not do what I personally want it to do. Predoing all the math is something that I consider beneficial, fixing the refresh rate with an interrupt as well. Some people love the choice of 2 colors in effect. Alas the wilba code has remained essentially the same for the past years, just more keyboards added to it.
Ill just finish by saying that I appreciate all contribution to this project, striking the hummer and saying that X code is not allowed without giving a sufficient alternative is not a good idea. We have been having this same conversation for at least two years. I dont think this PR is where this should be talked either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read the comment a few times now and I summarize the following points:
-
Updating core per key code.
I would be willing to do that as long as it used features that made sense. Fixed refresh rate, 2 color effects etc. -
Missing out on 8bit math.
I personally think what xulkal/xscorpion have done is amazing work! If its missing out or not in this implementation is a matter of architecture of the code. -
Using code from another keyboard.
Summary of my above answer. Code is virtually unchanged for the past years (exception when via went into core). I am sure wilba or I would be glad to maintain that one file if that is an issue. The author of that code has allowed its use and modification. He has no issue with it. Striking a virtual hammer is not nice. If that is the rule though Ill happily re-implement this in my personal file, this sound so inefficient and like trying to go around a virtual rule but alas.
It causes me immense stress that every time such a PR is made, it is followed by a huge discussion about this code. The reason this code was moved to a personal file in the first place it was so its owner would be responsible for it. Delaying the addition of a keyboard just for this to be argued seems like not the way to solve the issue, if an issue exists at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial suggestion was due to the fact that there was a mention of a refactoring that was about to take place.
There are obviously aspects to both implementations that have their positives and have their negatives -- my intent was that the resulting implementation post-refactor could draw upon both to improve the end result, to the benefit of all users.
If there's no appetite to improve the situation in this way, so be it. It's open source, after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on that for sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Folks, I wanted to weigh in with some clarifications here.
Fundamentally, QMK is about open source and sharing code. Hiding a widely used feature in one vendor's keyboard folder goes against those core principles.
QMK's position regarding cross-keyboard code sharing has been consistent from the start- we don't allow it, except on a case by case basis to facilitate development while a feature is prepared for core. We gave a rare exception to the VIA developers, with the expectation that eventually that code would move to core.
It has been more than a year and no progress has been made towards that goal. There isn't even a plan to do so as far as we know. Accordingly, we are pushing back harder and harder because this situation is not tenable in the long term. It hinders development of core for fear of breaking non-core code. What we're looking for to move forward here is a commitment to move the boards currently referencing the willba_tech
vender folder over to the core RGB matrix feature.
I understand that there are some features of this RGB code that would be desirable in core, and we would be thrilled to help get them there. It may not have been clear in the past, but we are open to changing core to add things like 2 color effects and keyboard level customizations. Not including them initially was about the time available, not a judgement of their utility. Our metaphorical door is always open, whether in an issue here or on Discord.
We're not expecting that to happen in one fell swoop. In fact, what would make us most happy are small incremental steps. Add one desired feature (like 2 color effects) to core, and I bet some of the boards currently using the willba_tech code can be switched over. As more is implemented more boards will be able to switch. We don't need to get to 0 tomorrow, but we do want to see the trendline peak and start to go down.
@yiancar, I'm sorry you feel caught in the middle here. Unfortunately this is not the first time this has been a point of contention, and I fear it won't be the last.
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Bump?:) |
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Co-authored-by: Ryan <[email protected]>
Bumpy Bump Bump... |
* Initial commit - Add Keebwerk Mega pcb - Update wilba rgb code * Update keyboards/keebwerk/mega/ansi/keymaps/default/keymap.c Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/keymaps/via/readme.md Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/rules.mk Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/keymaps/via/keymap.c Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/keymaps/default/readme.md Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/ansi.c Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/ansi.c Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/ansi.c Co-authored-by: Ryan <[email protected]> * Update keyboards/keebwerk/mega/ansi/ansi.c Co-authored-by: Ryan <[email protected]> Co-authored-by: Ryan <[email protected]>
* upstream/master: (48 commits) [Keymap] idobo:egstad (qmk#10783) Adding few Korean translated files (qmk#5895) [Keyboard] Keebwerk MEGA Initial commit (qmk#10777) Indicator LEDs as config (qmk#10816) add missing physical layout options and VIA support for Sesame (qmk#10471) [Keyboard] Fix unused variables in mschwingen modelm (qmk#10811) Add big spacebar defaults to Underscore33 (qmk#10731) Add qmk info -l to show the layouts too (qmk#10882) New command: qmk lint (qmk#10761) Updates to Talljoe's Keymaps (qmk#10115) [Keymap] bcat keymaps and userspace (qmk#10705) add dp60 indicator mode (qmk#8801) E85 backlight & LED indicator updates (qmk#10678) Add support for 4 IS31FL3731 devices (qmk#10860) [Keymap] add brandonschlack userspace and keymaps (qmk#10411) [Keymap] add ai03/polaris:mekberg (qmk#10508) CLI: Add `qmk clean` (qmk#10785) Adds support for XD84 Pro (qmk#9750) Freyr refactor (qmk#10833) KC60 refactor (qmk#10834) ...
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist