Insert delay between shifted chars in send_string_with_delay #19280

dead10ck · 2022-12-09T16:50:00Z

Description

When using send_string_with_delay, if the string contains capital letters, it gets sent to send_char, which does not insert the delay between pressing the modifier keys needed to type the shifted letter. This results in key codes being sent unshifted in some situations, such as ssh connections where the keyboard is too fast for the modifier taps to register.

This commit inesrts a delay between every modifier tap when they are detected.

Types of Changes

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
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).

fauxpark

I'm not sure if this should be done. send_char is meant to send a single character (not key event) in one operation - ideally a single report, but I don't know if this is what actually happens. The obvious workaround is to simply increase TAP_CODE_DELAY in your config.h.

quantum/send_string/send_string.h

quantum/send_string/send_string.c

fauxpark · 2022-12-10T04:39:16Z

I tried to think of a way we could get the send_char() operation to use a single report by adding the modifiers to the keycode variable before giving it to tap_code() (using tap_code16() instead), but I don't think it's the best idea due to the way modifiers are encoded in the keycode. The low byte is used for the basic keycode, and the low 5 bits of the high byte for the modifiers; the top bit being the left/right flag therefore left shift and right alt can't be combined as it'd just be turned into right shift. This would probably work fine but maybe it'd be better to simply increase the size of the keycode lookup table to allow for modified keycodes. But that's something for another day...

zvecr · 2022-12-10T11:12:02Z

quantum/send_string/send_string.c

    }
+
    tap_code(keycode);


@fauxpark Would tap_code_delay be better here?

Suggested change

tap_code(keycode);

tap_code_delay(keycode, interval);

Yeah that makes a lot of sense; done.

No, that controls the time between keydown and keyup of the keycode, not the time between keyup of the previous event and keydown of the next.

Hm, I was thinking it would make sense to use a consistent delay for every key event. Even without using the same delay for tapping, the input delay is what was used between key down for the mods and the ASCII character. If we want to distinguish, then should we use TAP_CODE_DELAY between key down on mods and the ASCII character, then the input delay after key ups? This seems like it could get confusing for users trying to figure out the right timing for SEND_STRING_DELAY.

In my opinion, it makes more sense to use a consistent delay for every key event. But I'll defer to your judgement.

In my opinion, it makes more sense to use a consistent delay for every key event.

This would be my expectation too.

Then we should rename TAP_CODE_DELAY to more effectively communicate this. Right now it kind of implies that it's the delay between keydown and keyup when you call tap_code(), because it is.

Then we should rename TAP_CODE_DELAY to more effectively communicate this. Right now it kind of implies that it's the delay between keydown and keyup when you call tap_code(), because it is.

I mean I'm not too fussed about just using the tap code delay either way, so if this is a point of contention, I'm perfectly happy to just continue with previous semantics.

But with that said, I'm having trouble understanding what you're saying. In my view, TAP_CODE_DELAY is "delay between keydown and keyup when you call tap_code()", as you say. But the delay at issue here is the one in use in SEND_STRING_DELAY, and is, from my perspective, independent of tap_code(). The fact that it calls tap_code() internally is just an implementation detail. So to rename a long-standing configuration parameter because of its use as the default in an unrelated function seems puzzling to me.

But with that said, I'm having trouble understanding what you're saying.

You're suggesting we use TAP_CODE_DELAY as the default value for every delay that sendstring does, which includes the ones you're introducing, between the key events. Not just in the tap_code(). To me this muddies the meaning of the define's name, so if we want this value to be applied to more than just tap_code(), it should probably be renamed to be more generic. I'm not saying that should be done in this PR, just noting a potential point of confusion for users.

dead10ck · 2022-12-10T18:57:01Z

Another thought occurred to me: would it make sense to make the default wait time for SEND_STRING be TAP_CODE_DELAY, rather than 0? And if the user wanted instant, they could explicitly call SEND_STRING_DELAY(str, 0)?

Edit: in fact, I think we may have to if we want to use the same interval as the input for the ASCII character's tap delay. If we don't default to TAP_CODE_DELAY, this will have the effect of making SEND_STRING suddenly have a delay of 0 between every character, which may end up breaking a lot of people's macros.

quantum/send_string/send_string.c

quantum/send_string/send_string.h

dead10ck · 2022-12-12T03:04:57Z

Linking: .build/aeboards_ext65_rev3_via.elf                                                         [ERRORS]
 |
 | /avr/include/util/delay.h: In function 'tap_code':
 | /avr/include/util/delay.h:187:2: error: __builtin_avr_delay_cycles expects a compile time integer constant
 |   __builtin_avr_delay_cycles(__ticks_dc);
 |   ^
 | lto-wrapper: fatal error: avr-gcc returned 1 exit status
 | compilation terminated.
 | /bin/../lib/gcc/avr/8.3.0/../../../../avr/bin/ld: error: lto-wrapper failed
 | collect2: error: ld returned 1 exit status
 |
gmake[1]: *** [builddefs/common_rules.mk:267: .build/aeboards_ext65_rev3_via.elf] Error 1

Wait, I'm confused, I thought wait_ms no longer needed a constant argument? Why does this error get thrown from tap_code() but not in all the other places it was changed?

dead10ck · 2022-12-14T17:41:03Z

Curiously, rebased on top of the latest development, now the QMK CI Build fails with

platforms/chibios/drivers/i2c_master.c: In function 'i2c_stop':
./keyboards/dztech/tofu/jr/v1/config.h:20:20: error: 'I2CD2' undeclared (first use in this function); did you mean 'I2CD1'?
 #define I2C_DRIVER I2CD2
                    ^~~~~
platforms/chibios/drivers/i2c_master.c:216:14: note: in expansion of macro 'I2C_DRIVER'
     i2cStop(&I2C_DRIVER);
              ^~~~~~~~~~
 [ERRORS]
 |
 |
 |
gmake[1]: *** [builddefs/common_rules.mk:360: .build/obj_dztech_tofu_jr_v1_via/i2c_master.o] Error 1

which doesn't look like it has anything to do with my change.

Is there anything else blocking this PR? Are we just waiting for another review?

dead10ck · 2023-01-13T15:52:13Z

Bumping this again. Are there any further changes anyone would like to see?

dead10ck · 2023-03-03T01:28:09Z

Friendly ping again

quantum/action.c

github-actions · 2023-04-25T01:55:48Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

dead10ck · 2023-04-25T14:19:20Z

Is anything else needed for this PR?

github-actions · 2023-06-20T01:58:26Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

dead10ck · 2023-06-20T14:24:43Z

Still waiting on feedback about this PR; it's not stale as far as I know.

github-actions · 2023-08-26T01:46:52Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

dead10ck · 2023-08-26T02:09:10Z

Still not stale.

When using `send_string_with_delay`, if the string contains capital letters, it gets sent to `send_char`, which does not insert the delay between pressing the modifier keys needed to type the shifted letter. This results in key codes being sent unshifted in some situations, such as ssh connections where the keyboard is too fast for the modifier taps to register. This commit inesrts a delay between every modifier tap when they are detected.

dead10ck · 2023-09-18T18:12:29Z

There are some CI failures about the resulting binary size being too large. It seems unlikely to me that my change is related at all.

dead10ck · 2024-01-15T21:44:55Z

Anything I can do for this PR?

This was done for ARM in qmk#19280, but there is another copy of these functions that needed to be fixed too.

github-actions bot added the core label Dec 9, 2022

fauxpark reviewed Dec 9, 2022

View reviewed changes

quantum/send_string/send_string.h Outdated Show resolved Hide resolved

dead10ck force-pushed the insert-delay-shifted branch from 8001771 to 526658f Compare December 10, 2022 02:55

fauxpark requested changes Dec 10, 2022

View reviewed changes

quantum/send_string/send_string.c Show resolved Hide resolved

dead10ck force-pushed the insert-delay-shifted branch from 526658f to c3f1e93 Compare December 10, 2022 04:10

fauxpark requested changes Dec 10, 2022

View reviewed changes

quantum/send_string/send_string.c Outdated Show resolved Hide resolved

dead10ck force-pushed the insert-delay-shifted branch from c3f1e93 to 81b1fd9 Compare December 10, 2022 04:16

fauxpark approved these changes Dec 10, 2022

View reviewed changes

fauxpark requested a review from a team December 10, 2022 04:40

zvecr reviewed Dec 10, 2022

View reviewed changes

dead10ck force-pushed the insert-delay-shifted branch from 81b1fd9 to 0957c73 Compare December 10, 2022 18:51

dead10ck force-pushed the insert-delay-shifted branch 2 times, most recently from d7efd44 to 0f3e238 Compare December 10, 2022 20:18

fauxpark requested changes Dec 11, 2022

View reviewed changes

dead10ck force-pushed the insert-delay-shifted branch 2 times, most recently from 0957c73 to 0e1416f Compare December 11, 2022 14:36

dead10ck force-pushed the insert-delay-shifted branch 2 times, most recently from 0f3e238 to 7026db7 Compare December 14, 2022 15:58

dead10ck force-pushed the insert-delay-shifted branch from 7026db7 to 6378519 Compare January 13, 2023 15:55

drashna reviewed Mar 10, 2023

View reviewed changes

quantum/action.c Show resolved Hide resolved

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Apr 25, 2023

github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Apr 26, 2023

dead10ck force-pushed the insert-delay-shifted branch from 6378519 to 56dd8c1 Compare May 6, 2023 00:13

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jun 20, 2023

github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jun 21, 2023

drashna approved these changes Jul 11, 2023

View reviewed changes

drashna requested a review from a team July 11, 2023 22:21

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Aug 26, 2023

fauxpark added awaiting review and removed stale Issues or pull requests that have become inactive without resolution. labels Aug 26, 2023

dead10ck force-pushed the insert-delay-shifted branch from 56dd8c1 to 4f8d740 Compare September 18, 2023 14:56

drashna added the breaking_change_2024q1 label Jan 16, 2024

tzarc approved these changes Feb 16, 2024

View reviewed changes

tzarc merged commit 13434fc into qmk:develop Feb 16, 2024

dead10ck added a commit to dead10ck/qmk_firmware that referenced this pull request May 6, 2024

Insert delay between shifted chars in send_string_with_delay for AVR

de8aa4d

This was done for ARM in qmk#19280, but there is another copy of these functions that needed to be fixed too.

dead10ck mentioned this pull request May 6, 2024

Insert delay between shifted chars in send_string_with_delay for AVR #23673

Merged

14 tasks

nuess0r pushed a commit to nuess0r/qmk_firmware that referenced this pull request Sep 8, 2024

Insert delay between shifted chars in send_string_with_delay (qmk#19280)

7919dff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert delay between shifted chars in send_string_with_delay #19280

Insert delay between shifted chars in send_string_with_delay #19280

dead10ck commented Dec 9, 2022 •

edited

Loading

fauxpark left a comment •

edited

Loading

fauxpark commented Dec 10, 2022

zvecr Dec 10, 2022

dead10ck Dec 10, 2022

fauxpark Dec 11, 2022

dead10ck Dec 11, 2022 •

edited

Loading

zvecr Dec 12, 2022

fauxpark Dec 12, 2022

dead10ck Dec 12, 2022

fauxpark Dec 12, 2022

dead10ck commented Dec 10, 2022 •

edited

Loading

dead10ck commented Dec 12, 2022

dead10ck commented Dec 14, 2022

dead10ck commented Jan 13, 2023

dead10ck commented Mar 3, 2023

github-actions bot commented Apr 25, 2023

dead10ck commented Apr 25, 2023

github-actions bot commented Jun 20, 2023

dead10ck commented Jun 20, 2023

github-actions bot commented Aug 26, 2023

dead10ck commented Aug 26, 2023

dead10ck commented Sep 18, 2023

dead10ck commented Jan 15, 2024

Insert delay between shifted chars in send_string_with_delay #19280

Insert delay between shifted chars in send_string_with_delay #19280

Conversation

dead10ck commented Dec 9, 2022 • edited Loading

Description

Types of Changes

Checklist

fauxpark left a comment • edited Loading

Choose a reason for hiding this comment

fauxpark commented Dec 10, 2022

zvecr Dec 10, 2022

Choose a reason for hiding this comment

dead10ck Dec 10, 2022

Choose a reason for hiding this comment

fauxpark Dec 11, 2022

Choose a reason for hiding this comment

dead10ck Dec 11, 2022 • edited Loading

Choose a reason for hiding this comment

zvecr Dec 12, 2022

Choose a reason for hiding this comment

fauxpark Dec 12, 2022

Choose a reason for hiding this comment

dead10ck Dec 12, 2022

Choose a reason for hiding this comment

fauxpark Dec 12, 2022

Choose a reason for hiding this comment

dead10ck commented Dec 10, 2022 • edited Loading

dead10ck commented Dec 12, 2022

dead10ck commented Dec 14, 2022

dead10ck commented Jan 13, 2023

dead10ck commented Mar 3, 2023

github-actions bot commented Apr 25, 2023

dead10ck commented Apr 25, 2023

github-actions bot commented Jun 20, 2023

dead10ck commented Jun 20, 2023

github-actions bot commented Aug 26, 2023

dead10ck commented Aug 26, 2023

dead10ck commented Sep 18, 2023

dead10ck commented Jan 15, 2024

dead10ck commented Dec 9, 2022 •

edited

Loading

fauxpark left a comment •

edited

Loading

dead10ck Dec 11, 2022 •

edited

Loading

dead10ck commented Dec 10, 2022 •

edited

Loading