Skip to content

Commit

Permalink
Uses @eltang's famous macro! Still not perfect though :(
Browse files Browse the repository at this point in the history
  • Loading branch information
Erez Zukerman committed May 6, 2016
1 parent bf6f3fe commit 78bd31f
Show file tree
Hide file tree
Showing 2 changed files with 1,162 additions and 1,150 deletions.
Loading

19 comments on commit 78bd31f

@ezuk
Copy link
Contributor

@ezuk ezuk commented on 78bd31f May 6, 2016

Choose a reason for hiding this comment

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

@eltang - behold! I tested your macro! It did not make the keyboard explode, which is a good thing. But it also didn't work very well, which is not as good of a thing.

I can get parens to send when I hit the Shift key -- but if I hold it for a moment or two, it doesn't do anything. Do you know why?

@mecanogrh
Copy link

@mecanogrh mecanogrh commented on 78bd31f May 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

if (record->tap.count)

is always true here, if you want to test a tap vs an hold you must compare it to an int :)

So if there are no hold handler nothing will happen on hold.

@mecanogrh
Copy link

Choose a reason for hiding this comment

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

To be a little more verbose

if (record->tap.count) > 0

tap handler

else

hold handler

@mecanogrh
Copy link

@mecanogrh mecanogrh commented on 78bd31f May 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

oh and place the tapping count reset code only in the tap handler, like so

if (record->tap.count) > 0

tap handler, do your things then only reset the tapping count

record->tap.count = 0;
else

hold handler, useless to reset the tapping count here because it is already at 0!

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@mecanogrh Why must record->tap.count be compared to zero? It is not always a positive number. It starts counting from zero.

@ezuk The press might not be registered as a tap if you hold the key too long! However, a long press should still register Shift if another key is pressed while that key is held down.

@ezuk
Copy link
Contributor

@ezuk ezuk commented on 78bd31f May 6, 2016 via email

Choose a reason for hiding this comment

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

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@ezuk I checked the tapping code, and it doesn't currently support the functionality you describe. However, doesn't this implementation already work better than the one it replaces? I forgot to mention to that the parenthesis can be made to repeat by tapping and then immediately holding the key if only a small modification to the code is made!

@ezuk
Copy link
Contributor

@ezuk ezuk commented on 78bd31f May 7, 2016

Choose a reason for hiding this comment

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

Nope, this implementation feels pretty similar to the one it replaced. And... why would I repeat the parens? Do you find yourself having to type repeated ((((( chars often?

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Well, it should be better than the one currently in the README, correct? I guess the repetition isn't as important a feature. Should I get some code up here for a key that sends a parenthesis as long as there wasn't an intervening keypress?

@ezuk
Copy link
Contributor

@ezuk ezuk commented on 78bd31f May 7, 2016

Choose a reason for hiding this comment

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

Might be a nice habit for you to get into, trying the code on a keyboard before you say if something "should" or "should not" be better. You know, stick it in a keymap, compile, flash, test it. Just an idea. ;)

If you'd like to share working, tested code that sends a paren as long as there wasn't an intervening keypress, that'd be awesome!

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I would, but I don't have a second keyboard that's able to run QMK. It's not too possible for me to run a quick test and get back to what I was doing (with the keyboard).

I can do a preliminary test, but I'll probably take a little longer than if I didn't.

@ezuk
Copy link
Contributor

@ezuk ezuk commented on 78bd31f May 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I can do a preliminary test, but I'll probably take a little longer than if I didn't.

😆 😆 This quote is gold. Yes, it would take time -- this is what it means to be a pro, to test your work. And Jack and I use our main keyboards to test stuff all the time... you don't need another one. Just assign a key, flash it, see what it does. Not hard :)

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The thing is, I wasn't using git when I (poorly) wrote the code that powers my current keymap. I tried to apply some later changes to the code and the whole thing just broke. Now, the only working copy of my keymap I have is its .hex file. I really do need to flash an entire new keymap just to run a simple test.

@ezuk
Copy link
Contributor

@ezuk ezuk commented on 78bd31f May 7, 2016

Choose a reason for hiding this comment

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

Ah, I understand -- that's for explaining @eltang. So here is what you need to do now:

  1. Commit your current keymap to git, even if it is broken. Not as a pull request to this repo, but to your own pull request.
  2. Try to compile it.
  3. Fix the errors you get.
  4. Try to compile it.
  5. Repeat steps 3 and 4 until it compiles.
  6. Flash the result on your keyboard and test it.
  7. Improve it as needed, constantly repeating steps 3-6.
  8. Commit your improvements and open a pull request with your keyboard.

Being able to flash a keyboard with firmware to test it is a basic requirement for contributing to the repo, as I am sure @jackhumbert would agree.

Please let me know once you've done this -- would be good to look at your code as well. I understand if this means you will be a little less available to help others while do this -- that's okay, as fixing your own keymap takes precedence.

Good luck!

@mecanogrh
Copy link

@mecanogrh mecanogrh commented on 78bd31f May 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@mecanogrh Why must record->tap.count be compared to zero? It is not always a positive number. It starts counting from zero.

At zero tap.count is an hold if it is strictly superior to zero it's a tap.

Then if you don't compare to a value in a conditional, you are testing if the variable is true, true is always returned when the variable is defined.

[edit]
In C, as bools are ints, indeed testing a single tap works without operators.

condition-testing statements (if, while) assume that zero is false and all other values are true.

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@ezuk Here you go. I've tested this.

Put this at the top of your keymap.

static uint8_t keypress_count;

Make the macros for your two Shift keys look like this.

case 0: {
    static uint8_t old_keypress_count;
    if (record->event.pressed) {
        old_keypress_count = keypress_count;
        register_mods(MOD_BIT(KC_LSFT));
    }
    else {
        if (keypress_count != old_keypress_count) {
            register_code(KC_9);
            unregister_code(KC_9);
        }
        unregister_mods(MOD_BIT(KC_LSFT));
    }
    break;
}

Finally, put this at the bottom of your keymap file.

bool process_action_user(keyrecord_t *record) {
    if (record->event.pressed) ++keypress_count;
    return 1;
}

Good luck.

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@ezuk I added a break statement. This macro used my last case label when I tested it, so didn't need it. Sorry about that.

@ezuk
Copy link
Contributor

@ezuk ezuk commented on 78bd31f May 18, 2016

Choose a reason for hiding this comment

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

Thank you, @ETLang! Will use this and report ASAP, might take me a few days.

@eltang
Copy link
Contributor

@eltang eltang commented on 78bd31f May 18, 2016

Choose a reason for hiding this comment

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

@ezuk I forgot to mention that you do not need to call these with ACTION_MACRO_TAP.

Please sign in to comment.