Firngrod/improved same half logic

[firngrod]

This improves upon the same half secondary logic in that it no longer actively triggers primary from same half activations, but rather just prevents same half keys from triggering secondary role.
While there are a number of edge case changes from this, none are what I would call bugs, and I think the benefits outweigh the costs.
Benefits:

• Centralized logic: The decision is now made only in the isKeyAllowedToBeActionKey function, the rest of the code functions the same regardless of same half logic
• When using TriggerOnPress, it is now possible to use multi-mod combos. I even managed to forget I had triggerOnPress on for a brief while after testing that it worked.
• I think the naming is much more clear on this behavior than on the previous one. Let's start a discussion on it!

Arguably negative changes are mainly to do with the timing aspect. One can type out entire key sequences on the same half while a secondary role key is being held with nothing happening before the key is released, which is sort of counter-intuitive, but only actually ever happens if one is determined on testing it. Otherwise, it does ever so slightly the timing of application of some keys, but it's nothing next to just the fact of using Secondary Roles.

[firngrod]

I was unsure of whether to go with a predicate or whether to bake the logic into the search function. I like the predicate structure better as it leaves the Secondary Role business logic in secondary_role_driver.c file and the search logic in postponer.c. I do not think there is any real performance cost to doing it this way. Possibly there's a readability cost?

[kareltucek]

Well, right, what is actually relevant is that the call tree of those utils calls has become quite large, which imposes penalties on both performance and readability due to call overheads.
Also, looking at the code, my implementation of Utils_KeyStateToKeyId is hugely overengineered. That could be simplified down to a oneliner plus or minus edge-case handling - the 64 base is more or less set in stone now.

I prefer baking it into the search, unless you insist on the predicate.
Reasons:

readability cost
seems like a premature generalization / yagni
performance cost is tricky to judge because it depends on what you compare it to, but compared to an unpacked in-place computation, it is huge-ish

I guess the PostponerQuery_FindFirstReleased should issue a single call to Utils_AreKeysOnTheSameHalf with key_state_t* pointers as arguments.
If it makes sense to you, feel free to move Utils_AreKeysOnTheSameHalf logic into postponer (probably as a static function) and unpack its content there.

[firngrod]

I will do that. I'm inexperienced in microprocessor development, so I don't have a firm grasp of how much to optimize, and I tend to err or the side of generalization.

[firngrod]

How up-to-date are these comments being kept? I feel that they are quite eclipsed by the macro reference manual of which any developer is surely aware.

[kareltucek]

Just as any documentation or comments are in coding projects. When writing it, I felt the need to have a brief reference here, containing whatever I consider relevant for me as a programmer, not having to think about its exhaustiveness or other aspects of a documentation.

I am not dead set on keeping it. If you have a strong desire to just delete it, then feel free to.

[kareltucek]

I think the naming is much more clear on this behavior than on the previous one. Let's start a discussion on it!

They sound fine! :-)

[kareltucek]

Do any edge cases need to be considered?

I have thought about the following, and figured it is actually the desired behavior:

• press a
• press s
• release s
• press j
• release j
• 'a' now triggers its secondary role
• if 's' is a dual-role key, it gets evaluated as primary, yet still applied as 'a+s'
• finally, j gets applied as 'a+j'
• this sounds as the desired behavior.

[kareltucek]

If you are up to it, then please do refactor the predicate part.

Otherwise, let me know and we can merge it as is.

[kareltucek]

Just as any documentation or comments are in coding projects. When writing it, I felt the need to have a brief reference here, containing whatever I consider relevant for me as a programmer, not having to think about its exhaustiveness or other aspects of a documentation.

I am not dead set on keeping it. If you have a strong desire to just delete it, then feel free to.

[kareltucek]

Well, right, what is actually relevant is that the call tree of those utils calls has become quite large, which imposes penalties on both performance and readability due to call overheads.
Also, looking at the code, my implementation of Utils_KeyStateToKeyId is hugely overengineered. That could be simplified down to a oneliner plus or minus edge-case handling - the 64 base is more or less set in stone now.

I prefer baking it into the search, unless you insist on the predicate.
Reasons:

readability cost
seems like a premature generalization / yagni
performance cost is tricky to judge because it depends on what you compare it to, but compared to an unpacked in-place computation, it is huge-ish

I guess the PostponerQuery_FindFirstReleased should issue a single call to Utils_AreKeysOnTheSameHalf with key_state_t* pointers as arguments.
If it makes sense to you, feel free to move Utils_AreKeysOnTheSameHalf logic into postponer (probably as a static function) and unpack its content there.

[firngrod]

Do any edge cases need to be considered?

I have thought about the following, and figured it is actually the desired behavior:

* press a

* press s

* release s

* press j

* release j

* 'a' now triggers its secondary role

* if 's' is a dual-role key, it gets evaluated as primary, yet still applied as 'a+s'

* finally, j gets applied as 'a+j'

* this sounds as the desired behavior.

Yes, you are correct, the first key would go secondary and the two next keys would go primary with mod applied, even though the first one was from the same half.
I don't know what to call it. It's well defined and expected behavior, but not the goal behavior. Call it accepted behavior? It's an unintended usecase of the feature which I think is not going to cause any issues.

[kareltucek]

I think it actually is the correct and desired behavior the way it is :). Just took a few minutes to realize that there is nothing strange about it and that it definitely isn't expected to be a multimod scenario.

[firngrod]

I see it like if someone asked if you had considered that they new wooden wrist rest smelled differently from the rubber one. Well, yes it does, it's not really a feature that was aimed for, but it is to be expected, and no one was expected to start actively sniffing it.

[firngrod]

Once the other branch merges, I will rebase this one and start working on the changes.