Fixed keyboard lockup during PC cold start (when power applied)

[ole00]

based on @Hamberthm fixes for esp-32. See issue #21

Hamberthm's comments:

• On my 486 PC, ACKs were being missed by the host controller during LED set command handling. This made the process to end and the controller to ignore the keyboard after that command. This makes it stable.

• Testing in my Toshiba 205CDS with a combined port, DEVICE ID command was failing to write the ID properly. Added some loops to shield time to the controller and get them properly.

• The interface was missing some commands in one of my sytems because of waiting to send the 0xAA "BAT test success". BAT success should always be sent a couple of houndred of milliseconds after power up, regarding state.

[Harvie]

To be honest, this project do not have much activity :-D i am 100% happy to see some, but i have to admit i've kinda grown apart from understanding the code base and i am no longer much familiar with PS2 protocol, because i rarely use it nowadays. Can you please give me detailed description of the changes made in code (and describe what the possible drawbacks might be - if any)?

I am sorry for being too investigative. But i think people tend to run this library with various old platforms which might have peculiarities in their PS2, so i just wanna make sure we're not breaking stuff for someone else by fixing the compatibility with these. It should be OK as long as we're sticking to PS2 standard and common practices, but i don't have many PS2 devices to test this (except for maybe USB PS2 converter dongle, which might not be typical use case anyway). So it's kinda hard for me to review any changes.

That said i would be happy if people would help me review each others pull requests. (or at least provide some links to relevant PS2 standard documentation which explains why the changes are correct)

[ole00]

OK I understand your point. No problem at all - I'll keep those changes in my branch and add are reference to issue #21. Thanks.

[Hamberthm]

Hello @Harvie! First of all, thank you SO much for the work you did on this project. When I started my project bt2ps2 using your code as a base, I bumped into these issues. Keep in mind you have issues open for this for a while now (check https://github.com/Harvie/ps2dev/issues/21) so I strongly suggest you to apply this commit to help out those people. I'm the author of the changes @ole00 posted, and maybe a couple more that I did later on development (check the newest [esp32-ps2dev.cpp](https://github.com/Hamberthm/esp32-bt2ps2/blob/main/main/esp32-ps2dev.cpp on my project, but please ignore the functions defined on the top for Arduino compatibility, as I migrated it to VS Code). For this changes, I made testing on four systems that were having problems; two generic 486 systems, one Toshiba 205CDS Win95 laptop, and even AN ORIGINAL IBM PS/1 that didn't accept the code! I even can test on further classic systems (I have that pending still), because I'm a volunteer at a retro computer history museum in my city. Anyway, on the changes of this commit: -On keyboard_init(): The function was writing 0xAA almost instantly using a while loop on write return success, this caused some BIOSes to miss the ACK after issuing a reset command an expecting a confirmation. According to The Protocol, the keyboard sends 0xAA a couple of hundred milliseconds after power on regardless of bus state. So I did exactly that and that problem was solved. This is the expected behavior of a PS2 keyboard, the most important being the correct delay. -On keyboard_reply(): Some ACKs and ID writings were being written instantly and again, that caused problems with some slow BIOSes, for example on my 486 system that was missing the "turn leds on" ACK on command, and that made it think the keyboard was unresponsive and ignore it completely after that, when in reality the ACK was being missed due to it being written so fast. I solved some applying a while loop on ACK return success and some applying microsecond byte delays.

I think you should be safe introducing these changes, I did and for the moment I improved compatibility vastly. None of these changes broke compatibility on any of the systems I tried them in and I have many OK confirmations on the project an no complain for a BIOS reject, yet. All of this assuming @ole00 didn't add some major things but I don't see any.

Your project has major relevance on this area and many people is taking it as a base for their own (like me), so keep that in mind.

I'm here for any help I can provide!

Cheers Humberto

[Harvie]

OK I understand your point. No problem at all - I'll keep those changes in my branch and add are reference to issue #21. Thanks.

i don't think you do understand :-) i am not suggesting i am in any way opposed to merging this. i just want to understand the changes first and make sure it will not break anything else. no reason to close this :-)

[ole00]

@Harvie. I think you made a strong point that unless I'm able to test the new changes extensively on a bunch of different hardware I can't be sure whether the changes will bring regressions to other people's hardware using the existing ps2dev code. I have only one PC I can test the new changes with, and I'm not the author of these changes, so I really can't guarantee the new code will bring improvements (I believe it will - as Hamberthm explained above). My intention with this merge request was to bring the attention that: 1) the issue exists, 2) the issue has a solution that works (tested with Arduino NANO and ATtiny 84) - I was not sure whether you might be interested and if you are then great. If not, then it''s fine as well. EDIT: there is no rush merging the changes, perhaps the community could help and do the regression testing before this MR is applied? EDIT2: here is a suggestion how to make a test on a PC. 1) find a key which opens up a bios setting during boot. It is usually F2 or ESC or F10. 2) Check the timing when that key needs to be pressed after power-on in order to enter the bios settings. Let say it might be between 4 to 6 seconds. 3) use one of the ps2dev examples and adapt it to press the F2 (or the appropriate key) after 5 seconds.
4) Test the old code and the modified code on the PC whether it is able to enter the bios or not, and do it during cold start (power cable unplugged from the PC for at least 30 seconds) and warm start (power button on the PC case is pressed). On my PC the cold start locked the keyboard with the current ps2dev code.

[Harvie]

https://github.com/Hamberthm/esp32-bt2ps2

That is cool! I am always happy to see projects like these. Maybe we should add some "Featured use cases" to README with links to software using ps2dev like bt2ps2. You are alredy linking to this project anyway.

I think you made a strong point that unless I'm able to test the new changes extensively on a bunch of different hardware I can't be sure whether the changes will bring regressions to other people's hardware using the existing ps2dev code.

Actualy all i want is the "second pair of eyes".

According to The Protocol, the keyboard sends 0xAA a couple of hundred milliseconds after power on regardless of bus state. So I did exactly that and that problem was solved.

Thank you.

I think you should be safe introducing these changes, I did and for the moment I improved compatibility vastly. None of these changes broke compatibility on any of the systems I tried them in and I have many OK confirmations on the project an no complain for a BIOS reject,

Perfect. I'll just merge it and if someone has complaints, he can always open new issue.

[Harvie]

Would you be please so kind and helped me peer review this other PR? https://github.com/Harvie/ps2dev/pull/25

[Hamberthm]

Would you be please so kind and helped me peer review this other PR? #25

Seems like a shield to prevent data loss or corruption during write(), in case of the host sending I/O requests during the process. It introduces a test on write() to check if either the DATA or CLK lines are low before doing the actual write. In the event of a request, it will store whatever command the host sends in a 16-byte buffer prior to doing the actual write.
The read() function is also modified to take in account this buffer and read it in case of it containing data.

The patch seems well done and makes the code more immune against what is (in my opinion) poorly implemented protocol like in the mentioned HP analyzer.

However, I have a question and I will proceed to ask them there in #25 .

After my doubts are clear, I'll test the changes in my project and report back!