Initial embassy support #225

MabezDev · 2022-10-24T21:58:00Z

The following features are now available in esp-hal-common:

async, enables eh1, eha, and embassy-sync
embassy, enables embassy-time and the base time driver implementation
embassy-time-systick & embassy-time-timg features add embassy driver support for either the systick or timg peripheral
- Depending on the chip these features are also available on the chip specific hal

This PR also enables atomic emulation for the esp32s2 by default, the examples have been updated. I've also implemented Deref for the Timer wrapping structs to allow calling the underlying functions without boilerplate.

Blocked on

Xtensa targets won't build because we need 1.65 release for GAT support in embedded-hal-async

Unresolved issues

LoadStore exception with systimer timer on s2 - probably related to atomic emulation? - Moved to [ESP32-S2] LoadStore exception with systimer timer on embassy_hello_world example #253
git deps for embassy will stop us publishing new versions
CI needs to be reworked to test these new examples, we'll need nightly and the ability to supply some different features

Closes #231
Closes #232

bjoernQ · 2022-10-25T06:45:04Z

Awesome! Just out of curiosity: do you know of a roadmap when those git dependencies will get published on crates.io?

jessebraham · 2022-10-28T15:22:40Z

embassy-time and embassy-sync have both been published on crates.io.

esp-hal-common/Cargo.toml

jessebraham · 2022-11-01T13:27:38Z

I've rebased this on main to pull in my previous CI changes, so that I can take care of the Ci changes needed here.

MabezDev · 2022-11-07T14:10:02Z

This is now ready for review! Still looking into the S2 issue, but I don't think it's related to the implementation here. It's either a bug in the atomic emulation crate or a codegen issue.

jessebraham

Left a handful of comments, but overall it's looking quite good to me. Thanks again for your work on this!

I'm not super excited about the changes required for the ESP32-S2, though that's not your fault at all. I hope we can find a more elegant solution in the future.

esp-hal-common/src/embassy.rs

jessebraham · 2022-11-07T16:55:19Z

esp-hal-common/src/embassy.rs

+#[cfg_attr(
+    all(
+        feature = "embassy-time-timg",
+        any(feature = "esp32", feature = "esp32s3", feature = "esp32s2")


Is there a reason this isn't supported for all chips? We should state that somewhere if there is, I think.

Because the timg on the c3 (and I think c2) only has one configurable alarm, so the systick should be used instead.

Okay, that's fine for now then. I do think we either need to document this or define a new configuration symbol in our build script just because it's not immediately clear what's going on by looking at the code. But this can be done later, won't block the PR on it.

esp-hal-common/src/embassy.rs

esp-hal-common/src/embassy/time_driver_systimer.rs

esp-hal-common/src/embassy/time_driver_timg.rs

esp32-hal/examples/embassy_hello_world.rs

bjoernQ · 2022-11-08T08:21:17Z

Great job! Jesse already did a careful code-review so not too much from my side

yes, the boilerplate we now need for the ESP32-S2 examples are annoying - hopefully and most likely we can come up with something more elegant in the future
also, in future we need to think about how to get this together with esp-wifi since it's currently very opinionated regarding the use of the timers
for the embassy-example we should list all the needed features so users get the right error message (e.g. required-features = ["embassy", "embassy-time-systick"])

Seems like the build failures are just network issues?

MabezDev · 2022-11-08T11:21:42Z

for the embassy-example we should list all the needed features so users get the right error message (e.g. required-features = ["embassy", "embassy-time-systick"])

I don't think we can do this because the time driver is a choice for some chips, like the s2 & s3. Maybe it's possible to improve the error message if a time driver is not selected.

bjoernQ · 2022-11-08T11:26:38Z

for the embassy-example we should list all the needed features so users get the right error message (e.g. required-features = ["embassy", "embassy-time-systick"])

I don't think we can do this because the time driver is a choice for some chips, like the s2 & s3. Maybe it's possible to improve the error message if a time driver is not selected.

Ah I thought we could make a choice for the user 😄 in general it would be great if every user can get a sample to run at the second attempt (i.e. after learning some feature needs to be specified)

MabezDev · 2022-11-08T14:31:05Z

Ah I thought we could make a choice for the user smile in general it would be great if every user can get a sample to run at the second attempt (i.e. after learning some feature needs to be specified)

I suppose we could but it then makes testing CI harder.

also, in future we need to think about how to get this together with esp-wifi since it's currently very opinionated regarding the use of the timers

I think I will do a follow up PR soon which should address this, but for now I want to get the baseline embassy support in so we can start messing around with some drivers :)

- read_raw on timg renamed to now() - timg initialized and stored in static for use in the embassy driver - timg sets alarm value - untested whether alarms actually trigger

- Adds the timg timer block as a time driver for embassy - Not enabled on the C3 as it only has one timer block, better to use systimer - s2 example added but can't build due to atomic requirements in futures-core

…r backtracing

- Fix intr prio array being off by one - emabssy time prio interrupt set to max prio - use cfg instead of feature for systick detection

jessebraham

Tested and verified for all chips, so I think we're good to go! Thanks again for your work on this, very excited to move forward with async support!

* wip: timg embassy driver - read_raw on timg renamed to now() - timg initialized and stored in static for use in the embassy driver - timg sets alarm value - untested whether alarms actually trigger * TIMG timer driver for esp32, esp32s3 - Adds the timg timer block as a time driver for embassy - Not enabled on the C3 as it only has one timer block, better to use systimer - s2 example added but can't build due to atomic requirements in futures-core * Add S2 atomic support with emulation, fixup embassy support for the S2 * Move executor & static-cell to dev deps. Make eha optional * Add c2 support, run fmt * Update to crates.io embassy releases * Update eha * update timg time driver to new trait * Remove exception feature of esp-backtrace and use the user handler for backtracing * Add async testing workflow * Update systick example * Fix S2 examples * Update xtensa-toolchain * set rustflags for s2 target * Disable systick for esp32s2 until we can fix the noted issues * review improvements - Fix intr prio array being off by one - emabssy time prio interrupt set to max prio - use cfg instead of feature for systick detection * Update example time delays

MabezDev marked this pull request as draft October 24, 2022 22:19

MabezDev force-pushed the feature/embassy-initial branch from 726c6b9 to 1b09b1c Compare October 25, 2022 10:25

jessebraham reviewed Oct 28, 2022

View reviewed changes

esp-hal-common/Cargo.toml Outdated Show resolved Hide resolved

jessebraham force-pushed the feature/embassy-initial branch from 59d7306 to 17aa6a6 Compare November 1, 2022 13:27

MabezDev force-pushed the feature/embassy-initial branch 7 times, most recently from ca925e4 to 9a0d099 Compare November 7, 2022 14:08

MabezDev marked this pull request as ready for review November 7, 2022 14:08

MabezDev requested review from bjoernQ and jessebraham November 7, 2022 14:09

MabezDev mentioned this pull request Nov 7, 2022

[ESP32-S2] LoadStore exception with systimer timer on embassy_hello_world example #253

Closed

MabezDev force-pushed the feature/embassy-initial branch from 03809f2 to 2c6b9af Compare November 7, 2022 16:53

jessebraham reviewed Nov 7, 2022

View reviewed changes

MabezDev force-pushed the feature/embassy-initial branch from c2f14d8 to 77c847c Compare November 8, 2022 11:21

MabezDev force-pushed the feature/embassy-initial branch from 77c847c to 4ea4316 Compare November 8, 2022 14:32

MabezDev requested a review from jessebraham November 9, 2022 13:54

MabezDev added 2 commits November 9, 2022 15:39

wip: timg embassy driver

b0d1cf6

- read_raw on timg renamed to now() - timg initialized and stored in static for use in the embassy driver - timg sets alarm value - untested whether alarms actually trigger

TIMG timer driver for esp32, esp32s3

9533db0

- Adds the timg timer block as a time driver for embassy - Not enabled on the C3 as it only has one timer block, better to use systimer - s2 example added but can't build due to atomic requirements in futures-core

MabezDev added 15 commits November 9, 2022 15:39

Add S2 atomic support with emulation, fixup embassy support for the S2

0f3f979

Move executor & static-cell to dev deps. Make eha optional

cd2be9e

Add c2 support, run fmt

ae50255

Update to crates.io embassy releases

efc9d87

Update eha

a4de1dc

update timg time driver to new trait

816370c

Remove exception feature of esp-backtrace and use the user handler fo…

574918b

…r backtracing

Add async testing workflow

3d4eea4

Update systick example

7d352e7

Fix S2 examples

3a6f917

Update xtensa-toolchain

7cfc4c0

set rustflags for s2 target

b64e2cb

Disable systick for esp32s2 until we can fix the noted issues

3a411b5

review improvements

5669a38

- Fix intr prio array being off by one - emabssy time prio interrupt set to max prio - use cfg instead of feature for systick detection

Update example time delays

39d78c3

MabezDev force-pushed the feature/embassy-initial branch from 4ea4316 to 39d78c3 Compare November 9, 2022 15:39

jessebraham approved these changes Nov 9, 2022

View reviewed changes

jessebraham merged commit 9064177 into main Nov 9, 2022

MabezDev deleted the feature/embassy-initial branch November 9, 2022 21:58

brainstorm mentioned this pull request Nov 9, 2022

Preliminary I2S Support #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial embassy support #225

Initial embassy support #225

MabezDev commented Oct 24, 2022 •

edited

Loading

bjoernQ commented Oct 25, 2022

jessebraham commented Oct 28, 2022

jessebraham commented Nov 1, 2022

MabezDev commented Nov 7, 2022

jessebraham left a comment

jessebraham Nov 7, 2022

MabezDev Nov 8, 2022

jessebraham Nov 9, 2022

bjoernQ commented Nov 8, 2022

MabezDev commented Nov 8, 2022

bjoernQ commented Nov 8, 2022

MabezDev commented Nov 8, 2022

jessebraham left a comment

Initial embassy support #225

Initial embassy support #225

Conversation

MabezDev commented Oct 24, 2022 • edited Loading

Blocked on

Unresolved issues

bjoernQ commented Oct 25, 2022

jessebraham commented Oct 28, 2022

jessebraham commented Nov 1, 2022

MabezDev commented Nov 7, 2022

jessebraham left a comment

Choose a reason for hiding this comment

jessebraham Nov 7, 2022

Choose a reason for hiding this comment

MabezDev Nov 8, 2022

Choose a reason for hiding this comment

jessebraham Nov 9, 2022

Choose a reason for hiding this comment

bjoernQ commented Nov 8, 2022

MabezDev commented Nov 8, 2022

bjoernQ commented Nov 8, 2022

MabezDev commented Nov 8, 2022

jessebraham left a comment

Choose a reason for hiding this comment

MabezDev commented Oct 24, 2022 •

edited

Loading