[Bug] Trackpoint work improperly in Interrupt Version (ARM chibios) PS/2 Mouse Support

[d93921012]

Describe the Bug

In the function ps2_mouse_task() in drivers/ps2/ps2_mouse.c, it should return when pbuf_has_data() retun false. If this function continues to execute the remaining codes, the status of the mouse will be updated with wrong data.

void ps2_mouse_task(void) {
    static uint8_t buttons_prev = 0;
    extern int     tp_buttons;

    /* receives packet from mouse */
#ifdef PS2_MOUSE_USE_REMOTE_MODE
    uint8_t rcv;
    rcv = ps2_host_send(PS2_MOUSE_READ_DATA);
    if (rcv == PS2_ACK) {
        mouse_report.buttons = ps2_host_recv_response();
        mouse_report.x       = ps2_host_recv_response();
        mouse_report.y       = ps2_host_recv_response();
#    ifdef PS2_MOUSE_ENABLE_SCROLLING
        mouse_report.v = -(ps2_host_recv_response() & PS2_MOUSE_SCROLL_MASK);
#    endif
    } else {
        if (debug_mouse) print("ps2_mouse: fail to get mouse packet\n");
    }
#else
    if (pbuf_has_data()) {
        mouse_report.buttons = ps2_host_recv_response();
        mouse_report.x       = ps2_host_recv_response();
        mouse_report.y       = ps2_host_recv_response();
#    ifdef PS2_MOUSE_ENABLE_SCROLLING
        mouse_report.v       = -(ps2_host_recv_response() & PS2_MOUSE_SCROLL_MASK);
#    endif
    } else {
        if (debug_mouse) print("ps2_mouse: fail to get mouse packet\n");
        // If there is no data the function should return to avoid to update the status.
        return;
    }
#endif

To reproduce the error, pressing and holding the left button of the mouse will cause the browser to cyclically trigger mouse down and mouse up events.

[strobo5]

I'm seeing the same issue, also with a Trackpoint and ChibiOS on an RP2040 and the PIO implementation for PS/2. Nice that you already found the cause!

So what I think what we want to avoid is this (I'm basically repeating what you wrote): when the main loop calls ps2_mouse_task() for the first time after the button down event, it finds no new data in the PS/2 receive buffer, but it tries to detect a button state change anyway. mouse_report was cleared on the previous loop iteration, but buttons_prev still holds the button down state and thus a wrong button up event is detected and reported. Click and hold, move the mouse, and each PS/2 packet sets the button state down again.

The suggested change (return when there is no new mouse data) works perfectly. mouse_report never gets changed in this case so there should be no need to reach ps2_mouse_clear_report() at the end of the function. Maybe create a merge request?