Implement I/O abort for read()/write().

[tyan0]

First, I would like to appriciate the authors of this excellent library.
I have used this library to perform keyboard inputs automatically to Hewlett Packard spectrum analyzer (HP8590E series). I noticed the issue that sometimes the library hangs or the equipment receives garbage data during transfering the data. I looked into this problem and found the issue causes when the equipment pulls the clock line down while write() is performed. I confirmed that this patch fixes the issue.

Please consider merging this pull request to the repository.

Thanks in advance.

[Harvie]

Thanks for your interrest in this project.
Can you please provide more in-depth explanation of how is your fix supposed to work? That would make it easier for me to review, because TBH i don't even remember how this thing worked in the first place, i am not original author and it's been some time since the last time i saw the code :-)

Also i think i would need some more testing done against other platforms/PCs, since i've noticed different devices have some differences in behaviour of their PS2 controller. So if one or two other users can confirm this change didn't break anything for them, that would be beneficial as wel... if not, we can just merge it and see if someone complains :-)

[tyan0]

Thanks for the reply. I tried to describe what the patch does. Any questions are welcome.

The issue

The equipment can request I/O at any time (even during the data transfer from keyboard to the equipment) except for the timing that the data transfer is almost finisshed. Let us consider following three cases.

Case 1

The equipment (host) requests I/O by pulling-down the clock line just after the program calls PS2dev::ack().

The equipment pulls-down the data line and release clock line after a while. PS2dev::ack() calls PS2dev::wrie() and write() checks the clock/data line at

ps2dev/src/ps2dev.cpp

Line 75 in a4053d5

if (digitalRead(_ps2clk) == LOW) {

and

ps2dev/src/ps2dev.cpp

Line 79 in a4053d5

if (digitalRead(_ps2data) == LOW) {

, then returns error. PS2dev::ack() retries write(), however, it will continue to fail because the equiipment pulls-down data line untile the keyboard resposes by pulling-down clock line.

This causes infinite loop (hang up) at

ps2dev/src/ps2dev.cpp

Line 236 in a4053d5

while(write(0xFA));

. The simillar happens also at

ps2dev/src/ps2dev.cpp

Line 236 in a4053d5

while(write(0xFA));

.

Case 2

The equipment requests I/O by pulling-down the clock line after data transfer begins at

ps2dev/src/ps2dev.cpp

Line 91 in a4053d5

for (i=0; i < 8; i++)

in PS2dev::write().

The equipment pulls-down the clock line or data line during transferring the data. This breaks toggling the clock line or setting the data line by the PS2dev::write() which conflicts with I/O request from the equipment. This results in garbled data at the equipment or lost of the data.

Case 3

The equipment pulls-down the clock line then the program calls PS2dev::read(), however, the equipment errornously cancels I/O request by some kind of glitch.

PS2dev::read() waits for clock==HIGH && data==LOW until TIMEOUT occurs at

ps2dev/src/ps2dev.cpp

Line 154 in a4053d5

while((digitalRead(_ps2data) != LOW) || (digitalRead(_ps2clk) != HIGH)) {

. This causes regression of the latency.

The solution

The patch does the followings.

Case 1, 2

For the case 1 and the case 2, PS2dev::write() aborts data transfer in PS2dev::raw_write() (https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L111-L115) and calls PS2dev::raw_read() to read the data from the equipment (https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L70-L72). Then the read data will be stored in read ahead buffer (PS2dev::rabuf[]) (https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L73-L81). After the I/O request is cleared, PS2dev::write() retries to call PS2dev::raw_write() (https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L86). When the application calls PS2dev::read(), PS2dev::read() first checks the read ahead buffer and return the buffer contents instead of calling PS2dev::raw_read() (https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L181-L185).
If no data is stored in the read ahead buffer, PS2dev::raw_read() is called (https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L186).

PS2dev::available() checks the read ahead buffer first, then return true if the data exists. If no data exists, PS2dev::raw_available() called.

Case 3

For the case 3, https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L200 is added in the while loop (https://github.com/tyan0/ps2dev/blob/60741315ce3a0ad598e4d7f9cafb37bcd4e4066d/src/ps2dev.cpp#L199).

[Harvie]

Oh, sorry i've kinda missed this. I'll read it.

[Harvie]

BTW i've recently merged this: https://github.com/Harvie/ps2dev/pull/27/files
can you please check if it does seem reasonable to you? (especialy when combined with the changes you've made)

[Harvie]

Is it OK if i merge both of these ?

[tyan0]

I checked #27 and found no possible conflicts. Thanks!

[Hamberthm]

Hello @tyan0 !

I was just checking your patch after @Harvie asked me to it. I see it as a good shield for this kind of interruption and It's in my opinion, very well implemented with little to none overhead for normal operation.

I don't know if this kind of behavior is to be expected in the protocol or if it's just bad implementation in the HP analyzer. I guess it can even be the cause of some of the bugs like the ones in #27, I really don't know.

I have a question. Let's suppose the host interrupts a write() event coming from us to request I/O. Then as your implementation, we store the input form the host to the buffer, and then proceed with the wirte() event.

If the request from the host is a command needing response, for example an ID request to the keyboard, wouldn't the host expect the response byte/s immediately after issuing the command? What would happen if instead of this response byte the host receives whatever we were trying to send in the first place, and then the actual response (after the read())?

For example, if I smash keys and spam the host with key writes during system boot up, I can imagine the ID request from the host will fail and this can cause the system to ignore the keyboard after boot.

But, I really don't know if this is to be expected. Maybe we should abort the write() event after detecting the I/O request to prevent wrongly responding to a command from the host?

[tyan0]

Thanks for the review. Aborting I/O is required as described in http://www-ug.eecg.utoronto.ca/desl/nios_devices_SoC/datasheets/PS2%20Protocol.htm. Please search "abort" in the document. However, it seems that the retransmission should be chunk by chunk.

I have a question. Let's suppose the host interrupts a write() event coming from us to request I/O. Then as your implementation, we store the input form the host to the buffer, and then proceed with the wirte() event.
If the request from the host is a command needing response, for example an ID request to the keyboard, wouldn't the host expect the response byte/s immediately after issuing the command? What would happen if instead of this response byte the host receives whatever we were trying to send in the first place, and then the actual response (after the read())?

Perhaps this is not right behaviour. In this case, the host requests aborting ID request itself, therefore, keyboard should not retransmit ID response. Contrary, if the host requests to abort transmission of keypress, the keyboard should retransmit it from the beginning of the chunk.

I'll consider how to implement this correctly.

[tyan0]

I have revised the patch. What do you think of this version?

BTW, I found another bug. 9daab49 fixes the response for LED control. I confirmed the behaviour of real keyboard. Response is 0xFA rather than 0xAF as follows

[Harvie]

Response is 0xFA rather than 0xAF as follows

Nice catch! Thanks.
This seems correct, because 0xFA is generic acknowledge.
At least according to https://techdocs.altium.com/display/FPGA/PS2+Commands

For example, if I smash keys and spam the host with key writes during system boot up, I can imagine the ID request from the host will fail and this can cause the system to ignore the keyboard after boot.

@Hamberthm does the new version make more sense to you in this regard?

[Harvie]

@Hamberthm unless you have something else to say about this, i will merge. ok?

[Hamberthm]

Hi @Harvie I'm really sorry I didn't have time yet to look at the new fix more deeply, but it surely looks like @tyan0 knows what he's doing, so go ahead. I won't forget about this (it's living on my inbox until I review it) so be assured I'll look at it and merge it into my project and come back to confirm or reopen if necessary.

[Harvie]

Thanks