Extract embassy support into esp-hal-embassy and esp-hal-embassy-procmacros packages

[jessebraham]

Opening this as a draft as I still need to do some more review, probably more cleanup, and some more testing. However, I'd like to get some eyes on it as it should be close to ready.

This PR decouples Embassy support from the esp-hal-common package. I've additionally created an additional procmacro package, esp-hal-embassy-procmacros, for Embassy-specific needs.

Some notable changes:

• All features have had the embassy- prefix removed, as it's now redundant
• I have renamed time-systick to time-systimer to better reflect which peripheral is being used

I will add review comments for anything else that may need attention, as I identify these parts.

CC @MabezDev @bjoernQ @bugadani

[bugadani]

Is there a valid configuration where neither of these are active?

[jessebraham]

Nope, just forgot to add the build script to verify the features, and that was temporarily added to silence some warnings. Thanks for the reminder, will get that fixed tomorrow.

[bjoernQ]

I have renamed time-systick to time-systimer to better reflect which peripheral is being used

Oh no ... took me a year to learn it's not time-systimer ... 😆

[bugadani]

So what's the current state wrt. systimer, the async drivers and interrupt handlers? Is it possible the users end up with a duplicate interrupt handler error? Can we even avoid that with the crate split?

For me it's a bit weird that as a user, I'll have to import embassy-executor for two types and a macro, as well as esp-hal-embassy for the chip support and esp-hal as well. It wasn't super clear what features needed to be set where and I think this change makes it worse. We either need to somehow provide everything (Spawners and the task macro) without the embassy-executor dependency (which is not super annoying IMO because of the arena size configuration), or we have to make this absolutely super clear in documentation which people won't read anyway if they use esp-template 🤦‍♂️

[MabezDev]

Completely off-topic but maybe we need a esp-metadata crate which contains the tomls etc from the soc module. This would allow crates other than esp-hal to use these nice cfgs!

[MabezDev]

Hmm this didn't turn out quite like I had in my head, I'm not sure depending on two crates is better than one. I guess there is no way for esp-hal-embassy to not depend on esp-hal-common? I like the separation, but not at the cost of a user having to depend on two things. I'll think a little bit more about this.

[jessebraham]

Hmm this didn't turn out quite like I had in my head

Would you mind elaborating on this a bit? I'm really not sure how else this would look (maybe I'm just missing something obvious, though).

[MabezDev]

Sorry, that was me just rambling, my main point was that I hadn't anticipated the cyclic dependency, I was hoping we would have been able to reexport one of the crates from the other but that won't be possible. I really like the separated dependencies/features etc, I just wonder if we're pushing the maintenance burden to end user cognitive burden by having them have to be aware and depend on two different crates.

[bjoernQ]

For me it's a bit weird that as a user, I'll have to import embassy-executor for two types and a macro, as well as esp-hal-embassy for the chip support and esp-hal as well.

We could re-export those from esp-hal-embassy since it already depends on embassy-executor?

[jessebraham]

So what's the current state wrt. systimer, the async drivers and interrupt handlers? Is it possible the users end up with a duplicate interrupt handler error? Can we even avoid that with the crate split?

Sorry, forgot to reply to this. I think this is an unsolved problem still, I don't have a solution yet. I have cfg'd out the async delay implementation entirely for SYSTIMER for the time being, though. So at least that should not cause issues, perhaps I'm missing some other instances where this could be problematic though. At the very least, I don't think this split leaves us in any worse of a place than we currently are (though please let me know if I am wrong about this).

For me it's a bit weird that as a user, I'll have to import embassy-executor for two types and a macro, as well as esp-hal-embassy for the chip support and esp-hal as well. It wasn't super clear what features needed to be set where and I think this change makes it worse. We either need to somehow provide everything (Spawners and the task macro) without the embassy-executor dependency (which is not super annoying IMO because of the arena size configuration), or we have to make this absolutely super clear in documentation which people won't read anyway if they use esp-template 🤦‍♂️

As suggested by @bjoernQ perhaps we could re-export the relevant embassy-executor bits?

[jessebraham]

I really like the separated dependencies/features etc, I just wonder if we're pushing the maintenance burden to end user cognitive burden by having them have to be aware and depend on two different crates.

I'm not sure how much of a problem this really will be in practice. The HAL will be required regardless, and then if people want to use Embassy alongside it they would ideally just require one additional package, esp-hal-embassy (assuming we handle re-exports as mentioned above). But again, please let me know if I'm missing something here.

[bugadani]

perhaps we could re-export the relevant embassy-executor

My main worry is #[task] which I believe generates code that references embassy-executor directly (via static POOL: ::embassy_executor::raw::TaskPool...).

[jessebraham]

I still need to add the build script and do some assorted cleanup, but I've rebased this branch at least. Will do the rest next week.