bit of an opinion based question here...
I'm currently using a uController(cc1310) that implements the interrupt on change from all the 32 pins in a single callback.
By enabling the interrupt for a given pin, said callback will be, well, called and we need to check which pin generated the interrupt, by doing:
/* gpio_interrupt_callback is registered in the interrupt vector */
void gpio_interrupt_callback( void )
{
uint32_t pinMask = HWREG( GPIO_BASE + GPIO_O_EVFLAGS31_0 );
if (pinMask & 0x00000001)
{
/* pin_0 generated interrupt */
}
...
}
At this point, each bit in pinMask is the corresponding pin interrupt flag.
Until now, each application needed only one pin/process to use an ioc event, so every time we needed it, we'd just register our callback and it'd be done.
This last application will use more, so I'm thinking of making a GPIO_interrupt module, where I could register a callback for a given pin, and the module would filter by pinMask and call the correct callback.
So...
which option do the Gurus suggest?
a)
I can add only the if's for the interrupts I'ḿ using
typedef void (*pin_interrupt_callback_t)( void );
pin_interrupt_callback_t pin0_callback;
/* gpio_interrupt_callback is registered in the interrupt vector */
void gpio_interrupt_callback( void )
{
uint32_t pinMask = HWREG( GPIO_BASE + GPIO_O_EVFLAGS31_0 );
if (pinMask & 0x00000001)
{
HWREG( GPIO_BASE + GPIO_O_EVFLAGS31_0 ) |= 0x00000001;
/* pin_0 generated interrupt */
if(NULL != pin0_callback)
pin0_callback();
}
else if (pinMask & 0x00000002)
{
...
}
...
}
b)
This way it's a bit more dynamic?, where I can add or remove callbacks to/from the table (A table could also be used in a) though)
typedef void (*pin_interrupt_callback_t)( void );
pin_interrupt_callback_t pin_callback[32] = {
pin0_callback,
pin1_callback,
...
}
/* gpio_interrupt_callback is registered in the interrupt vector */
void gpio_interrupt_callback( void )
{
uint32_t pinMask = HWREG( GPIO_BASE + GPIO_O_EVFLAGS31_0 );
for(uint8_t i = 0; i < 32; i++)
{
if(pinMask & (1 << i))
if(NULL != pin_callback[i])
pin_callback[i]();
}
}
c)
This way is the most complex one, but I'll only cycle through the existing interrupts, and I can register the same callback for several pins (can also do that in a) and b), but here it will only be called once if more than one interrupt happen at the same time)
And we can even register more than one callback for each pin
typedef void (*pin_interrupt_callback_t)( uint32_t mask );
typedef struct pin_callback_element
{
uint32_t callbackMask;
pin_callback_element *next;
pin_interrupt_callback_t callback;
} pin_callback_element_t;
pin_callback_element_t *pin_callbacks = NULL;
/* elem must be declared as `static pin_callback_table_t elem;` */
uint8_t register_callback(
pin_callback_element_t *elem,
uint32_t pinMask,
pin_interrupt_callback_t callback )
{
/* Make sure no element is registered for a NULL callback */
elem->next = NULL;
elem->callbackMask = pinMask;
elem->callback = callback;
/* Add element to the pin_callbacks list */
}
/* gpio_interrupt_callback is registered in the interrupt vector */
void gpio_interrupt_callback( void )
{
uint32_t pinMask = HWREG( GPIO_BASE + GPIO_O_EVFLAGS31_0 );
pin_callback_element_t *p_cp = pin_callbacks;
while(NULL != p_cp)
{
if(0 != (pinMask & p_cp->callbackMask))
p_cp->callback(pinMask);
p_cb = p_cb->next;
}
}
or d)
None of the above.
I know interrupt callbacks must be as fast and small as possible, but the module we're now making can be used by other applications, that may or may not use other pin interrupts on their own, so I think it would be a bad idea to have common code be altered according to the other apps
Sorry for the long post and thanks for the feedback
I'd go with b) since you can/should register the callbacks in a read-only look-up table. This table should ideally be set at compile-time and placed where you have all your other GPIO-related constants.
The reason for this is that it's very annoying to have each decentralized part of a project mucking around with GPIO internally. You'll want a centralized list where you can get an overview over which pins are inputs, outputs, floating, pulled etc. That way you can easily review/verify the software against the schematic and vice versa. And also avoid conflicts where different parts of the program are using the same pins by accident.
You could in theory make a dynamic read/write list as in c), a linked list where different callers can register callbacks. I often create such for general-purpose timers, but in the specific case of GPIO which is very close to the hardware, I would advise against it for the already mentioned reasons.
So maybe something like this:
typedef void callback_t (void);
#define CALLBACK_N 32u
static callback_t* const callback [CALLBACK_N] = // function ptr table in read-only flash
{
[1] = callback_1,
[22] = callback_22
};
This means that all the indices we didn't initialize explicitly will get set to null implicitly.
It's unfortunate that the weird TI libs return a bit mask instead of a bit number, or the for loop in the ISR could have been removed and replace with a direct access into the look-up table.
Some minor nit-picks:
In the ISR maybe change the order of the if statements for slightly faster execution (micro optimization):
if(pin_callback[i] != NULL) // check vs zero will be single instruction
if(pinMask & (1u << i))
Always u suffix integer constants. Always do 1u <<, never do 1 <<. The latter could invoke undefined behavior in case you shift data into the sign bit of the signed int that 1 created.
There's no need for "yoda conditions" NULL != pin_callback[i] nonsense from the 1980s era. Just use a half-decent compiler like Turbo C from 1989 or later - they will warn against incorrect assignments in conditions. And with my fix above, the function pointer table is read-only anyway.