[Core] Use a mutex guard for split shared memory #16647
Conversation
...on platforms that support it, which is only ChibiOS at this point. The problem: The shared memory on the ChibiOS platform can be accessed concurrently on the secondary device. This happens when the serial transaction thread, reacts to transactions requests by the main side of the keyboard. This can lead to race conditions and corrupted transactions. The solution: The access to the split shared memory is made exclusive at any given time by guarding it with a mutex. The previous solution using a critical section didn't really work out because serial functions like send and receive would disable and enable interrupts on their own, thus leaving the system with enabled interrupts after the first transaction. The same was true for slave transaction handlers for e.g. sync timers, those functions effectively broke the critical section as well.
KarlK90
force-pushed
the
feature/mutex_guard_split_shared_memory
branch
from
baf6606 to
c6ee6bf
Compare
|
Seems to work well on the blackpill f411 and the f303. Or at least, not cause any apparent issuse. However, this removes the atomic option for avr, without implementing it elsewhere? |
This is true, but I don't see a good clean way to implement this feature on AVR with the lack of a Mutex primitive. On the other hand with the removal of the critical sections aka. So the before the call order was:
So not having the code at all for AVR makes it a little bit worse in theory. |
I don't think it's actually enabling interrupts when leaving the timer stuff at least. I had to manually enable interrupts for the timer to update. daskygit@e4ad2bd?w=1 (this fixes i2c split usb enumeration when only using a single side). The few people that have tested it didn't report any issues. I'm assuming the changes above will also allow i2c splits to work correctly. |
|
Been using this for a few days, and it seems to cause debouncing issues on my tractyl manuform (blackpill f411). Not sure if that's a bug caused by this code, or because it's handling it properly, basically (especially since most but not all of the issues appear to be on the secondary half). |
Hmmn, I'll think about a way to measure unusual long locks of the mutex. With debouncing issues you mean that keys get stuck or that input is not registered? |
|
Specifically, they get registered repeatedly, like a bad joint type behavior. But removing this code change (and just this change) fixes the issue. |
|
Do you have link to your current firmware build? I don't have the pwm sensors but a F411 test bed, I'll try to reproduce and investigate the issues. |
|
Re-added the code change to the branch I was using, and split it off. |
Thanks for the feedback 🙂 |
Description
...on platforms that support it, which is only ChibiOS at this point. Further more the wrong usage of the API would result in lockups on the RP2040 QMK port. With these changes ChibiOS no longer halts with a critical system error when enabling the state check.
The problem:
The shared memory on the ChibiOS platform can be accessed concurrently on the secondary device. This happens when the serial transaction thread reacts to transactions requests by the main side of the keyboard. This can lead to race conditions and corrupted transactions.
The solution:
The access to the split shared memory is made exclusive at any given time by guarding it with a mutex.
The previous solution using a critical section didn't really work out because serial functions like send and receive would disable and enable interrupts on their own, thus leaving the system with enabled interrupts after the first transaction. The same was true for slave transaction handlers for e.g. sync timers, those functions effectively broke the critical section as well.
Tested succesfully with RP2040 and STM32F411 splits using the usart and bitbang driver
Types of Changes
Issues Fixed or Closed by This PR
Checklist