Skip to content

Added API to raise and reset software interrupts #426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 18, 2023
Merged

Conversation

onsdagens
Copy link
Contributor

Implemented an API for raising and resetting software interrupts as mentioned in #425 on the ESP32C2, ESP32C3, ESP32S2 and ESP32S3. Builds and works as expected on the C3 at least, not tested on any of the other chips since i don't own them.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start, thank you!

@@ -56,7 +63,50 @@ pub enum Peripheral {
#[cfg(esp32c6)]
Twai1,
}
#[cfg(not(any(esp32, esp32c6)))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Software CPU interrupts are supported on all chips, so I don't think we need the feature gates? It might the case that they are not included in the PACs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they are in the PACs but maybe named differently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i could only find them in system for the chips i included.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least we use SW_INTR3 in esp-wifi

Copy link
Contributor Author

@onsdagens onsdagens Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in the INTPRI peripheral in the C6 PAC. I don't have one for testing however, so i don't feel confident implementing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the esp32 it's in the DPORT, so i guess support there is just a matter of enabling it

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 8, 2023

Awesome! Thank you!

It would be great if you could add a very simple example (at least for the C3 since you cannot verify it on the other chips).
If you don't want to add an example that would be also fine - can be done by someone else.

Also, implementation for ESP32 and ESP32-C6 will be added by someone else later

@jessebraham
Copy link
Member

jessebraham commented Mar 8, 2023

Also, implementation for ESP32 and ESP32-C6 will be added by someone else later

I'd rather include it in this PR honestly, though I won't block merging on that. These things have a tendency of being forgotten.

If there are issues with the PAC please let me know exactly what they are and we can get them resolved.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 9, 2023

I tested it on the supported targes (i.e. not ESP32, not ESP32-C6) and it works fine

For ESP32 we need the interrupts in the SVD/PAC first
For ESP32-C2 we should align the naming of the interrupts (currently ETS_FROM_CPU_INTR0 ...)
For ESP32-C6 we should align the naming of the interrupts (currently CPU_FROM_CPU_0 ...)

I'd suggest we can merge this as is, wait for the SVD/PAC changes (there are already changes planned so it makes sense to do it all together) and then add ESP32 and ESP32-C6

@jessebraham
Copy link
Member

I will be reviewing some SVD changes made by a colleague today. I have additionally done some work in aligning the peripheral and interrupt names, but I will verify the ones mentioned by @bjoernQ before publishing new SVDs. I'm hoping to have new PACs published either this afternoon or tomorrow, however there's always a possibility this could slip to next week.

@jessebraham
Copy link
Member

jessebraham commented Mar 15, 2023

Sorry, I messed up with the last SVDs and the ESP32's CPU interrupts are still missing in the latest PACs 🙈 I will make sure that they're present in the next release.

@jessebraham
Copy link
Member

With #447 merged the required interrupts should now be available.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 27, 2023

@onsdagens Do you plan to continue on this or would you like someone to take over (especially since you said you don't have all the hardware to verify it on all targets). No rush, however

@onsdagens
Copy link
Contributor Author

@onsdagens Do you plan to continue on this or would you like someone to take over (especially since you said you don't have all the hardware to verify it on all targets). No rush, however

i can give it a go, but as you said i can't test so would appreciate some help with that

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 28, 2023

@onsdagens Do you plan to continue on this or would you like someone to take over (especially since you said you don't have all the hardware to verify it on all targets). No rush, however

i can give it a go, but as you said i can't test so would appreciate some help with that

Awesome! Thanks for your effort. If there is something you need to get tested just ping me

@onsdagens
Copy link
Contributor Author

oopsie didn't mean to do that, just finished some other things so i'll be working on getting this up and running next.

@onsdagens
Copy link
Contributor Author

onsdagens commented Apr 14, 2023

Merged with latest version, added support for all of the MCUs and added some examples. Keep in mind that the examples are not tested for anything but the C3, i know they compile for the others but that's as far as it goes, could use some help with testing @bjoernQ .

@onsdagens onsdagens reopened this Apr 14, 2023
@onsdagens
Copy link
Contributor Author

Not sure what the issue is with CI here.

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 17, 2023

Problem with CI is that the latest nightlies refuse to compile embedded-hal-async - I will create a PR to pin the nightly compiler version in CI for now (until embedded-hal-async got fixed) which should make CI happy again

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 17, 2023

Rebasing should fix the CI problem (hopefully)

@onsdagens
Copy link
Contributor Author

Rebasing should fix the CI problem (hopefully)

Thanks! Now there seems to be a dependency issue, embassy-executor does not want to build. I'm unsure why it's pinned to that specific revision, so i don't know how to proceed here without breaking something else.

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 17, 2023

Rebasing should fix the CI problem (hopefully)

Thanks! Now there seems to be a dependency issue, embassy-executor does not want to build. I'm unsure why it's pinned to that specific revision, so i don't know how to proceed here without breaking something else.

Seems it was just a temporary connectivity problem - rerunning the failed jobs cured it

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 18, 2023

Tested all the examples on all targets! 👍 Works fine!

Only thing I don't agree with is the top-level comment in the examples 😆
If that is changed this can be merged

@onsdagens
Copy link
Contributor Author

Haha, oops! Fixed the comments now.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bjoernQ
Copy link
Contributor

bjoernQ commented Apr 18, 2023

Oh rustfmt isn't happy, yet

@onsdagens
Copy link
Contributor Author

Sorry, didn't expect comments to be misformatted... live and learn.

@bjoernQ bjoernQ merged commit 3fa71b2 into esp-rs:main Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants