Make GPIO functions for AVR atomic #9575
Conversation
On AVR MCUs the GPIO macros like writePinLow() perform read-modify-write operations on PORTx and DDRx registers, unless their argument is a compile time constant (in that case the compiler can optimize those operations into single `cbi` or `sbi` instructions). Usually this works without any problems; however, the software PWM emulation code in quantum/backlight/backlight_avr.c manipulates GPIOs from interrupt handlers, and this may cause issues which manifest in a flickering backlight on some boards (qmk#5953). The problem happens when the read-modify-write sequence performed by the matrix scanning code is interrupted by the software PWM handler, and the software PWM uses a pin from the same port as one of the matrix row pins (or column pins for a ROW2COL matrix); in this case the resumed read-modify-write code will overwrite the update performed by the software PWM emulation code. Change the implementation of all GPIO functions so that they are atomic with respect to interrupts; this is achieved by disabling interrupts around the read-modify-write operations. This fixes qmk#5953, and should not break other boards, although there will be some additional overhead for some GPIO operations (but the most common case of performing writePinLow() or writePinHigh() on pins which are known at compile time should have no additional overhead, because in this case the assembly implementation using `cbi` or `sbi` should be picked by the compiler).
|
Making those functions |
Previous change to make AVR GPIO operations atomic converted the GPIO macros into `static inline` function. Unfortunately, there is some code that uses those operations in `inline` functions, and gcc complains when an `inline` function tries to call a `static inline` function. Using a plain `inline` for AVR GPIO functions also did not work, because in this case gcc does not actually inline the code, therefore the optimization for constant arguments does not work. Apparently the only solution is to convert those functions back to macros, so that the code would always be inlined properly. The GPIO_FORCE_PRECOMPUTE() and GPIO_BARRIER() macros were added to make the code which executes with interrupts disabled as small as possible; without those macros gcc sometimes puts the slow _BV((pin) & 0xF) calculation (which contains a loop) after `cli`, which needlessly increases the interrupt latency.
Some existing code attempted to perform GPIO operations on NO_PIN. The previous implementation of the GPIO macros for AVR effectively treated those calls as noops, because `_BV((pin) & 0xF)` truncated to a byte ended up being 0; however, the atomic implementation attempted to generate `sbi` or `cbi` instructions with out-of-range operands if the `pin` argument was a compile time constant equal to NO_PIN. Add checks to avoid assembler errors and make such calls do nothing again.
|
Because using This version of the code was not tested on any real hardware at the moment. |
|
I'd like the GPIO pin operation macros to keep it simple and direct, because I'll use those macros in the interrupt handling routine as well. So don't add atomic operations to the existing macros. Instead, you are welcome to add a new macro with atomic operations, such as the following
|
|
The issue is that most of the existing code (e.g., quantum/matrix.c, which is the main culprit, but any other GPIO access may cause the same problem) must use atomic GPIO operations. So the way to make this 100% correct is to make the existing operation atomic (they will still work inside interrupt handlers, although slower than necessary), and then maybe add In addition, The macro implementation did not work again, this time due to the code in the Looks like I need to wrap those macros in a layer of |
Use the gcc statement expression extension to make AVR GPIO macros
usable inside expressions, in particular, in a ternary operator:
level ? writePinHigh(pin) : writePinLow(pin)
The code using a ternary operator was found in the `chidori` keyboard;
it was apparently written before `writePin(pin, level)` was introduced.
The implementation of `writePin(pin, level)` is also reverted to the one
using the ternary operator, like shown above, to make sure that this
style of code is kept working.
There is still a difference from the old implementation: old macros
returned the value of the last modified register (`PORTx` or `DDRx`),
while new ones have type `void`. It is not possible to keep the old
behavior without losing the `cbi`/`sbi` optimization, and if any old
code was trying to actually use that return value, it should break in an
obvious way with new macros.
drashna
added
the
breaking_change
|
If this is an issue, then it is something that should be fixed. But given how the functions are used, it definitely needs to be handled carefully. |
|
The unfortunate thing here is that apparently the only case when AVR GPIOs are manipulated from interrupt handlers is the I also looked at how ChibiOS implements the GPIO access. The documentation for palSetLine() and other similar macros says that those operations are not guaranteed to be atomic, so the problem may potentially happen there too, if some code tries to manipulate GPIOs from interrupt handlers or other threads. For STM32 the actual implementation of GPIO functions in For KINETIS the quality of implementation in |
|
This has merge conflicts |
|
Isn't this already done? https://github.com/qmk/qmk_firmware/blob/master/quantum/matrix.c#L83 |
|
Yes, #10491 solved the most important part of the problem, and a more general solution might not be needed for now, so this PR can be closed. |
Description
On AVR MCUs the GPIO macros like
writePinLow()perform read-modify-write operations onPORTxandDDRxregisters, unless their argument is a compile time constant (in that case the compiler can optimize those operations into singlecbiorsbiinstructions). Usually this works without any problems; however, the software PWM emulation code inquantum/backlight/backlight_avr.cmanipulates GPIOs from interrupt handlers, and this may cause issues which manifest in a flickering backlight on some boards (#5953). The problem happens when the read-modify-write sequence performed by the matrix scanning code is interrupted by the software PWM handler, and the software PWM uses a pin from the same port as one of the matrix row pins (or column pins for a ROW2COL matrix); in this case the resumed read-modify-write code will overwrite the update performed by the software PWM emulation code.Change the implementation of all GPIO functions so that they are atomic with respect to interrupts; this is achieved by disabling interrupts around the read-modify-write operations. This fixes #5953, and should not break other boards, although there will be some additional overhead for some GPIO operations (but the most common case of performing
writePinLow()orwritePinHigh()on pins which are known at compile time should have no additional overhead, because in this case the assembly implementation usingcbiorsbishould be picked by the compiler).This change fixes the issue #5953 (tested on the XD87 HS board), at least if the RGB underglow is not used (a running rgblight animation would still cause noticeable flicker, especially when the backlight brightness is low, but that is a separate issue). However, I'm not really happy about adding overhead to all those GPIO function (although that overhead may be not that large in practice — I measured the scanning rate on XD87 HS, and this change decreased it from 2187 to 2077 scans per second, which is not very significant).
Technically some part of these changes are not really required to fix the backlight issue:
PORTxregister from interrupts, therefore the code which accessesDDRxmay be left as is, but that just does not feel right.setPinInput()andsetPinInputHigh()as a whole is not required — it could be possible to add a special case for the constant argument there and just perform a sequence of twocbi/sbiinstructions without disabling interrupts. However, that would result in a possibility of receiving an interrupt in an intermediate state when the pin is already configured as input, but the pullup state is wrong. (Note that for switching from input to output that possibility still exists, and the intermediate state in that case might be even worse — the pin could actively output a wrong logical level for some time; maybe we should add functions likesetPinOutputLow()andsetPinOutputHigh(), which changePORTxbeforeDDRx, and usesetPinOutputLow()in the matrix scanning code.)Types of Changes
Issues Fixed or Closed by This PR
Checklist