Qmk_firmware: jj40 tap dance is not working

[danielo515]

Hello,

Probably I'm making some stupid mistake somewhere, but I'm a bit lost here. The problem is with tap dance on the keyboard jj40. My tap dance macros are working, however they only work when I hit another key. For example, if I have one tap configured to make dot on single tap and comma on double tap I also have to hit another key (any other key) to get the tap dance sent/finished. Anyone that wants to take a look, here is my repository with my changes:

https://github.com/danielo515/qmk_firmware/tree/keyboard/jj40/keyboards/jj40

Thanks in advance.

[Oscillope]

Have you tried testing using just ACTION_TAP_DANCE_DOUBLE instead of the advanced version? That could at least narrow down if it's a bug in your tap dance functions or something else.

[danielo515]

Hello @Oscillope
I'm pretty sure the problems are not with my tap dance functions. I think the problem is that the code is ignoring the tapping term (the timeout of the tap dance).
I think this because I can do the following:

1. Trigger the first part of any tap dance, including a simple one using ACTION_TAP_DANCE_DOUBLE. So if the tap dance requires double tap I just tap once
2. Wait any number of seconds. I tried from 1 to 4 seconds
3. Tap the second time
4. Tap any other key and the value of the double tap dance plus the value of the new key it's sent

In any case, I'll comment out the complex tap dances and try with just one ACTION_TAP_DANCE_DOUBLE, but I'm afraid the problem will persist.

Thanks for your help

[danielo515]

Hello @Oscillope
I just commented out All the code related to complex tap dances (including the function declarations) and I just left one basic ACTION_TAP_DANCE_DOUBLE. The behavior is exactly the same, sadly as expected.

[Oscillope]

OK, thanks for testing. I'm not super familiar with tap dance but I can try to take a look at it this weekend and see if I can figure anything out, assuming nobody who knows better takes care of it before then :)

[danielo515]

Thank you very much! I'll wait for this weekend

[danielo515]

Hello again @Oscillope and anyone with the will to help us.

I've been making some more testing to see if I can isolate the problem.
I forget to mention this, but I'm using the same repository for another small custom keyboard (hence the code is shared) and tap dance is working. The only difference is that the other keyboard has an arduino pro micro, which uses Atmega32U4 while jj40 uses Atmega32A. So the options are: A code incompatibility or a limitation of the Atmega32A.
At first I though that all the code related to timing out things may be not working, but today I made some test with One Shot Keys and timeouts are working fine, so it should be a problem in how the tap dance handles timeouts.

Maye @drashna could help us taking a look at the tap dance code, he helped me on the past.

[danielo515]

Hello @Oscillope ,
Did you manage to take a look at it ? Not sure but maybe @algernon could help us a bit, he is the author of the tap dance feature after all.

Regards

[algernon]

My first thought was timers not working, but if OneShots do time out, then that is not the case. The other thing might be TAPPING_TERM being too long. Other than that, I don't see why timeouts wouldn't work...

Except if the timers on the Atmega32A are not 16 bit... judging by the datasheet, the 32A has 2 8-bit timers and 1 16-bit one, while the 32U4 has 2 16-bit timers. That may be an issue. Comparing the code, OneShots calculate elapsed time differently: it uses TIMER_DIFF_16(timer_read(), oneshot_time) >= ONESHOT_TIMEOUT, while TapDance uses timer_elapsed, which does it a bit differently: return TIMER_DIFF_16((t & 0xFFFF), last);.

Can you check if Leader, Space Cadet Shift, or Auto-Shift work? They use the same timer_elapsed function. If they work, the bug is in TapDance. If they do not, the issue is likely in timer_elapsed.

[algernon]

(And by work, I mean if their timeout functionality works, of course.)

[danielo515]

Hello @algernon,

I was fearing something like that, I mean a hardware limitation of the Atmeta32A, but I lack the skills to understand a data sheet.
As I said, I tried all the features that I thought make use of timeouts, but that is limited to tap dance and one shot keys :smile: (leader key was also on my list)
I'll try the ones you mention and report back. I'm not sure what is Space Cadet Shift, is not the same as one shot shift?
Many thanks for your time and your help

[algernon]

Space Cadet Shift is not OneShot, but if you test with Leader, that'll do just fine.

[Oscillope]

Hi @danielo515, sorry I was unresponsive... didn't have as much time as I thought this weekend. I'll try to look at timer_elapsed soon, assuming @algernon doesn't get to it first. I actually noticed that Layer Tap (the LT macro for switching layers) doesn't work as well, so that may be related, though I also don't really know how that macro works so it could be a completely separate problem.

We can probably just make the code use an 8-bit timer instead (or maybe even chain the 2 8-bit timers if we really need the extra resolution) if that does end up being the problem.

[Oscillope]

Looking at tmk_core/avr/timer.c, it seems like we might even just be able to switch it to use Timer 1 (the 16-bit one) instead of Timer 0 (the 8-bit one), if the code is being built for an atmega32a. I can try this out soon, I just don't want to do it in the middle of the day since this is the only keyboard I have right now.

[danielo515]

Hello everyone.
I'm not sure how space cadet works, but this is what I am experiencing:

• If i hit the shift key, it sends a parenthesis
• If I hit and hold the shift key it acts as a shift modifier

So I guess it works as expected, but I don't see how it compares to double tap. This one is a tap or tap and hold relation, which was already working on other areas, for example on layer switching ( LT macro works fine for me).

I think it worth mentioning that I'm using the space cadet on a temporary layer, not sure if that can affect. I'll check leader key, seems to be more close to tap dace.

[danielo515]

Hello again @algernon
Here I am again, after testing the leader key, and I have to say that it... works, it works perfectly. In fact, the default timeout of 300ms was too short and it was very difficult to type the sequence (albeit it worked when did it really fast), so I changed it to 900 ms and also works. If I wait for a second or more, then the normal key is sent, but if I do it within the time-frame then the sequence is executed. I tried with the code provided as example on the documentation.
So the problem should be on the tap dance implementation, right ? It's a pitty, because tap-dance was one of the main features that attracted me to QMK.

Thanks @Oscillope also for taking the time to take a look at it. I can understand that people have things to do, so don't worry.

Regards

[Oscillope]

~@danielo515 Can you try Tap Dance again? I actually just tried it on my JJ40 for the first time and it seems to work perfectly (using the simple example from the QMK manual). I did have to do a clean build in order for everything to work properly, but it now does seem to work fine. To clean: make jj40:all:clean. Let me know how it goes.~

EDIT: Nevermind... I see the issue you were talking about. In my example, if I tap twice, caps lock toggles fine, but in order to trigger Esc, I have to tap once and then hit another key. Hmmmmm...

[danielo515]

Hello everyone and happy new year!
Sorry about this, but did anyone make any advance ? I want to come back to work with my brand new keyboard fully operative 😄

Regards!

[danielo515]

Hello,

On a desperate attempt to make this work I tried the following

void dance_brace_each( qk_tap_dance_state_t *state, void *user_data){
  if(state->count > 1) reset_tap_dance(state);
}

And then use that function as a for each parametter for the tap dance macro. No luck, the behavior remains the same.

Regards

[danielo515]

After some flashes the leader hey stopped working. I'm going desperate with this keyboard.

I just noticed that @ryanm101 , @krusli and @sanosis are also jj40 owners. Do you guys use tap dance at all ? Hope I don't bother you by mentioning you.

[krusli]

I've used it before on my XD60. If I have some more free time I'll look into it.

[ryanm101]

@danielo515 I've not used tapdance yet at all. I'm new to the DIY mech scene

[danielo515]

@ryanm101 if your are interested in trying it out it's a quite easy and useful function, plus you could help us determine if it works on your keyboard. Basically is assigning a function to a key based on the number of times you hit it within an specific time frame

[ryanm101]

@danielo515 Since posting i've done a bit of reading on it and yes definitely looks useful to have. I just need to figure out which keys I want it on. I'll perhaps have a play next weekend

[underpk]

hi @danielo515 you find a solution yet? I have the same problem as yours on different board.

[danielo515]

Hello @underpk
Not, not yet. I'm using a custom solution meanwhile, which is close to tap dance but a bit different.
Basically when I hit a key more than once within a timeframe, a set of instructions is executed in place of that key. For example, if I hit parenthesis then a parenthesis is sent, if I hit it twice then the parenthesis is erased an replaced with a curly brace, if I hit it a third time, then the curly brace is erased and replaced by a brace. This is not limited to replacing one character by another, you can use it also for constructing a more or less complex string by just hitting one key.
That said, it can not be considered a replacement for tap dance.
Regards

[underpk]

Seems like Autohotkey would be easier to solve this problem for me haha

[krusli]

I went ahead and had a look at the code. When I went to try tap dance, it seems to work as expected? I used:

enum {
  TD_TEST = 0
};

qk_tap_dance_action_t tap_dance_actions[] = {
  [TD_TEST] = ACTION_TAP_DANCE_DOUBLE(KC_1, KC_2)
};

and added #define TAPPING_TERM 500 to config.h and TAP_DANCE_ENABLE = yes to rules.mk

Previously the custom matrix scanning code matrix.c file was missing a call to matrix_scan_quantum(), which I added when I was porting backlighting to the JJ40 and perhaps that fixed the problem?

Edit: took out the matrix_scan_quantum() call and that replicates the original broken behaviour as mentioned by @danielo515 here. matrix_scan_quantum() also contains a call to matrix_scan_tap_dance(). From the docs:
"Our next stop is matrix_scan_tap_dance(). This handles the timeout of tap-dance keys."

timer.c already sets up the appropriate timer compare value interrupt for the 32A via a #define. I've had a look at both ways of reading the timer value and comparing it, and both seem functionally identical to me.

[danielo515]

Previously the custom matrix scanning code matrix.c file was missing a call to matrix_scan_quantum(), which I added when I was porting backlighting to the JJ40 and perhaps that fixed the problem?

I was asking gods for this to happen. I closely followed your work for restore backlighting to the JJ40, and I was thinking "It will be good if the fix to this problem ended fixing the tap dance one too". Not sure why, but I thought that backlighting effects may require a tune-up of the timer mechanisms.

I've been porting some of your advances (not all) on backlighting to my branch. I think it is time to merge your entire branch.
But first, let's look if what you said fixes the issue.

[krusli]

If anything I was pretty confident that TIMER0 was functional as the breathing effect for the RGB LEDs does rely on TIMER0 for the effects (read_timer(), etc.). My backlighting code uses TIMER1 instead to set up PWM for dimming, following the PS2AVRGB code and the breathing effect by hooking into an interrupt I set up using code which closely follows backlight.c.

I was pretty (pleasantly) surprised myself to see the keys timing out normally when trying it out - I actually spent some time poring through the code already to try to think up why it isn't being called.

[danielo515]

Hello @krusli

I'm having some problems with your branch. Since you have defined a hard version of matrix_scan_user I can't override it on my keymap.
Don't get me wrong, I don't have great interest on overriding the function, but I am very interested on the leader key feature. Such feature requires editions (or hooks) on matrix_scan_user . I would love to keep my differences from the master qmk repository on my own keymap folder.
Is there any way I can add the leader key functionality without editing jj40.c ?

Thanks in advance.

[krusli]

You're right - for a keyboard/keymap level config it should be matrix_scan_kb instead of matrix_scan_user.

In theory, replacing the entire matrix_scan_user(void) function in jj40.c as follows should let you still define your own custom code to run at each matrix scan.

// previously void matrix_scan_user(void)
void matrix_scan_kb(void) {
  // if LEDs were previously on before poweroff, turn them back on
  if (rgb_init == false && rgblight_config.enable) {
    i2c_init();
    i2c_send(0xb0, (uint8_t*)led, 3 * RGBLED_NUM);
    rgb_init = true;
  }

  rgblight_task();

  matrix_scan_user(); // if not defined in keymap.c, will call the default implementation (do nothing)
// otherwise will call your custom matrix scanning code in your own matrix_scan_user(void) definition
  /* Nothing else for now. */
}

Edit: whoops, looks like that code has been defined within matrix.c. Let me get this working then I'll get back to you.

Edit 2: it's a blank function. Remove void matrix_scan_kb(void) from matrix.c so it can be re-defined in jj40.c.

Edit 3: pushed to my repo

[danielo515]

Again, thank you very much @krusli
Are you going to make those changes on your branch too so everybody can benefit from it ?
By the way, don't you think that matrix_scan_kb should bo on matrix.c instead of on jj40.c ?

Something I noticed is that the compiled code size has increased greatly. I was forced to remove the following features to make my build fit on the jj40:

CONSOLE_ENABLE = no
COMMAND_ENABLE = no

Not a deal-breaker because I was not using it anyway, but definitely something to mention.

[danielo515]

I can confirm that the changes made by @krusli enabled the tap dance to work on the jj40 keyboard.

I'm so happy and grateful that I'm not sure how to express it. Let's try with some emojis:

😄 😄 😭 😭 🎉 🎉 🎉 🏆 🎉 🎉 🎉

[danielo515]

I can confirm that tap dance now works flawlessly. Awesome!
Thanks and thanks again