[Core] PS/2 PIO Driver for RP2040

[gamelaster]

Description

This PR contains PS/2 PIO Driver for RP2040. It supports both PS/2 Mouse Stream and Remote modes, and it should support PS/2 Keyboard too.

Please, let me know if there are any changes needed in terms of code, comments or documentation. Thanks!

Merge instructions

• [Core] Rework PS/2 driver selection #17892 needs to be merged before.
• Squash merge is recommended.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[KarlK90]

Nice! First quick pass over the code, reads nicely 👍 found one soundness issue and we can re-use ChibiOS facilities for the bytes queue.

[KarlK90]

I think we can unify the ps2_host_recv_response function by using an ChibiOS input_queue_t which is filled from the ISR on rx fifo non-empty interrupt e.g.

1. Both remote and buffered mode enable the rx fifo not empty interrupt by default and do not disable it, so an IRQ is fired every time there is data available. (I'm not sure if the IRQ has to be asserted to not immediately fire again in the ISR handler)
2. On a rx fifo not empty interrupt the input queue is filled from the ISR handler via iqPutI() as long as the rx fifo is not empty.
3. The data reception part of ps2_host_recv_response is implemented by reading a single byte from the input queue by using iqGetTimeout() which handles the case that no data has arrived in time for us.

[sigprof]

Some optimizations in the PIO code may be possible (but I did not try them on any real hardware), and maybe it would be possible to keep the proper open drain behavior of PS/2 pins instead of actively driving them high in some places.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[gamelaster]

Plan is to finish some suggestions about PIO code before next merge window.
Delayed to 2022 Feb merge window.

[joh]

Hi, thank you for this excellent work! This is an eagerly awaited feature :)

My apologies for intruding on this PR, but I've made an attempt to optimize the PIO code as per the reviewer suggestions. I would be happy to share my changes, if it's of interest? It seems to work nicely with my Tex trackpoint in stream mode.

Again, I don't mean to encroach anyone's work. If you would like to retain sole ownership of this feature, that is also perfectly fine :)

[gamelaster]

Hi @joh , happy to hear that it works good for you!

About the changes, I'm open to the improvements, although, down-side is that the QMK PIO source have only pre-compiled version, stripping off all the comments or constants. I plan to release this PS/2 driver as standalone library for pico-sdk, where will be also .pio file, and the compiled version will be re-used here in QMK driver. (So when anyone want to make improvements to QMK PIO code, he can test it and do changes on library version, and then port it over here).

So, if we can add your changes to my standalone PS/2 PIO library, then we can get it also over here.
One issue is, that I didn't finished the library, and I have my PS/2 devkit packed right now, so it might
take me some time until I will polish the library and publish it.

[joh]

Great! I also did most of the development and testing first in pico-sdk, before porting it over to QMK. I will be happy to contribute the improvements to your pico-sdk library as well, whenever you're ready :)

In the meantime, I'm attaching a patch with my changes here so it can be tested and reviewed for QMK. I don't think multiple people can push to the same PR, so I think the best approach would be if you apply the patch with git am and then push it as an update to this PR.

0001-Rework-the-PS-2-PIO-driver-as-per-PR-17893.patch.txt

[joh]

Please find attached patch which ensures pio_rx_buffer is aligned for 32-bit access, as per the discussion in #qmk_firmware. Misalignment of the buffer triggers an exception when attempting to do a 32-bit write from pio_serve_interrupt() (line 105):

*frame_buffer = pio_sm_get(pio, state_machine);

0001-Ensure-pio_rx_buffer-is-aligned-for-32-bit-access.patch.txt

[KarlK90]

@gamelaster friendly ping if you want the driver to be merged before 29th of January. Which is this breaking changes cycle closing date.

[gamelaster]

@KarlK90 thanks for reminder, I will try to do it within following days.
@joh thanks for the buffer alignment patch, I applied it. (context)

About the PIO rework, I will rather do it from scratch and in my testing repo, I have some nice tests done here, which will help me to assure that all changes doesn't break anything, but I will take inspiration from your patch. Thanks!

[gamelaster]

@KarlK90 @sigprof I hope all suggestions and issues were resolved, I'm testing current driver for some time, and it seems to work flawlessly, so I hope it is suitable for merge.

[joh]

Great! For reference, here is a commented version of the PIO code:

.wrap_target
    jmp    pin, transmit               // if clock high: may transmit
    // else: clock is low, start receive
start_receive:
    set    x, 10                       // loop 11 times
receive_loop:
    wait   0 pin, 1                    // wait for clock to become low
    in     pins, 1                     // shift 1 bit from data pin into ISR
    wait   1 pin, 1                    // wait for clock to become high
    jmp    x--, receive_loop           // if x--: goto 13
    jmp    entry_point                 // done receiving
transmit:
    jmp    !osre, start_transmit       // if OSRE not empty, goto start_transmit
    jmp    entry_point                 // else: goto start
start_transmit:
    // NB: pindirs are inverted! So 1 sets pin input, 0 sets pin output
    set    pindirs, 1            [31]  // set data input, clock pin output, wait 32 cycles = 160 us
    set    pindirs, 0             [2]  // set data + clock as output, wait 2 cycles = 10 us
    set    pindirs, 2                  // set data output, clock input (release clock)
    wait   0 pin, 1                    // wait for clock to become low
    set    x, 9                        // loop 10 times
transmit_loop:
    out    pindirs, 1                  // shift 1 bit from OSR onto pindirs
    wait   1 pin, 1                    // wait for clock to become high
    wait   0 pin, 1                    // wait for clock to become low
    jmp    x--, transmit_loop          // if x--: goto transmit loop
    set    pindirs, 3                  // set data + clock as input (release pins)
    wait   0 pin, 1                    // wait for clock to become low
    wait   1 pin, 1                    // wait for clock to become high
.wrap

[KarlK90]

Expect two minor nits this is good to go in from my side! Thank you.

[KarlK90]

@gamelaster With the last review in this is good to go. Sorry for the delay and thanks for your work.

[marfrit]

Quick question - what's the right values for ps2.clock_pin and ps2.data_pin - e.g., "GP17" and "GP18" only works when the #if is commented out; 17 and 18 as values are not accepted as being not valid by the schema.

[gamelaster]

@marfrit does the driver work for you when you comment the #if ?

[marfrit]

@gamelaster I cannot yet find out; plan is to adjust my improvised adaption of the TMK ibmpc converter to use PIO instead; however, that requires soldering for which I don't have much time nowadays. Subsequently, I will need two PS2 interfaces to support the Model M13 and M4.

[marfrit]

@gamelaster please see https://github.com/marfrit/qmk_firmware/tree/pio_ps2_converter/keyboards/converter/pio_ps2_converter as an examle, info.json has

    "ps2": {
        "enabled": true,
        "driver": "vendor",
        "clock_pin": "GP20",
        "data_pin": "GP21"
    },

and
make converter/pio_ps2_converter:default
yields
6:6: error: #error PS/2 Clock pin must be followed by data pin!

What am I doing wrong?

[gamelaster]

@marfrit as it says, PS/2 Clock pin must be followed by data pin. That means that clock must be GP21 and data ping GP20

[marfrit]

@gamelaster Hi, "A must be followed by B" does mean in my understanding that A comes first, B comes second. Thank you.

[joh]

@marfrit I agree, the order of your pins looks correct: first clock pin (GP20), then data pin (GP21). What is the numerical value of GP20 and GP21 for your board?

[pca006132]

Not sure if this is the right place to ask, but I wonder if it is good to use this driver for PS/2 directly without any level shifter, as the voltage for PS/2 is 5V while RP2040 GPIO pins are technically not 5V tolerant.

I tried this with a trackpoint (PTPM754DR) using RP2040 zero pin 14, 15 and PIO1, with 4.7k pull up resistors. It will fail to initialize intermittently, there is a success rate of 40%, but as long as it initialized correctly (the mouse can move) it doesn't have any problem. I tried the exact same circuit with a blackpill stm32f401 without connecting the reset pins and everything works correctly. Do you have any clue why it can fail like this?

[gamelaster]

@pca006132 I use it like this on RP2040, and it works properly (via pull-ups). Are you sure you have RESET circuit properly done?

[pca006132]

No, I don't have reset circuit for now as I don't have capacitors at hand. I tried using GP26 to control RST by setting it to input floating, pull up using 4.7k, wait for 1000ms and then change to output low, but it doesn't work and I found that the measured voltage is just 4.0V when I use input floating, which is really weird...
I will report again after I got the capacitors and build the proper reset circuit.

[pca006132]

Oh OK, it seems that GP26 can be used for ADC and have different behavior... other pins work fine. Will try to see if I can make it work reliably.

[gamelaster]

@pca006132 I had this kind of undefined behavior when the RESET circuit was not good. If you can, I can send you the circuit which works for me.

[pca006132]

It will be great if you can send me the circuit. Thanks!

[gamelaster]

Note that PS2 is still powered by 5V.

[wolfwood]

@pca006132 it may work to use 5v to power the trackpoint, but the RP2040's gpios are not 5v tolerant, and it may lead to failure. some trackpoints can be powered by 3.3v, otherwise there's level shifters as you've said.

[gamelaster]

@wolfwood Most of times, you don't need level shifters, as most of trackpoints will treat 3V3 still as high. Thus, pull-ups data and clock line to 3V3 is sufficient.

[pca006132]

@gamelaster if 3V3 is sufficient, do we need the pull-ups or can we let the internal pull-ups do it?

[gamelaster]

@pca006132 I don't know if it is possible to have pin both as PIO pin and also have pull-up here, according to some articles on internet it seems to be possible, but we have external pull-ups. So, maybe, you need to try.

[marfrit]

About "should work with keyboards": it does - https://github.com/marfrit/qmk_firmware/tree/pio_ps2_converter (with hw not published yet), but can also be created with a Pico dev board and a level shifter.

If I wanted to have two PS/2 devices - say a mouse and a keyboard (i.e. https://sharktastica.co.uk/wiki?id=modelm4) - what would be the proper way?

[morganvenable]

Can anyone enlighten me as to why the clock pin has to follow the data pin in the pinout sequence?
By chance I have the opposite situation -- Clock is GP24, Data is GP25. Not sure why that would matter? Is that just an unimplemented improvement or is it technically impossible somehow?

[sigprof]

Implementing the “data_pin = clock_pin + 1” configuration could be possible, but would need a separate revision of the PIO program (or patching the existing PIO code on the fly):

• The set pindirs, … PIO instructions depend on the data and clock pins being next to each other and in the particular order. Here the 2-bit data values would need to be changed to swap the bits (and the set mapping would need to be changed to use the clock pin as the base).
• The wait 0 pin, 1 and wait 1 pin, 1 instructions also depend on clock_pin = data_pin+1 (data_pin needs to be the first in the in mapping, so that the in instruction could read it). In this case something like wait 0 pin, 31 would need to be used (hopefully such wrapping around would work).

Although maybe it could be possible to use the side-set feature to control one of the pindirs — that could make it possible to use completely arbitrary clock and data pins (the wait instructions could use the wait gpio variant with dynamic code rewriting to put the pin number there; unfortunately, jmp !pin does not exist). The modifications won't be as simple as they could be though, because the simplest way would require allocating 2 bits for the optional side-set data, and those bits would need to be taken from the “delay” field, therefore specifying a delay of 31 won't be possible. Although if the state of the clock pindir could be defined as a constant for all instructions, only a single side-set bit would be needed.

[morganvenable]

Thanks for the clarification! My board is going in for a respin anyway, so not a huge need here, I can just flip them in the next round, and find an easier approach with the prior generic ARM implementation for current half dozen users who are experimenting with this.

…

On Sun, Jul 23, 2023 at 3:13 PM Sergey Vlasov ***@***.***> wrote: Implementing the “data_pin = clock_pin + 1” configuration could be possible, but would need a separate revision of the PIO program (or patching the existing PIO code on the fly): - The set pindirs, … PIO instructions depend on the data and clock pins being next to each other and in the particular order. Here the 2-bit data values would need to be changed to swap the bits (and the set mapping would need to be changed to use the clock pin as the base). - The wait 0 pin, 1 and wait 1 pin, 1 instructions also depend on clock_pin = data_pin+1 (data_pin needs to be the first in the in mapping, so that the in instruction could read it). In this case something like wait 0 pin, 31 would need to be used (hopefully such wrapping around would work). Although maybe it could be possible to use the side-set feature to control one of the pindirs — that could make it possible to use completely arbitrary clock and data pins (the wait instructions could use the wait gpio variant with dynamic code rewriting to put the pin number there; unfortunately, jmp !pin does not exist). The modifications won't be as simple as they could be though, because the simplest way would require allocating 2 bits for the optional side-set data, and those bits would need to be taken from the “delay” field, therefore specifying a delay of 31 won't be possible. Although if the state of the clock pindir could be defined as a constant for all instructions, only a single side-set bit would be needed. — Reply to this email directly, view it on GitHub <#17893 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGVYM6G4XOWFE7OMYZXYDXRWOYDANCNFSM55ONPW4A> . You are receiving this because you commented.Message ID: ***@***.***>

[morganvenable]

note: If you're using this on a split and using PIO0 for split comms, you'll need to #define PS2_PIO_USE_PIO1 in config.h or the linker will fail. would it make sense to include a port choice in the basic info.json config instead?