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 features from QMK #336

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

Adding features from QMK #336

eltang opened this issue Apr 21, 2016 · 22 comments

Comments

@eltang
Copy link

eltang commented Apr 21, 2016

Here is a list of features from QMK that I think would be worthwhile to incorporate into TMK. I'll keep this list updated as I see new features being developed.

  1. Ability to define key matrix with pin names
  2. Magic keycodes
  3. WS2812 RGB light effects
  4. Scripts for setting up build environment
  5. A cache that prevents keys from getting stuck when switching layers

If anyone has any questions about these features, I can try to answer them.

@tmk
Copy link
Owner

tmk commented Apr 21, 2016

Yeah, I think this is most wanted and I'll like to make it as default.

@eltang
Copy link
Author

eltang commented Apr 21, 2016

I pushed for the feature to become default in QMK, but for some reason it was not made that way.

As you can see, I actually wrote the final encoding for that pull request. I am quite proud of it as it saves a lot of memory space.

I am trying to clean up the code right now, but I have a question that you might be able to answer. layer_switch_get_action is called not only by process_action but also process_tapping and is_tap_key. I don't quite understand the tapping code yet, so I am not sure if it is appropriate to use the cache in the latter two cases.

I'll send a pull request each to you and @vifon when I finish.

@tmk
Copy link
Owner

tmk commented Apr 21, 2016

@yashikno is not related to me and this project, but github displays his avtar on my old commits. Becuase I've used 'noname@nowhere' as email address when making commits before, but then he registered @yashikno on github.com with 'noname@nowhere'. Not his fault, maybe github's.

Yeah, tapping code is mess and I also don't want to thouch :)
I'll look into those cases later and cherry-pick from QMK.

@eltang
Copy link
Author

eltang commented Apr 21, 2016

Okay. Do you want me to send you the code I have so far (to a new branch)?

@eltang
Copy link
Author

eltang commented Apr 21, 2016

Also, do you have plans to clean up the tapping code? I have tried and failed a few times to understand it.

@tmk
Copy link
Owner

tmk commented Apr 21, 2016

Yes, I like to refactor it, the current code is result of heuristic tunes to my typing and set of ad hoc fixes. It is very difficut to understand it even for me.

I remember 'touch cursor' tool has very clean code and works well. I would refer to its code If I refactor tapping.
http://martin-stone.github.io/touchcursor/
https://github.com/martin-stone/touchcursor

@eltang
Copy link
Author

eltang commented Apr 21, 2016

Awesome. Maybe we should delay the integration of the cache until after the tapping code has been refactored. We should have a clearer idea of where the cache should be used then.

@eltang
Copy link
Author

eltang commented Apr 21, 2016

Could you please point me to the relevant part of the code? I can't seem to find it.

@eltang
Copy link
Author

eltang commented Apr 26, 2016

Do you have a timeline for implementing this feature?

@tmk
Copy link
Owner

tmk commented Apr 27, 2016

I have no definite plan about jackhumbert#182 so far. But I'll try if I have time enough.

@tmk
Copy link
Owner

tmk commented Apr 27, 2016

Or you mean refactor of tapping code? hmm, I don't know, but it won't basically as far as tapping works for me.

@eltang
Copy link
Author

eltang commented Apr 27, 2016

Those two changes might have to be done simultaneously, don't you think? Look back at our previous discussion.

@tmk
Copy link
Owner

tmk commented Apr 27, 2016

If needed I'll try to rewrite. I did't look the patch closer but I thought tapping has no relation. I missed something?

@eltang
Copy link
Author

eltang commented Apr 27, 2016

The tapping code calls layer_switch_get_action in multiple places and the cache may or may not be needed in those instances.

@tmk
Copy link
Owner

tmk commented Apr 27, 2016

could you give me a link to the source code?

@tmk
Copy link
Owner

tmk commented Apr 27, 2016

ah, store_or_get_action() doesn't work with tapping? why?

@eltang
Copy link
Author

eltang commented Apr 27, 2016

I'm not actually sure whether or not it affects the tapping code. Its original developer kept the original layer_switch_get_action functions in the tapping code. I just thought it might be good to keep in mind that the tapping code calls layer_switch_get_action while implementing the cache and refactoring the tapping code.

store_or_get_action is really only different from layer_switch_get_action when the layer is changed while a key is held down. How does the current tapping code react to that situation? Or is that not relevant because the tapping code will be rewritten?

@tmk
Copy link
Owner

tmk commented Apr 27, 2016

I don't see any reason it doesn't work but I may be wrong.
As you refered QMK doesn't use store_or_get_action() in those places for some reason.

Thanks for headsup!

@eltang
Copy link
Author

eltang commented Apr 27, 2016

All right then. Best of luck to you in getting those changes made. I'm looking forward to what you come up with. vifon/qmk_firmware#2 contains some unfinished changes to the cache code that I made. You may look at them if you are interested.

@eltang
Copy link
Author

eltang commented May 7, 2016

@tmk How's this coming along? I just had another idea for a great feature that resulted from a discussion over at the QMK repo. I think that when an ACTION_MACRO_TAP macro times out, the macro should be called a final time with a record->tap.end flag set as true. What do you think? I believe it will be very easy to implement.

@tmk tmk closed this as completed May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@tmk @eltang and others