[RFC, PoC, etc.] GPIO abstractions

[tzarc]

Early proposal

Do not assume any PoC will be merged as-is, this is intended to explore future designs.

This issue is to track potential rework or redesign of the GPIO definitions within QMK.
Part of the complexity in dealing with a lot of alternative pin definitions is the fact that there's no cohesive way to integrate "mismatching" types of GPIOs in matrix definitions or peripheral connections.

Current definitions of pin_t are purely limited to MCU-hosted pins.

Initial proposal is to redefine pin_t as a struct, with an enum to determine which underlying system services the request -- an example:

typedef enum pintype_t {
	PIN_MCU,

#ifdef USE_MCP23017
	PIN_MCP23017.
#endif USE_MCP23017

#ifdef USE_MCP2SS17
	PIN_MCP23S17.
#endif USE_MCP23S17
} pintype_t;

typedef struct pin_impl_t {
	pintype_t type;
	union {
		uint16_t mcu_pin;

#ifdef USE_MCP23017
		struct {
			uint8_t i2c_bus_idx;
			uint8_t i2c_addr;
			uint8_t pin_num;
		} mcp23017;
#endif USE_MCP23017		

#ifdef USE_MCP23S17
		struct {
			uint8_t spi_bus_idx;
			pin_t *cs_pin;
			uint8_t pin_num;
		} mcp23S17;
#endif USE_MCP23S17		
	};
} pin_impl_t;

typedef const pin_impl_t *pin_t;

// Determine the base set of APIs required
typedef struct gpio_pin_vtable_t {
	// Perhaps consolidate these with another "pin_pull_t" parameter?
	void (*set_pin_input)(pin_t pin);
	void (*set_pin_input_low)(pin_t pin);
	void (*set_pin_input_high)(pin_t pin);

	void (*set_pin_output)(pin_t pin);

	bool (*read_pin)(pin_t pin);
	void (*write_pin)(pin_t pin, bool value);

	// Things like AF modes
	void (*set_pin_function)(pin_t pin, int mode);

	// ...
} gpio_pin_vtable_t;

// #define GP23 23U
static const pin_t pin_GP23 = {.type = PIN_MCU, .mcu_pin = 23U};
#define GP23 (&pin_GP23)

static const gpio_pin_vtable_t *pin_vtables[] = {
	[PIN_MCU] = &gpio_pin_vtable_mcu,

#ifdef USE_MCP23017
	[PIN_MCP23017] = &gpio_pin_vtable_mcp23017,
#endif USE_MCP23017		

#ifdef USE_MCP23S17
	[PIN_MCP23S17] = &gpio_pin_vtable_mcp23S17,
#endif USE_MCP23S17		
};

#define readPin(pin) \
	(pin_vtables[pin->type]->read_pin(pin))

readPin as an example is purely that -- would need error checking and the like, and the vtables themselves would just assign NULL to items they don't have the ability to do.

[elpekenin]

I'd like to help with this. Couple things:

1. Where should this code be located? Im thinking on a pin folder where pin.h defines the types and macros and pin.c "merges" all the types on the vtables array. And several, rather small, .c files with type-specific logic
   • When using basic MCU pins, the way to interact with a pin would be different (ChibiOS vs VUSB for example), so those definitions should be defined on platforms
   • Other "pins" should be able to use QMK abstractions to talk with IC's over SPI or whatever, so that should probably live on quantum

Is that correct?

2. We would like some extra checks, wouldn't we?
   • Eg, ifdef MCU_RP2040 only the last GPIOs can be used for analog operation, so we could have a if (pin->mcu_pin > 25U) (wrote a random number here) to do dprintf("Analog reading not supported on pin %d\n", pin->mcu_pin)
   • Is this handled already, to some extend?

[sigprof]

typedef struct pin_impl_t {
	pintype_t type;
	union {
		uint16_t mcu_pin;

This would probably end up being something like platform_mcu_pin_t mcu_pin, with platform_mcu_pin_t being defined as ioline_t for ChibiOS, uint8_t for AVR, or whatever it is called in RIOT.

Note that ioline_t in ChibiOS is defined by the HAL port code, so the underlying type may still vary. Currently all ChibiOS ports we care about use uint32_t (HT32 uses uintptr_t, but it's uint32_t under the hood anyway); AVR ports have a struct there, but QMK does not support those ports. In the STM32 implementation the ioline_t value actually embeds the pointer to the GPIO register block inside (the code assumes that 4 low bits of that pointer are 0, and uses those bits to store the pad number); direct usage of the GPIO number on RP2040 is a really special case.

With uint32_t there the pintype_t type field would occupy the same space as gpio_pin_vtable_t *vtable due to alignment; so the space optimization would really affect only the AVR case (but that would also be the case where it's the most needed — on proper MCUs that const data won't be copied to RAM anyway). Having the vtable pointer there directly would also allow the linker to discard a GPIO implementation that ends up completely unused, but that optimization may not be really important (the proper way is to leave the unused implementations disabled). Unfortunately, there seems to be no way to discard methods that are never actually used anywhere (apart from adding config options for them too, but that would be lots of ifdefs scattered around all GPIO implementations).

Also with some casts we could use separate structs for pin types instead of a single union for all types, and save some bytes (because the structs won't be padded to the maximum possible size).

[sigprof]

• Eg, ifdef MCU_RP2040 only the last GPIOs can be used for analog operation

Currently the ChibiOS analog driver code would just silently return 0 if you call analogReadPin() for an unsupported pin, because pinToMux() would return an invalid value pointing to a nonexistent ADC. The AVR code apparently attempts to select some nonexistent ADC input channel which gives 0V too (but the conversion would still be performed and may read some noise).

Making this code work with the new GPIO abstraction would require either rewriting parts of it (things like case F0: won't work), or making the code dig into the abstraction (check for pin->type == MCU_PIN, then extract the mcu_pin value and work from there). Another possible solution is adding the analog_read method to GPIO methods — this way you could potentially implement support for external ADCs, but in this case a config option around that method would definitely be required, otherwise the analog support would always be referenced.

[elpekenin]

check for pin->type == MCU_PIN, then extract the mcu_pin value and work from there

Yeah, was thinking of vtables containing that function, so that other abstractions can also be used for analog reads + at that point we can (hopefully?) perform a guard clause like the one i wrote, on the MCU-pin function, before calling the underlying implementation

[tzarc]

So the casting would result in something like:

typedef struct pin_base_t {
	pin_type_t type;
} pin_base_t;

typedef struct pin_chibios_t {
	pin_base_t base;
	ioline_t line;
} pin_chibios_t;

typedef struct pin_avr_t {
	pin_base_t base;
	uint8_t pin;
} pin_avr_t;

// #define GP29 29U
const pin_chibios_t pin_gp29 = {.base = {.type = MCU_PIN}, .line = 29U};
#define GP29 ((const pin_base_t*)&pin_gp29)

Though as mentioned, at that point we may as well just embed the vtable directly, as if it were C++:

typedef struct pin_chibios_t {
	gpio_pin_vtable_t vtable;
	ioline_t line;
} pin_chibios_t;

typedef struct pin_avr_t {
	gpio_pin_vtable_t vtable;
	uint8_t pin;
} pin_avr_t;

// #define GP29 29U
const pin_chibios_t pin_gp29 = {.vtable = &gpio_pin_vtable_mcu_chibios, .line = 29U};
#define GP29 ((const gpio_pin_vtable_t*)&pin_gp29)

[sigprof]

I wonder whether we would need to make this extra abstraction layer optional (especially on AVR the overhead may be significant; also the code using those abstractions would probably require at least LTO to be fully optimized, because otherwise the compiler won't see the constant values stored in pin objects and vtables).

#if GPIO_EXPANDER_ENABLE

/* Using GPIO abstractions */

typedef struct pin_vtable_t {
    bool (*read_pin)(pin_t pin);
    void (*write_pin)(pin_t pin, bool value);
} pin_vtable_t;

typedef struct pin_base_t {
    const pin_vtable_t *vtable;
} pin_base_t;

typedef const pin_base_t *pin_t;

typedef struct pin_platform_t {
    pin_base_t         base;
    pin_platform_raw_t raw_pin;
};

extern const pin_vtable_t pin_vtable_platform;

#    define DECLARE_PLATFORM_PIN(name) extern const pin_platform_t pin_##name
#    define DEFINE_PLATFORM_PIN(name, value)             \
        const pin_platform_t pin_##name = {              \
            .base    = {.vtable = &pin_vtable_platform}, \
            .raw_pin = (value),                          \
        }
#    define PLATFORM_PIN_VALUE(name, value) (&pin_##name.base)

#    define PIN_GET_OP(pin, op) ((pin)->vtable->op)

#else

/* Using only raw MCU pins */

typedef pin_platform_raw_t pin_t;

#    define DECLARE_PLATFORM_PIN(name) struct pin_base_t
#    define DEFINE_PLATFORM_PIN(name, value) struct pin_base_t
#    define PLATFORM_PIN_VALUE(name, value) value

#    define PIN_GET_OP(pin, op) platform_raw_##op

#endif

inline bool readPin(pin_t pin) {
    return PIN_GET_OP(pin, read_pin)(pin);
}

inline void writePin(pin_t pin, bool value) {
    PIN_GET_OP(pin, write_pin)(pin, value);
}

Here pin_platform_raw_t is intended to be what is pin_t now, and the platform code would need to provide things like bool platform_raw_read_pin(pin_platform_raw_t raw_pin), void platform_raw_write_pin(pin_platform_raw_t raw_pin, bool value), …

A pin_defs.h file would have things like

DECLARE_PLATFORM_PIN(B0);
#define B0 PLATFORM_PIN_VALUE(B0, PAL_LINE(GPIOB, 0U))

and would also need a corresponding pin_defs.c file with:

DEFINE_PLATFORM_PIN(B0, PAL_LINE(GPIOB, 0U))

(not sure what to do with the duplication there, except maybe generating those files — a macro can't output a #define)

[elpekenin]

BTW im making some experiments over here(still minimal, it compiles but havent flashed anything yet).

Sharing in case someone wants to provide some feedback, help with it, and trying to avoid other people duplicating work

[elpekenin]

Made some progress, would love some feedback 🤓

https://github.com/elpekenin/qmk_firmware/tree/pin-playground

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

[github-actions]

This issue has been automatically closed because it has not had activity in the last 30 days. If this issue is still valid, re-open the issue and let us know.
// [stale-action-closed]