cpu/kinetis/periph_gpio_irq: fix re-enabling an existing interrupt#14376
cpu/kinetis/periph_gpio_irq: fix re-enabling an existing interrupt#14376benemorius wants to merge 4 commits into
Conversation
It's calculated in bytes but is used to declare an array of uint32_t. sizeof(isr_map) went from 448 to 112 (in bss)
benpicco
added
Area: cpu
| static inline int get_ctx(int port, int pin) | ||
| { | ||
| return (isr_map[(port * 4) + (pin >> 3)] >> ((pin & 0x7) * 4)) & 0xf; | ||
| return ((isr_map[(port * 4) + (pin >> 3)] >> ((pin & 0x7) * 4)) & 0xf) - 1; |
There was a problem hiding this comment.
Should this also be done in write_map?
There was a problem hiding this comment.
The equivalent is done in the only consumer of write_map() that needs it, in gpio_init_int()
| isr_ctx[ctx_num].arg = arg; | ||
| isr_ctx[ctx_num].state = flank; | ||
| write_map(port_num(pin), pin_num(pin), ctx_num); | ||
| /* context map requires ctx_num + 1, as 0 is the no-context value */ |
There was a problem hiding this comment.
I would have thought ctx_num < 0 would indicate no context - or how is this meant?
There was a problem hiding this comment.
Because of the efficient way that someone did the isr_map, we need the isr_map to store 0 when a pin has no context to map to. This means that when we want to map a pin to isr_ctx 0, we actually need to write 1 to isr_map.
It would read cleaner to rewrite everything to use -1 for no-context but that isn't compatible with how the bitmapped isr_map is done. But I could be mistaken. I didn't try that hard.
| * @brief Calculate the needed memory (in bytes) to store 4 bits per MCU pin | ||
| */ | ||
| #define ISR_MAP_SIZE (GPIO_PORTS_NUMOF * PINS_PER_PORT * 4 / 8) | ||
| #define ISR_MAP_SIZE (GPIO_PORTS_NUMOF * PINS_PER_PORT * 4 / 8 / sizeof(uint32_t)) |
There was a problem hiding this comment.
So just for my understanding: we can have 16 interrupts in total, mapped to any pin?
There was a problem hiding this comment.
Yes, more or less. We can have up to all pins mapped, with up to CTX_NUMOF unique callbacks to map them to.
isr_map is what maps a pin to (if applicable) a callback context in isr_ctx
In isr_map each pin gets 4 bits to specify an isr_ctx which is 16 minus 1 possible callbacks that a pin can point to. It's minus 1 because we need a value to indicate that there is no isr_ctx for that pin in isr_map and that value is 0. There used to be no concept of a no-context value in isr_map and that's what the fix irq contexts not being freed commit is for.
An additional limitation is CTX_NUMOF which currently limits us to only 8 callback contexts in isr_ctx. We could increase this to 15 before having to change the workings of isr_map.
|
It's been a long while since I wrote these commits and apparently the fact that I can't easily answer you @benpicco without studying the code means that the code or comments need further improvement. I'll try to get to this soon. |
|
@benpicco I had hoped to address your comments by changing code or comments but I didn't seen much opportunity for that. Let me know if you didn't understand my responses or if you'd still like to suggest changes. |
|
Sorry for dropping the ball on this. |
|
@benpicco if you found the PR comments adequate I will use them to supplement the code comments. Otherwise we can continue to discuss the confusion until we get it right and then I will update the code comments accordingly. |
|
Nevermind, I did actually find some code changes to improve readability. With that plus the comments I moved from PR comments to code comments, hopefully this is in better shape. Let me know what you think @benpicco These changes didn't fit well as fixup commits so I left it as a proper commit. |
|
ping @benpicco! |
|
Can we move this one forward? |
4 participants
Contribution description
This PR has 3 related bugfixes that I've split out of #11789. In that PR, the UART implementation is modified to use GPIO interrupts to wake from low power. It fails to work after the first received character without these fixes. IIRC disabling and re-enabling a GPIO interrupt always triggers these bugs.
Testing procedure
Issues/PRs references
This PR is needed for #14377 to work properly.