fix matrix_io_delay() timing in quantum/matrix.c

[mtei]

Description

The timing of the call to matrix_io_delay() has been changed to the appropriate time. With this change, we can reduce the number of times we call matrix_io_delay() by one.

Separated matrix_io_delay() into the following two functions so that you can set the appropriate delay value for each.

• matrix_output_select_delay() - after select_row()/select_col()
  The delay is a small number of clocks specific to the MCU.
  The default implementation is as follows:

  __attribute__((weak)) void matrix_output_select_delay(void) { waitInputPinDelay(); }

  See below for more information on waitInputPinDelay().

• matrix_output_unselect_delay() - after unselect_row()/unselect_col()
  The delay is in the range of a few microseconds, depending on the capacitance and resistance of the entire keyboard circuitry.
  The default implementation is as follows:

  __attribute__((weak)) void matrix_io_delay(void) { wait_us(MATRIX_IO_DELAY); }
  __attribute__((weak)) void matrix_output_unselect_delay(void) { matrix_io_delay(); }

Added waitInputPinDelay() into quantum/quantum.h.

On AVR's GPIO and ARM's GPIO, the input signal changes need some clock time to be read into the input pins.

The waitInputPinDelay() will wait the necessary time. The wait time is set to GPIO_INPUT_PIN_DELAY in units of the clock.

If GPIO_INPUT_PIN_DELAY is not set, the following values are used.

• AVR
  The datasheets for ATmega32u4/16u4, ATmega32u2/16u2, ATmega328p, AT90usb646/1286, etc. say that a delay of one clock is required after a change in the input signal. Therefore, the default value of GPIO_INPUT_PIN_DELAY can be set to 1, but we'll set it to 2 to allow for some leeway.

• ARM-based MCUs
  For GPIOs on ARM-based MCUs, the input pins are sampled by the clock of the bus to which the GPIO is connected.
  The connected buses differ depending on the various series of MCUs.
  Also, since the CPU instruction execution clock and GPIO bus clock can vary depending on the MCU GPIO bus configuration and MCU internal register settings, the optimal delay value cannot be determined. Therefore, GPIO_INPUT_PIN_DELAY defaults to a rather large value of 0.25 microseconds.

Current matrix.c timing (click)

current matrix.c timing

over view

detail view

New matrix.c timing (click)

new matrix.c timing

over view

detail view

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.
• 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).

[noroadsleft]

Looks good to me.

[zvecr]

qmk_firmware/quantum/matrix.c

Line 96 in d4dc2a5

// Select row and wait for row selecton to stabilize

Given the above comment, i cant see why the matrix delay was moved from directly after select_row?

Also, was there a reason this was not applied to quantum/split_common/matrix.c?

[mtei]

qmk_firmware/quantum/matrix.c

Line 96 in d4dc2a5

// Select row and wait for row selecton to stabilize

Given the above comment, i cant see why the matrix delay was moved from directly after select_row?

That comment is wrong. I think a lot of people have the same misconceptions as the comment.

Also, was there a reason this was not applied to quantum/split_common/matrix.c?

Of course, I was going to change quantum/split_common/matrix.c as well.

[mtei]

FYI:

after select_row()

1 cpu clock time is needed between select_row() and readPin() .
See the following figure in the ATmega32u4 datasheet.

Figure 10-4. Synchronization when Reading a Software Assigned Pin Value

after unselect_row()

You will have to wait for the row and column signals go up to HIGH before the next select_row().
HIGH is 1.9V(=Vcc * 0.2 + 0.9) according to the ATmega32u4 datasheet.

[zvecr]

QMK runs on more than a 32u4, we would have to certain on the observed behaviour is consistent on all supported chips. Given where the delay is right now, that seems "safer" while not optimal. Not a massive fan of the risk vs reward here.

[mtei]

Look at the picture above.

There is an unstable time for the signal just after unselect_row(). In the current code, matrix_io_delay() doesn't cover that time.

This PR is intended to remedy that.

[zvecr]

There is an unstable time for the signal just after unselect_row(). In the current code, matrix_io_delay() doesn't cover that time.

In its current form, the delay before read will also handle the last unselect_row.

And as i also mention above, that image only shows one of the many chips we support. Making assumptions based off that single observed behaviour could lead to issues.

[zvecr]

Needs validation (and not just "it works on my board" sort of testing, oscilloscope level verification) on...

Various STM32:

• f103
• f303
• f4x1
• f072

More AVR:

• atmega32a
• atmega328p

[mtei]

QMK runs on more than a 32u4, we would have to certain on the observed behaviour is consistent on all supported chips.

The waveform after unselect_row() above is the result of the routine manipulation to the GPIO. That's the basic behavior of digital circuits. All AVRs and ARMs behave no differently.
The only difference is the time it takes for the signal to stand up.

If you don't believe my explanation, ask someone who knows about hardware.

[zvecr]

If you don't believe my explanation, ask someone who knows about hardware.

Its not my burden to provide defence and justification of the change, its the submitter. I'm asking for proof of the statement All AVRs and ARMs behave no differently.

If we go with your previous observation of 1 clock cycle is required after select_row, what if a longer delay is required on some supported chips? The only way we know for certain is through testing or relying on data sheets. All we have are words and some 32u4 scope catpures, which is not enough proof to justify the change is valid for all scenarios.

[mtei]

Its not my burden to provide defence and justification of the change, its the submitter. I'm asking for proof of the statement All AVRs and ARMs behave no differently.

That’s right. Okay, I will try again.

[tzarc]

If we go with your previous observation of 1 clock cycle is required after select_row, what if a longer delay is required on some supported chips? The only way we know for certain is through testing or relying on data sheets. All we have are words and some 32u4 scope catpures, which is not enough proof to justify the change is valid for all scenarios.

Specifically to this point, I know that at least three nop instructions are required on at least some models of STM32 -- there is no observable change in GPIO state (using an oscilloscope) when pulsing high/low/high, or low/high/low.

Unsure if it's anything to do with instruction pipelining or the like, but single instruction GPIO is definitely not the case for everything.

(Results in the following implementation, for example: https://github.com/tzarc/qmk_build/blob/master/tzarc-cyclone/cyclone.c#L71)

[mtei]

I downloaded one of the STM32 series datasheets and am browsing through it.

The GPIO seems to be connected to the APB2 bus, which is slower than the CPU. The APB2 bus is running on a clock that is divided by the cpu clock. Perhaps that's what's involved.

[tzarc]

Just a note, been running this implementation for several weeks without any issues.

[drashna]

Should probably retarget to develop and rebase.

[mtei]

Due to tzarc's comment above, I'm investigating the need for a very short delay after 'select_row()'.

[tzarc]

I can see that my review was re-requested -- been using this on the Djinn for several months (including newer iterations), and the code looks and works perfectly fine as far as I can see.

[nomis]

Why is there no delay after the last unselect?

When the next matrix_scan runs it could select the first row/column again before 30µs has passed.

[mtei]

The default of 30us for MATRIX_IO_DELAY is probably too long.

In this picture, 5us seems to be enough.
(It should be enough if the voltage rises to half of Vcc, in which case even 1us or 2us be enough.)

Later, when I have more time, I will measure the AVR case and the Proton-C(ARM) case.

[nomis]

Ok, but there should still be a delay after the last unselect?

Otherwise there's an assumption being made that it will take at least that amount of time for other processing before selecting again.

I have a PR here to actually get short unselect times on ChibiOS instead of 100µs: #12211

If the scan rate can be improved by decreasing the unselect delay further then the per-key debounce algorithms really need to start tracking the order of key changes.

[mtei]

In the above picture (AVR), it looks like it takes about 40us from the end of matrix_scan() until the next matrix_scan() is called. In the case of ARM, assuming a 4x clock, it would be around 10us.

If the scan interval is shorter and insufficient, you can increase GPIO_INPUT_PIN_DELAY to extend the matrix_output_select_delay() time. You can increase the GPIO_INPUT_PIN_DELAY in units of cpu clocks, so you can adjust it precisely.

[nomis]

In the above picture (AVR), it looks like it takes about 40us from the end of matrix_scan() until the next matrix_scan() is called.

Yes but that's in one specific case. If someone runs QMK on something much faster or decides to call matrix_scan() in a loop until the matrix changes it could fail to meet the minimum time.

In reality I can remove the unselect delay entirely and the keyboard still works ok because it's just not fast enough to be a problem.

[mtei]

The following functions in quantum/matrix_common.c can be overridden, so do so if necessary.

__attribute__((weak)) void matrix_output_select_delay(void) { waitInputPinDelay(); }
__attribute__((weak)) void matrix_output_unselect_delay(void) { matrix_io_delay(); }
__attribute__((weak)) void matrix_scan_kb(void) { matrix_scan_user(); }
__attribute__((weak)) void matrix_scan_user(void) {}

[nomis]

Yes I know I can override those functions, I'm stating that the default implementation shouldn't skip the last unselect delay.