Feature: ibmpc_usb lock state indicators on converter itself #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some keys were mapping incorrectly (End <-> Apps, LGUI <-> RCtrl etc.) due to inconsistent cs2_e0code() compared to the rest of the files. Now corrected and tested on 2021 Unicomp New Model M and 1990 IBM p/n 1391406.
This was causing build warnings and wasn't really needed anyway.
The PS/2 controller code on Unicomp's New Model M has a logic design flaw whereby it outputs E0 77 (effectively substituting a press of Num Lock) instead of E0 7E E0 F0 7E (substituting a press and release of Scroll Lock, which is correct for Break, given its legacy from the IBM 5150). Added this into the function that maps E0 scancodes to correct for Unicomp's mistake. Tested, and does not interfere with actually pressing Ctrl + Num Lock, which executes a Pause, as it should.
Restructured lock state LED routine to update both the keyboard and any indicators wired to the converter itself. This deliberately does not use the Quantum indicator LED tools because that interfered with the more TMK-relevant routine for translating the USB HID LED update byte to IBM format and sending it to the keyboard, and also because TMK does not have the Quantum tools, and I would rather that the TMK, QMK, and Vial versions of this converter code were reasonably similar to make it more straightforward to keep all three updated somewhat in tandem. Consequently, pin assignments for LEDs wired to the converter are in led_pins.h for the time being.
On second thoughts, it feels dumb to write a controller pin handler when one already exists in QMK as it is, so I've gone back to the original strategy and used the built-in led.h writePin instead. It's not a huge amount of differentiation between TMK and QMK versions of the converter anyway, so I doubt it will complicate matters much (it just means some of the TMK code won't be needed in the QMK/Vial version and the writePin chunk that's unique to QMK/Vial needs to not be interfered with when porting across updated TMK code in future).
My intention with splitting up ibmpc_host_set_led() into two functions was to ensure that the bit order of the USB HID LED update byte was not reconfigured to IBM order unless it was actually going to be used. However, it has been suggested that splitting the check for whether the keyboard can receive the LED byte and the subsequent sending of the LED byte into separate functions in some way implies that they are less contextually connected and inter-reliant, which could hypothetically lead to a misunderstanding and potentially the addition of code that does unwise things with these newly separate functions (e.g. you can't send the LED byte without sending the check byte immediately prior, you can't send multiple LED bytes after only ascertaining once that the keyboard is capable of receiving them, and so forth). To reduce the likelihood of this interpretation, I've removed the case- specific functions for sending the two bytes from ibmpc and called ibmpc_host_send() directly from the converter side to make it clearer that the LED byte should only be sent immediately after the check byte.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactored LED indicator routine to incorporate updating LEDs on the converter itself (LEDs wired directly to controller pins) primarily for the sake of keyboards that speak unidirectional protocols and/or have no LED indicators on them. Still safeguards sending IBM-formatted LED control byte to "XT" protocol keyboards or anything that has not yet been identified, just does it in a different place to facilitate controlling the LEDs on the converter.
TMK PR for equivalent functionality: tmk/tmk_keyboard#755