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

Ideas for features and optimizations #272

Closed
eltang opened this issue Apr 22, 2016 · 37 comments
Closed

Ideas for features and optimizations #272

eltang opened this issue Apr 22, 2016 · 37 comments

Comments

@eltang
Copy link
Contributor

eltang commented Apr 22, 2016

I have some ideas for features and optimizations and I would like to discuss them here. They will require quite substantial changes and I want to make sure that they will be merged before making them.

  1. What is all this doing there? I feel like new action codes should have been defined and the code to handle them put in action.c.
  2. I want to completely overhaul the way modifiers are handled. I want to get rid of the messy weak_mods and macro_mods. The real_mods are changed every time the modifiers that are being sent to the system need to be changed. However the catch is that manual modifier state changes that are performed by the user are stored a physical_mods variable. On every press, real_mods will be set to physical_mods. In addition, the S() macro and its friends will be changed. Instead of simply applying the modifier, it sets the state of the modifier in real_mods to the inverse of the state of that modifier in physical_mods. The purpose of all these changes is to make it possible for layouts like Programmer Dvorak to be easily programmed and work seamlessly after that.
@jackhumbert
Copy link
Member

  1. That's the basis for QMK's customisability. I'm pretty happy with where it is right now (I just moved things around in there recently), but it could be moved to a new file and called somewhere in process_action instead of where it is currently.
  2. I like the idea of inverting mods instead of combining them, but I don't know that a large rewrite of the modifiers is really necessary to do that. An actual implementation of S() (that includes the inverting) rather than the action shortcut it's currently using would take care of the cases in layouts like programmer Dvorak, right?

@eltang
Copy link
Contributor Author

eltang commented Apr 22, 2016

  1. I think I'll start refactoring. A lot of keycodes (especially the Magic ones) can be squashed together. For example, Debug can just become Shift-Reset, or vice versa.
  2. My current implementation replaces the old behavior. I think it's fairly complete. I've been using it a while and it's caused me no problems. I'm not sure how I'd be able to make a separate shortcut—that'd probably take up most if not all of the few remaining available action codes. Just to be clear, this would replace Fix #156: clear weak mods on every key press #188.

@jackhumbert
Copy link
Member

  1. The squashing is kind of unnecessary - we currently have 16bit keycodes, and 32bit ones will be implemented fairly soon (see qmk_firmware/user_functions, I think). It also makes things confusing. Ideally, there would be a debug toggle, on, and off. There's a lot of space between the numbers on purpose, so similar features we add in the future can be easily inserted without moving a bunch of stuff around.
  2. Have you already implemented this then? It might be helpful for us to be able to see it. The shortcut could just be a 5bit mod that gets paired with a 8bit key/scancode, where the mods are adjusted manually, with the inversion. You could even just modify keymap_common.c:L153 in conjunction with code block to process the information (you can't process in keycode_to_action because of the lack of event info - something I would like to change as well).

@eltang
Copy link
Contributor Author

eltang commented Apr 22, 2016

  1. 32-bit codes. Those are going to solve … everything. I can help with the implementation if it's not already complete.
  2. I have already implemented this feature. In fact, I'm using it right now. Why do you hesitate to do away with the old behavior? If you're unclear on anything, I can try to clarify. I was not using git when I implemented this and the code is just sitting in some files uncommitted. I'll have to do a bit of work to get it on GitHub.

@jackhumbert
Copy link
Member

You should be able to use diff to figure out what's changed - if not, you can just download the latest code, and paste in your new files. Git will see your changes and everything. Try the Github app :)

@eltang
Copy link
Contributor Author

eltang commented Apr 22, 2016

It's way to messy to clean up right now. I made a gist of action.c, which is where the main action happens. You want to focus on lines 99–168.

@jackhumbert
Copy link
Member

Cool - you can see it compared to QMK's current action.c here.

@eltang
Copy link
Contributor Author

eltang commented Apr 22, 2016

So what do you think? Any questions? Ignore the stuff for Oneshot modifiers for now. I'll get to that later.

@jackhumbert
Copy link
Member

I can't dig too much into it right now but it looks good!

@mecanogrh
Copy link

and a lot clearer :) but still using weak_mods?

@eltang
Copy link
Contributor Author

eltang commented Apr 22, 2016

@mecanogrh I want to do away with those.

@eltang
Copy link
Contributor Author

eltang commented Apr 22, 2016

I think I've cleaned up all the changes having to do with modifiers. Should I send a pull request?

@jackhumbert
Copy link
Member

Yeah! It'll be a lot easier to review the changes there.

@eltang
Copy link
Contributor Author

eltang commented May 2, 2016

@jackhumbert We're not going to continue this thread?

@eltang
Copy link
Contributor Author

eltang commented May 16, 2016

@jackhumbert Is supporting the reversed diode orientation and keyboards without diodes at all very important to you? There are a few decisions about the code I'm having a hard time making.

@jackhumbert
Copy link
Member

Reverse diodes are important - I'm not sure what you mean by keyboards w/o them.

@eltang
Copy link
Contributor Author

eltang commented May 16, 2016

So I've been thinking about changing MATRIX_ROWS and MATRIX_COLS to MATRIX_OUTPUTS and MATRIX_INPUTS in just the matrix.c files. The problem is that a lot of keyboards grandfathered from TMK still have their own matrix.c files and I would have to change a lot in order to accomplish that. You know, some keyboards don't have diodes on every key, and they ghost.

@jackhumbert
Copy link
Member

I think keeping the "rows" and "columns" terminology is more important - the "input" and "output" stuff should be limited to the implementation of the matrix scanning in matrix.c.

I'd like to go through and update each project, then contact someone with that board/controller to test it out, but that's a bit farther down the road.

@eltang
Copy link
Contributor Author

eltang commented May 16, 2016

Yes, the terms "input" and "output" would only be seen in matrix.c and keyboard.c. The old terms would be retained everywhere else. Does that sound good?

Should I go ahead and change those files without testing them?

Do you want to support keyboards that ghost? It looks like some of the grandfathered keyboards do. I've made some changes that might cause them to short.

@jackhumbert
Copy link
Member

I would prefer to do this sort of modification myself, sorry.

@eltang
Copy link
Contributor Author

eltang commented May 16, 2016

Which one are you talking about? I can try to explain, if needed.

@eltang
Copy link
Contributor Author

eltang commented May 18, 2016

@jackhumbert Sorry to ask another question, but my debugging skills are at their limit. Have you made any fundamental changes to the way keystrokes are processed recently? I have a branch in which my efficient version of matrix.c works, but whenever I try to bring it up to date, my keyboard either spams random keystrokes or gives no output at all. Any ideas?

@jackhumbert
Copy link
Member

I would just try to take a look at the commits and trace back. Remember that you can switch branches once you commit your changes.

@eltang
Copy link
Contributor Author

eltang commented May 18, 2016

So making a copy of that branch and running git merge master in it did the trick.

@ezuk
Copy link
Contributor

ezuk commented May 18, 2016

@ETLang - https://www.codeschool.com/learn/git - highly recommended.

@eltang
Copy link
Contributor Author

eltang commented May 18, 2016

@ezuk Git wasn't really the problem. My code just seems fragile for some reason.

@ezuk
Copy link
Contributor

ezuk commented May 18, 2016

being proficient in git really helps track down issues. of course, this is just a suggestion.

@eltang
Copy link
Contributor Author

eltang commented May 18, 2016

@ezuk Well, I have a commit that's causing my keyboard to not work, but it really only consists of refactoring. How can I use git to help me?

@eltang
Copy link
Contributor Author

eltang commented May 20, 2016

@jackhumbert @ezuk So it turned out that I made an insanely stupid mistake in the code.

@jackhumbert
Copy link
Member

@eltang you don't have to announce it ;)

@eltang
Copy link
Contributor Author

eltang commented May 20, 2016

@jackhumbert I guess you like surprises then? :P

@jackhumbert
Copy link
Member

@eltang I mean, you don't have to comment here saying that you made a mistake - just open the PR and we'll talk about it there. It saves us a bit of time.

@eltang
Copy link
Contributor Author

eltang commented May 20, 2016

@jackhumbert Ah. I just wanted to make sure you and @ezuk didn't keep looking for a solution to my problem in the meantime.

@jackhumbert
Copy link
Member

@eltang I don't know about Erez, but I thought you resolved it.. either way, talking about it with us able to see the resolution/fix is always preferred.

@eltang
Copy link
Contributor Author

eltang commented May 20, 2016

@jackhumbert Oops, brain fart. My code broke again after that, but for some reason I thought it was the same problem. Sorry about that.

@ezuk
Copy link
Contributor

ezuk commented May 20, 2016

TMI

On Fri, May 20, 2016, 11:53 Eric Tang notifications@github.com wrote:

@jackhumbert https://github.com/jackhumbert Oops, brain fart. My code
broke again after that, but for some reason I thought it was the same
problem.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#272 (comment)

@jackhumbert
Copy link
Member

@eltang this is what I was talking about - fewer status updates, more results :) or: don't just tell, show.

I'm gonna lock this thread for now as encouragement ;)

@qmk qmk locked and limited conversation to collaborators May 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants