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

Port VIA Support over to core feature #4910

Closed
wants to merge 6 commits into from

Conversation

drashna
Copy link
Member

@drashna drashna commented Jan 22, 2019

Description

This Pull Request decouples the VIA support config from the Wilba Tech and Zeal keyboards, and moves it into the quantum code, as a core feature.

This introduces a VIA_SUPPORT_ENABLE makefile setting that enables both RAW HID, and Dynamic Keymap support, to facilitate the ability to easy enable support.

Additionally, this converts the kb functions (matrix_int, and process_record) into core functions, so they no longer replace existing functions. They have been moved to via_init and process_via respectively.

Also, this changes the Travis CI build scripts to compile with VIA support, and to copy those keymaps to qmk.fm

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

  • Various issues brought up

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. (https://docs.qmk.fm/#/contributing)
  • 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).

@drashna
Copy link
Member Author

drashna commented Jan 22, 2019

@Wilba6582 If this is stepping on your toes, I will apologize up front.
But if it is, feel free to steal the Travis scripts, since I think it would be a great idea to host VIA compatible firmware images on the main QMK site (so, in theory, they'll be downloadable with the QMK Toolbox)

Otherwise, this should allow a Proton C type conversion for VIA boards. It should be very seamless, actually.

Though, this needs testing, and I do have one question:
The limit on 4 layers is due to EEPROM size limits, correct? If you wanted to add more layers, you'd need to expand the EEPROM size, correct?

Also, this needs documentation, before it can be merged.

@wilba
Copy link
Contributor

wilba commented Jan 22, 2019

@drashna yes, it is stepping on my toes.

  1. The protocol is not finalized
  2. The code is not refactored. This can't be integrated into the PCBs that use RGB backlight.
  3. Sorry, I don't have time right now to explain everything else that's wrong.

Basically, VIA Configurator (and the QMK changes to support it) are a work in progess, I've already implemented the generic parts like raw HID messaging and dynamic keymaps and pushed them up into core so that they are not coupled and can be used in flexible ways on a per-keyboard basis. I've started the work to generalize things so that common code between keyboard implementations is shared rather than redundant duplication. Even I have not even finished refactoring how this works with the keyboard implementations that I wrote (Zeal60, M60-A, KOYU, M6-A, M6-B, M10-B, WT8-A, WT60-A, WT65-A, WT80-A). wt_main.c was a start on this, but not the final version. I haven't even switched my boards over to using a via keymap, and you're starting on the refactoring? Seriously?

The recent proof-of-concept of making a via keymap be the only build with VIA support enabled is how I would like to proceed with this, as it may be a nice way to deal with some keyboards having their own "init" that still needs to happen in conjunction with the "init" of dynamic keymaps.

Furthermore, no one cared about my dynamic keymaps code when it was just in Zeal60 and didn't have a pretty GUI front-end. Now that people see VIA and the original vision I had for this way of using QMK, everyone is in such a mad rush to make it work for their keyboards. @olivia and I are perhaps half way done, there's still a lot to do on the VIA Configurator side, it's not even close to being feature complete, meanwhile, I'm not done refactoring this into generic, decoupled bits that can integrate nicely into any keyboard. Certainly the way forward is not to just copy-paste my code into core just because you don't like referencing wt_main.c all the way out in keyboards land... you know why it's out there? Because it's not ready yet. If it was, I would have done a PR already.

@drashna drashna closed this Jan 22, 2019
@drashna drashna reopened this Jan 22, 2019
@drashna drashna closed this Jan 22, 2019
@drashna
Copy link
Member Author

drashna commented Jan 22, 2019

@Wilba6582

I apologize for that. It was not my intent to upset you.

In retrospect, I should have contacted you before opening the PR, first. Though, I wasn't really sure of how to contact you to do so, let alone what is the best method of doing so.

As for the dynamic keymap code, I know it's something that Jack has wanted to do for a while now. there is a branch that has all of the ifdefs removed from the keycode list, explicitly to that end. And some other stuff.

But yes, you are the one that has put most (if not all) of the work into this, thus far. However, it's absolutely understandable that nobody wanted to use it until there was actually a GUI frontend, .... because those can be very complicated. But that people are rushing towards it is a good thing.

And that rush is big part of what inspired this. I can't speak for anyone else, but ... I really do want to see the RAW HID, dynamic keymaps and VIA well supported. I think it's a fantastic thing, in general. And I would like to see as much support for it added, as possible.

So I apologize that I got overzealous in wanting to bring that support to the core code.

Though, referencing keyboard code is a very bad idea. It means that any changes to your code can break who knows how many other keyboards. And while that's not your responsibility, it brings up some of the exact same issues with the split keyboard code.

However, I do understand your feelings on this subject. And if there is anything that I can do to help, I would love to be able to do so. Even if that is to do nothing, at all.

@wilba
Copy link
Contributor

wilba commented Jan 23, 2019

I apologize for that. It was not my intent to upset you.

I'm not upset.

In retrospect, I should have contacted you before opening the PR, first. Though, I wasn't really sure of how to contact you to do so, let alone what is the best method of doing so.

??? This makes no sense. You can @Wilba6582 in an issue. You can message me on Discord. I'm in the QMK Discord server, even. My WT-xx keyboard implementations link to my website http://wilba.tech which has contact links at the bottom. If complete strangers who wants to commission me for PCBs can contact me, I am sure you could have found a way.

Though, referencing keyboard code is a very bad idea. It means that any changes to your code can break who knows how many other keyboards. And while that's not your responsibility, it brings up some of the exact same issues with the split keyboard code.

The only keyboards that reference my code in keyboards are mine or the ones that want to live on the bleeding edge and are too impatient to wait for me to refactor it. One of these is @nooges who wrote this guide: https://gist.github.com/nooges/5878b2648b6daeefb5c44d4ec16bf912

And what's up the top of that guide?

NOTE: Don't do this yet, please wait for Wilba's QMK code to be refactored prior to pushing anything to QMK, as we don't want to end up with a bunch of different forks of the code.

You seem to have ignored the last point of my last reply, it is still in keyboards because it is still code I wrote for my PCBs, to work with VIA, which @olivia wrote for my PCBs, as a GUI front end to the work that I had already done for Zeal60/Zeal65, and more specifically, as a simple, easy to use configurator for the many M60-A customers which would include many newcomers to the hobby.

There is some code in keyboards/wilba_tech that is generalized enough to be relatively easy to reference in other keyboards, and it's there because I didn't want duplicated code in my WT-xx PCBs. It was never intended for every keyboard wanting VIA support to use it this way.

@olivia and I have plans for VIA. We are happy that people like it and want to use it themselves. We also have the long term goal of supporting other PCBs.

This will involve refactoring and QMK core changes, which need to be well thought out, not rushed copy paste jobs, or multiple iterations, that would break keyboards just like you said my code would if I changed it in keyboards.

For example, what you are doing in #3113 would be a useful prerequisite to dealing with keyboard level code requiring specific initialization as well as VIA initialization.

Ideally, quantum_keycodes.h would be cleaned up a bit, so the enum integer values are fixed (i.e. remove the #ifdef blocks), and even allow VIA to support user keycodes (I have ideas about that, yet to be implemented). There is a discussion in #4805 about this very topic, and how it would be beneficial (to VIA Configurator) to make a bunch of improvements in one go, and coordinated with an update to VIA Configurator's keycode list.

I also need to refactor how the message handling works, splitting it apart, so common code in Core can handle the generic stuff like dynamic keymaps, and other messages are passed to the keyboard level so they can be handled differently per keyboard. Even then, it won't be good enough for every use case, for example, split keyboard implementations in which the master could be swapped will need to route keymap changes on the master to the slave, in case that slave becomes the master. That requires the keyboard level to intercept the keymap change messages.

The ability to override existing QMK functionality and do specific things at the keyboard level is what makes QMK great. I want to continue this paradigm with the VIA support code, not lock everything up in Core.

As for the dynamic keymap code, I know it's something that Jack has wanted to do for a while now. there is a branch that has all of the ifdefs removed from the keycode list, explicitly to that end. And some other stuff.

That's nice, but why are people all working on things like this without even mentioning it to me or @olivia? I have had absolutely no contact from the QMK contributors about VIA or the QMK code I wrote to support it, at best, some replies to comments I make in PRs about things that might affect VIA working with the latest revision of QMK.

I wrote the original implementation of dynamic keymaps and added the raw HID support years ago, pushed them into Core so that others could use them in whatever way they wanted. To be frank, QMK contributors have had their chance to develop their own system of host-configurable run-time keymap configuration. Now that my implementation is what VIA uses, it seems like QMK contributors think it's fair game to assimilate it into Core without even discussing it with me, or even considering that the protocol I wrote is incomplete, and changes to it would require matching changes to VIA, which is beyond the scope of QMK and requires discussion with @olivia and myself. Yes, this is criticism of the communication or lack thereof leading up to this point, and the associated perception we have that there's been a lack of respect.

I think it would make sense to raise an issue for conversing on the topic. I welcome ideas and feedback on how to approach this refactoring work.

@drashna
Copy link
Member Author

drashna commented Jan 23, 2019

Some of that may be projection, and grasping at straws, because this has me very stressed out, because your contributions have been awesome, if not downright amazing. And I don't want to be the reason that you pull away from QMK FIrmware.

However, I think it's good that a lot of this is being talked about, though the circumstance is far from ideal (about as far from it, as one can get). To be honest, I think that we need more discussion. Or at least somewhere where a game plan (or whatever) is documented, so that we all know what is going on, what's "in progress", what needs to happen, etc. And by "we", I mean QMK people, VIA people, etc.

If only to point people to something for why something is or isn't happening.

For instance, #3113 being very useful for VIA support. Or ... the entire issue with the Iris (such as #4893).
Like, should we refrain from merging anymore keyboard code that uses the VIA support stuff, give you a heads up about it first and wait for your OK, etc.

I also think that "we" (again, QMK and VIA people) need a place to talk about development and direction of both projects. Be it public, semi-public, or private. And not the #firmware channel on discord. Maybe a #via channel. or even a different server.

And yes, I know that I screwed up here. And I want to do whatever I can to make things right.

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.

2 participants