cpu/esp32: use ESP-IDF interrupt handling API#18261
Conversation
github-actions
bot
added
Area: cpu
gschorcht
added
Type: enhancement
| /* set interrupt handler and enable the CPU interrupt */ | ||
| xt_set_interrupt_handler(CPU_INUM_RTC, _rtc_isr, NULL); | ||
| xt_ints_on(BIT(CPU_INUM_RTC)); | ||
| xt_set_interrupt_handler(CPU_INUM_RTT, _rtc_isr, NULL); |
There was a problem hiding this comment.
For this file only the define CPU_INUM_RTC is renamed to keep it compilable. The changes for ESP-IDF for interrupt handling for this file are in commit 6cca58a.
| /* set interrupt handler and enable the CPU interrupt */ | ||
| xt_set_interrupt_handler(CPU_INUM_RTC, _sys_isr, NULL); | ||
| xt_ints_on(BIT(CPU_INUM_RTC)); | ||
| xt_set_interrupt_handler(CPU_INUM_RTT, _sys_isr, NULL); |
There was a problem hiding this comment.
For this file only the define CPU_INUM_RTC is renamed to keep it compilable. The changes for ESP-IDF for interrupt handling for this file are in commit 2a9ffee.
cpu/esp32/irq_arch.c
Outdated
|
|
||
| typedef struct intr_handle_data_t { | ||
| int src; /* peripheral interrupt source */ | ||
| uint32_t intr; /* interrupt number */ |
There was a problem hiding this comment.
Does this need to be 32 bit?
There was a problem hiding this comment.
Good question. All functions of ESP-IDF use int for interrupt numbers which is 32 bit. Implicit type casts shouldn't be a problem but accessing smaller elements in structs might be less efficient.
cpu/esp32/irq_arch.c
Outdated
| } intr_handle_data_t; | ||
|
|
||
| /* TODO change to a clearer approach */ | ||
| static struct intr_handle_data_t _irq_data_table[] = { |
There was a problem hiding this comment.
Unfortunatly not. The address of the according element in the table is used as handle and the assignment here
Line 161 in 7fbf42e
The index in the table could also be used as a handle. But I decided to spend these bytes in RAM for the table and to use the address of the element as handle instead of the index, because this can then be used directly in all other functions that get the handle as parameter. It was just a tradeoff of performance and ressources. The size of the RAM is of secondary importance in ESP SoCs.
There was a problem hiding this comment.
I pushed commit 03d1b16 just for demonstration how it could be implemented with the index as interrupt handle and with a number of type casts.
Unfortunatly, the ESP-IDF uses a forward declaration for struct intr_handle_data_t and declares intr_handle_t as pointer to this structure.
typedef struct intr_handle_data_t intr_handle_data_t;
typedef intr_handle_data_t *intr_handle_t ;
That is, it expects that intr_handle_t is a pointer to a struct intr_handle_data_t without defining its structure. That's why, I used the structure of entries in the interrupt data table directly as struct intr_handle_data_t and the address of the elements as intr_handle_t.
With commit 036b1d6, a simple type cast would allow to declare the table as static const which saves 192 byte (with uint32_t for the interrupt number) or 128 bytes (with uint8_t for the interrupt number). IMHO, this would be a good compromise and more clear than the tricky use of index as interrupt handle.
There was a problem hiding this comment.
@benpicco Which one of the two approaches would you prefer? I would clean up the PR for the next compilation run accordingly.
BTW, the hashes seem to be still a problem.
There was a problem hiding this comment.
I think you can better judge the trade-offs of the two solutions, I'm fine with either.
Just squash if you have something you are happy with.
Unfortunately I don't have any ideas what causes the hash mismatch.
There was a problem hiding this comment.
Unfortunately I don't have any ideas what causes the hash mismatch.
Ah, this time I had luck. The compilation has been successful 👍
There was a problem hiding this comment.
Just squash if you have something you are happy with.
Finally I squashed that version which uses the address of the elements as handle and the single type cast.
gschorcht
force-pushed
the
cpu/esp32/use_esp_idf_interrupt_handling
branch
from
036b1d6 to
4be0abf
Compare
|
@benpicco Thanks for reviewing and merging. Now I can rebase different PRs that are then compilable. |
3 participants
Contribution description
This PR is a splitt-off from PR #17841 and replaces partially PR #18247.
It provides the changes in interrupt handling for ESP32x SoCs to use the ESP-IDF interrupt handling API. Also,
CPU_INUM_RTCis renamed toCPU_INUM_RTTto correctly identify the interrupt source.Testing procedure
Issues/PRs references
Splitt-off from PR #17841 and PR #18247
Replaces PR #18247 partially