-
Notifications
You must be signed in to change notification settings - Fork 71
Firngrod/primary from same half #1393
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
base: hrm
Are you sure you want to change the base?
Firngrod/primary from same half #1393
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job overall, just a couple of questions and suggestions :-).
| uint8_t lastKeyQueuePos = PostponerQuery_PendingKeypressCount() - 1; | ||
| PostponerQuery_InfoByQueueIdx(lastKeyQueuePos, &actionPress, &actionRelease); | ||
| uint16_t actionKeyId = PostponerExtended_PendingId(lastKeyQueuePos); | ||
| uint16_t dualRoleId = Utils_KeyStateToKeyId(resolutionKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last key in the queue, or the action key keyid? You may get scenarios where there are more keys after the action key, in which case this is a major change to the logic of the entire driver, isn't it?
If you are just trying to get the action key keyid, then I would expect this to do the trick:
Edit: remove the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PostponerQuery_InfoByQueueIdx(0, &actionPress, &actionRelease); gives me the first key pressed after the secondary-role key under evaluation, not necessarily the one which triggered the evaluation. For trigger on press, that's fine, but it breaks with Trigger on release especially when we care which key triggered the resolution.
If I press Ctrl/O+Alt/S on my right half and Delete on my left, Ctrl/O, PostponerQuery_InfoByQueueIdx(0, &actionPress, &actionRelease); gives me the Alt/S key every time the Ctrl/O evaluates, never the Delete, and it doesn't work.
Maybe I should pass the action key down all the way from SecondaryRoles_ResolveState? I'm going to try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that was the resolution key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please elaborate on the above scenario.
What is the exact scenario, what do you expect to happen and why?
So far the way I see it, when you press O+S, with primaryFromTheSameHalf enabled, it should resolve as primary action for O (by definition of primaryFromTheSameHalf), and perform the O primary action. Next, you get S press applied, which triggers a new dual role resolution on S+Delete.
What am I getting wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's trigger on release and O and S are being held. I didn't make that clear. Neither O or S should trigger before timeout or a key has been released. And since Delete is released first, O should trigger secondary because it was from the other half. That is why I need information on the last postponed key, the Delete key. Realistically, I should loop over the queue to find the first released key. I think I'm going to try that to see if it makes any difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The queue lookups are sorted by press times, right?)
So the scenario is:
- press O
- press S
- press Delete
- release Delete
- we are now examining O secondary resolution caused by release of the delete key.
- your intention is to activate a secondary O+Delete role
In what real life scenario are you triggering a secondary role with another intermediate tap in between of the secondary key and the action key?
I mean, I would expect that the S tap is an evidence that you are in the middle of writing and so that the O+whatever would be an undesired O secondary role activation.
Realistically, I should loop over the queue to find the first released key.
Well, you have no guarantee that secondary evaluation takes place on every wake event, so you will indeed have to loop over the queue.
But let's not get ahead of ourselves. I still don't understand the scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My conceptual idea of trigger by release is that when an alt role key is being held, the release of ANY key pressed after it should trigger resolution, not just the next key. This allows the very important possibility of combining alt role keys in a multi-mod combo, like the above mentioned Ctrl+Alt+Del. Press O, press N, press Delete, release Delete, release O and N any order.
On release of Del, I want O to trigger Secondary for Ctrl because of that Delete release trigger, then N to trigger secondary to Alt because of that Delete trigger, then Delete with those mods, then the mods release.
What happens in the current main branch logic is that once I release the combos, I either release O or N first. If I release N first, O triggers Ctrl because of N release, then N triggers Alt because of the previous Delete release, then Delete happens and mods are released in the proper order. If I release O first, first O triggers primary because N is still held, then N triggers secondary because Delete was previously released, and then things unroll from there. This is largely mitigated by the Trigger safety margin, and generally it works as I want it to anyway, and it's all acceptable.
However, that logic will never work properly once we start caring which key caused the resolution to happen, in this case whether N or Delete caused the O resolution by releasing first. Without looking through the queue for the first released button, multi-mod combos can only be made by utilizing the timeout to trigger the first mod as secondary.
In general, taking the last key seems to work fine, but I do sometimes see what I feel are false negatives, although I cannot be sure. It may be that I press a secondary Shift, then press the one I want to Capitalize, then press the next key immediately before I release the to-be-capitalized key. I want to see if that is the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ctrl+Alt+Del
Ah, right, I didn't realize this was a multi-mod scenario.
I see now the reasoning and fully agree.
It may be that I press a secondary Shift, then press the one I want to Capitalize, then press the next key immediately before I release the to-be-capitalized key. I want to see if that is the case.
Correct. That's why you can't blindly take the last key.
With a proper implementation things still should work if you press a postponeKeys delayUntilRelease macro, then tap your secondary scenario, then tap another key or two, then release the postponeKeys macro.
So to sum up, my conclusion is that:
-
For release triggers, you need to take the first-by-time released key in the queue, that also has the corresponding press in the queue. (This should be implemented in the postponer as a new function that does all the work of finding such a key.)
-
For the press trigger, you still need to take the first-by-order pressed key in the queue, as there may be no released key in the queue at all. (I.e., according to the old algorithm.)
(Ofc., let me know if there is anything suspicious in my reasoning whatsoever.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I will get on that when I have the time. Hopefully tonight or tomorrow night.
I did try my best to keep to the repository guidelines. Let me know if anything is out of place.
I elected not to make it exclusive to advanced strategy as it could also be useful for simple strategy.