cpu/esp32: use ESP-IDF interrupt handling API #18261

gschorcht · 2022-06-25T12:00:59Z

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_RTC is renamed to CPU_INUM_RTT to correctly identify the interrupt source.

Testing procedure

Green CI

Compile and check any simple test app, for example:

BOARD=esp32-wroom-32 make -j8 -C tests/shell flash term

Issues/PRs references

Splitt-off from PR #17841 and PR #18247
Replaces PR #18247 partially

gschorcht · 2022-06-25T22:46:21Z

cpu/esp32/periph/rtt_hw_rtc.c


 /* 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);


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.

gschorcht · 2022-06-25T22:46:50Z

cpu/esp32/periph/rtt_hw_sys.c


 /* 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);


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.

benpicco · 2022-06-26T15:40:26Z

cpu/esp32/irq_arch.c

+#define ETS_CAN_INTR_SOURCE ETS_TWAI_INTR_SOURCE
+#endif
+
+typedef struct intr_handle_data_t {
 int src; /* peripheral interrupt source */
 uint32_t intr; /* interrupt number */


Does this need to be 32 bit?

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.

benpicco · 2022-06-26T15:41:04Z

cpu/esp32/irq_arch.c

+} intr_handle_data_t;
+
+/* TODO change to a clearer approach */
+static struct intr_handle_data_t _irq_data_table[] = {


Can this be const?

Unfortunatly not. The address of the according element in the table is used as handle and the assignment here

RIOT/cpu/esp32/irq_arch.c

Line 161 in 7fbf42e

*((intr_handle_t *)ret_handle) = &_irq_data_table[i];

discards the const qualifier.

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.

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.

@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.

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.

Unfortunately I don't have any ideas what causes the hash mismatch.

Ah, this time I had luck. The compilation has been successful 👍

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.

cpu/esp32/irq_arch.c

gschorcht · 2022-06-28T14:49:04Z

@benpicco Thanks for reviewing and merging. Now I can rebase different PRs that are then compilable.

gschorcht requested a review from benpicco June 25, 2022 12:01

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Jun 25, 2022

gschorcht added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 25, 2022

gschorcht mentioned this pull request Jun 25, 2022

cpu/esp32: platform independent code for thread_arch, irq_arch and exception #18247

Closed

benpicco requested a review from maribu June 25, 2022 17:03

gschorcht commented Jun 25, 2022

View reviewed changes

gschorcht mentioned this pull request Jun 26, 2022

cpu/esp32: add ESP-IDF API #18253

Merged

benpicco reviewed Jun 26, 2022

View reviewed changes

gschorcht mentioned this pull request Jun 27, 2022

cpu/esp32: replace RIOT ESP32 SoC startup function by ESP-IDF SoC startup function #18268

Merged

gschorcht added 2 commits June 28, 2022 07:30

cpu/esp32: use ESP-IDF for interrupt handling

6bd6afb

cpu/esp32: rename CPU_INUM_RTC to CPU_INUM_RTT

4be0abf

gschorcht force-pushed the cpu/esp32/use_esp_idf_interrupt_handling branch from 036b1d6 to 4be0abf Compare June 28, 2022 05:31

gschorcht mentioned this pull request Jun 28, 2022

cpu/esp32: use ESP-IDF gpio HAL for periph/gpio #18270

Merged

benpicco approved these changes Jun 28, 2022

View reviewed changes

benpicco merged commit 1b2360e into RIOT-OS:master Jun 28, 2022

gschorcht deleted the cpu/esp32/use_esp_idf_interrupt_handling branch June 28, 2022 14:50

gschorcht mentioned this pull request Jun 28, 2022

cpu/esp32: use ESP-IDF i2c HAL for ESP32 periph/i2c #18279

Merged

gschorcht mentioned this pull request Jul 18, 2022

cpu/esp32: changes to use ESP-IDF HAL/LL API #17841

Closed

chrysn added this to the Release 2022.07 milestone Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/esp32: use ESP-IDF interrupt handling API #18261

cpu/esp32: use ESP-IDF interrupt handling API #18261

gschorcht commented Jun 25, 2022

gschorcht Jun 25, 2022

gschorcht Jun 25, 2022

benpicco Jun 26, 2022

gschorcht Jun 26, 2022 •

edited

Loading

benpicco Jun 26, 2022

gschorcht Jun 26, 2022

gschorcht Jun 26, 2022

gschorcht Jun 27, 2022

benpicco Jun 27, 2022 •

edited

Loading

gschorcht Jun 28, 2022

gschorcht Jun 28, 2022

gschorcht commented Jun 28, 2022

cpu/esp32: use ESP-IDF interrupt handling API #18261

cpu/esp32: use ESP-IDF interrupt handling API #18261

Conversation

gschorcht commented Jun 25, 2022

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschorcht Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschorcht commented Jun 28, 2022

gschorcht Jun 26, 2022 •

edited

Loading

benpicco Jun 27, 2022 •

edited

Loading