Skip to content
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

Extract embassy support into esp-hal-embassy package #1595

Merged
merged 8 commits into from
Jun 3, 2024

Conversation

jessebraham
Copy link
Member

Let's try this again 😁

I've opened this PR as a draft for the time being as it still needs a README.md file for the new package, and I still need to finish documenting the new package as well. However, in the meantime, we can at least review the code changes. I will get this wrapped up on Monday.

I've done some very minimal testing and things seem to be in order, however more thorough testing is still required.

Closes #1035

@jessebraham

This comment was marked as outdated.

@@ -504,7 +519,7 @@ fn lint_packages(workspace: &Path, _args: LintPackagesArgs) -> Result<()> {
&[
"-Zbuild-std=core",
"--target=riscv32imc-unknown-none-elf",
"--features=esp32c3,wifi-default,ble,esp-now,async,embassy-net",
"--features=esp32c3,wifi-default,ble,esp-now,async,embassy-net,esp-hal-embassy/time-timg0",
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure why this was necessary, probably I'm missing something?

@jessebraham
Copy link
Member Author

Still need to finish documentation and review further, but rebased my branch and updated the defmt support. I don't know if I handled this correctly or not (just dumped in fmt.rs from esp-hal) so let me know if I need to change this.

@jessebraham jessebraham marked this pull request as ready for review May 29, 2024 15:10
![Crates.io](https://img.shields.io/crates/l/esp-hal-embassy?labelColor=1C2C2E&style=flat-square)
[![Matrix](https://img.shields.io/matrix/esp-rs:matrix.org?label=join%20matrix&labelColor=1C2C2E&color=BEC5C9&logo=matrix&style=flat-square)](https://matrix.to/#/#esp-rs:matrix.org)

[Embassy] support for `esp-hal`.
Copy link
Member Author

Choose a reason for hiding this comment

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

The README.md could definitely use some expansion, however I'm not really sure what should be included here and what belongs in documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Shall we add some links to the embassy book or to website? They are just 1 click further from the repo, but it might be useful.

@bjoernQ
Copy link
Contributor

bjoernQ commented May 29, 2024

Seems I have problem using the systimer-time-driver and running e.g. the embassy-hello-world code. Using timg0-driver works

@jessebraham
Copy link
Member Author

Looking into it

@jessebraham
Copy link
Member Author

Seems to be broken on main too, so much have happened during the timer abstraction work at some point.

@jessebraham

This comment was marked as outdated.

@jessebraham jessebraham marked this pull request as draft May 30, 2024 12:28
@jessebraham
Copy link
Member Author

jessebraham commented May 31, 2024

Okay I cannot for the life of me figure out why the "correct" fix for this isn't working. I've debugged my implementation, everything appears to be correct, but it's just plain not working.

So, I give up on that. I've included a hacky "fix", which is pretty objectively bad, but also it's not like we're not doing this all over the code base already. Ideally I'd like to fix this, however I also would like to get this merged and released so I can stop thinking about timers for awhile.

If anybody else wants to take a swing at a better fix please feel free, or if you have any suggestions I can try them out.

@jessebraham jessebraham marked this pull request as ready for review May 31, 2024 13:53
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 - Thanks!

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.

Thanks! Nice to see this split out, and a bunch of features removed from esp-hal.

The last piece of this puzzle will be docs I think, but we already have to figure that out for esp-wifi, so we can do that later.

@MabezDev MabezDev added this pull request to the merge queue Jun 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 3, 2024
@SergioGasquez SergioGasquez added this pull request to the merge queue Jun 3, 2024
Merged via the queue into esp-rs:main with commit 48e3e91 Jun 3, 2024
22 checks passed
@jessebraham jessebraham deleted the feature/esp-hal-embassy branch June 3, 2024 14:20
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.

RFC: Decouple embassy support from esp-hal
4 participants