Skip to content

Potential race condition in fifo.c causing freezes on modifier keys #25

@0xhackworth

Description

@0xhackworth

I've been looking into the freezing issues reported when using modifier keys (Alt/Sym/Shift) rapidly. Looking at app/fifo.c and app/keyboard.c, there appears to be a race condition.

​The Issue:
The timer_task (which handles scanning) runs in an interrupt context. It calls fifo_enqueue.
The main loop (or USB callbacks) calls fifo_dequeue.

Since fifo.c does not disable interrupts during these operations, self.count and self.write_idx can be corrupted if an interrupt fires in the middle of a read/write. This likely leads to the deadlock/freeze users are seeing.

​Proposed Fix:
I believe fifo.c needs to use hardware/sync.h to wrap the queue access in critical sections.

​I don't have the hardware on hand to test this compilation myself, but the logic below adds the necessary locking:

#include "app_config.h"
#include "fifo.h"
#include "hardware/sync.h" // Requires Pico SDK

static struct
{
	struct fifo_item fifo[KEY_FIFO_SIZE];
	uint8_t count;
	uint8_t read_idx;
	uint8_t write_idx;
} self;

uint8_t fifo_count(void)
{
	uint32_t interrupts = save_and_disable_interrupts();
	uint8_t count = self.count;
	restore_interrupts(interrupts);
	return count;
}

void fifo_flush(void)
{
	uint32_t interrupts = save_and_disable_interrupts();
	self.write_idx = 0;
	self.read_idx = 0;
	self.count = 0;
	restore_interrupts(interrupts);
}

bool fifo_enqueue(const struct fifo_item item)
{
	uint32_t interrupts = save_and_disable_interrupts();
	
	if (self.count >= KEY_FIFO_SIZE) {
		restore_interrupts(interrupts);
		return false;
	}

	self.fifo[self.write_idx++] = item;
	self.write_idx %= KEY_FIFO_SIZE;
	++self.count;

	restore_interrupts(interrupts);
	return true;
}

void fifo_enqueue_force(const struct fifo_item item)
{
	uint32_t interrupts = save_and_disable_interrupts();
	
	// Handle logic inline to avoid re-enabling interrupts prematurely via nested call
	if (self.count < KEY_FIFO_SIZE) {
		self.fifo[self.write_idx++] = item;
		self.write_idx %= KEY_FIFO_SIZE;
		++self.count;
	} else {
		self.fifo[self.write_idx++] = item;
		self.write_idx %= KEY_FIFO_SIZE;
		self.read_idx++;
		self.read_idx %= KEY_FIFO_SIZE;
	}
	
	restore_interrupts(interrupts);
}

struct fifo_item fifo_dequeue(void)
{
	uint32_t interrupts = save_and_disable_interrupts();
	
	struct fifo_item item = { 0 };
	if (self.count == 0) {
		restore_interrupts(interrupts);
		return item;
	}

	item = self.fifo[self.read_idx++];
	self.read_idx %= KEY_FIFO_SIZE;
	--self.count;

	restore_interrupts(interrupts);
	return item;
}

fifo.c

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions