ESP32C3 interrupts, GPIO and UART support #2167
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ld.lld: error: .../src/device/esp/esp32c3_intr.S:108:(.iram0.exception_vectors+0x0): relocation R_RISCV_JAL out of range: 538708046 is not in [-524288, 524287]
split machine_esp32c3.go into module related files.
aykevl
reviewed
Oct 10, 2021
There was a problem hiding this comment.
This is a very large PR. I haven't gone through all of it yet.
Can you perhaps make it smaller, so that it is easier to review and maintain?
- This PR implements both pin interrupts and UART. I would prefer it it only implemented one of them (you can always make a PR with the other one later).
- This PR implements nested interrupts, but do we really need those? I would suggest simple non-nested interrupts (like in the fe310 chip) for now, to keep things easy to understand.
- More generally, it looks like you've copied a lot of code from ESP-IDF. Please make sure you understand what every piece of it is doing, and if you don't understand it, remove it. For example, you are passing two parameters to
handleInterruptfrom esp32c3_intr.S but don't actually use those parameters. - Why do you move code from machine_esp32c3.go to new files? It's done for some other chips, but I honestly don't think it improves readability (it will make the list of files in the machine package very long). And in this case, it makes the diff bigger and thus the code becomes harder to review.
Comment on lines
+107
to
+108
| .rept 5 | ||
| j _interrupt_handler /* 6 identical entries, all pointing to the interrupt handler */ |
There was a problem hiding this comment.
Are there 5 or 6 identical entries? The comment and .rept statement disagree.
| addi t0, sp, CONTEXT_SIZE /* restore sp with the value when trap happened */ | ||
| sw t0, 34*REGSIZE(sp) | ||
|
|
||
| /* Call handleException(sp) or handleException(sp) |
There was a problem hiding this comment.
handleException or handleException? This doesn't look right.
Comment on lines
+98
to
+100
| for { | ||
| riscv.Asm("wfi") | ||
| } |
Comment on lines
+182
to
+191
| /* Increase interrupt threshold level */ | ||
| li t2, 0x7fffffff | ||
| and t1, s1, t2 /* t1 = mcause & mask */ | ||
| slli t1, t1, 2 /* t1 = mcause * 4 */ | ||
| li t2, CPU_INT_PRI_0_REG | ||
| add t1, t2, t1 /* t1 = INTC_INT_PRIO_REG + 4 * mcause */ | ||
| lw t2, 0(t1) /* t2 = INTC_INT_PRIO_REG[mcause] */ | ||
| addi t2, t2, 1 /* t2 = t2 +1 */ | ||
| sw t2, 0(t0) /* CPU_INT_THRESH = t2 */ | ||
| fence |
There was a problem hiding this comment.
You are implementing nested interrupts here. Is this intentional? Did you test nested interrupts?
Comment on lines
+10
to
+14
| uartConfig := &machine.UARTConfig{ | ||
| BaudRate: 115200, | ||
| TX: machine.Pin(2), | ||
| RX: machine.Pin(5), | ||
| } |
There was a problem hiding this comment.
Why does this need a new example?
| @@ -14,131 +13,92 @@ func CPUFrequency() uint32 { | |||
| return 160e6 // 160MHz | |||
| } | |||
|
|
|||
| type Module int | |||
There was a problem hiding this comment.
What's a module? It seems like this is something internal to the ESP32-C3, and therefore should not be part of the public API.
|
@zdima |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added and integrated interrupts, GPIO and UART.
The test examples are:
src/examples/esp/esp32c3/pininterrupt.go
src/examples/esp/esp32c3/echo.go
The signal vector remain in .text.
Moving it to the .init or even to the bottom of the .text segment breaks the signals functionality.