RFC: Unify the HAL

[jessebraham]

From its inception, the esp-hal repository has contained separate packages for each supported device. The thought early on was that there would be enough device-specific code to justify putting it in these packages rather than esp-hal-common. As time went on and additional devices were supported, it has been increasingly clear that this is not really the case.

Thanks to some recent refactoring, there is actually very little in each of these device-specific HAL packages now. It wouldn't require a lot of work to unify all of these packages into simply the esp-hal package. This additionall reduces duplicated code in the form of linker scripts. Making these changes (and as the repo stands now) this would leave us with the esp-hal, esp-hal-procmacros, and esp-hal-smartled packages in this repository.

This approach greatly simplifies the release process, and may make maintaining this repository easier overall. This would, unfortunately, put us in a position where the esp-hal package has a large number of features, not all of which play nicely together. I'm sure this could be solved with documentation and the build script with little work, though.

I would like to begin some conversation regarding these changes, and am mostly interested if there's any reason not to make these changes at this point.

---

It's time to begin the unification:

• Refactor peripheral drivers as needed to improve consistency across devices
  • Refactor the analog module #1100
  • Refactor DMA to make API more consistent across devices #1125
• Rename the esp-hal-common package to esp-hal
  • Unify: rename the esp-hal-common package to esp-hal [0/?] #1131
• Eliminate the chip-specific HAL packages, add a new examples package
  • Add the xtask package, implement automation for building examples #1157
  • Add an xtask subcommand for building documentation #1160
  • Unify: Remove the chip-specific HAL packages, adapt esp-hal for direct use [1/?] #1196
• Update documentation as needed, come up with a solution for deploying
  • Add a workflow to build and deploy esp-hal documentation to GitHub Pages #1248
  • Documentation of esp-hal once unified #1205
  • Update README to match with unification changes #1217
  • Migration guide for updating to esp-hal@0.16.x #1222
  • Update esp-hal/README #1228

[bjoernQ]

While this clearly makes maintenance easier I think it will also make it easier to use the HAL.

All the support crates follow a model of having features to select the target (e.g. esp-println) - so having just one package for the HAL should feel familiar and consistent.

[jessebraham]

I guess one major downside to this approach is that the documentation can only be built for a single chip. This is unfortunate, but we're already building and hosting docs for esp-idf-* anyway so not the end of the world to do the same for this project.

[sethp]

I hope this is an open request for comment, as I can offer a little bit of a counterpoint: you mentioned code deduplication as a goal, but as I'm using the esp32c3-hal, I find myself wishing more of the code were duplicated and less were in esp-hal-common. Allow me to explain: it seems to me that a single unified codebase puts all of the chip-specific code together: the esp32c3 bits are right next to / mixed up in the esp32c6 bits, etc.

I do think balance of that choice shifts from my perspective. I've been working a lot with interrupts, recently, and I've found myself wishing that there were an esp32c3-hal/src/interrupts.rs file which had no feature flags at all, but just had the entire flow from the default_setup_interrupts initializer and _start_trap entry point (both which currently live in esp-riscv-rt) through the entire vectored flow (flattened, as there would be no need to consider a non-vectored flow).

I'm also a bit lost when it comes to modifying the hal: my hope is to work specifically with the chip and problem I have at hand and generalize later, but it's not always clear to me how to do that. I typically don't know much about the other chips, and I've yet to find a way to make rust-analyzer give me a clear "single-chip view" of the esp-hal when I've got it open as a workspace. Both of those feel like they raise the barrier to entry a bit.

You also mentioned release process, and for us that follows more of a git merge/integration flow than it does issuing a new version on crates.io. We've found that novel variations on a peripheral (in our case, quad SPI) more or less require working from a fork. So far that's been relatively easy to navigate, as we've been able to mostly ignore the existence of the other chips for our local context, which I fear would get harder in a unified package.

Further, I'm not entirely sure how we'd manage to adopt the unification change: I don't see a way we could incrementally adopt it one "bite" at a time, right? I expect we'd have to resolve the merge conflicts in our fork and then probably modify all of our imports "all at once." Perhaps, would you consider publishing an upgrade guide and/or tooling for migrating our code?

Also, I don't much care for it, but if you intend to follow SemVer it would have a unified esp-hal package issue new major versions when any of the APIs for any of the chips changed, which might be something worth considering.

So, to sum up, you had asked:

if there's any reason not to make these changes at this point

I posit that the change makes maintenance a bit easier at the cost of making developing with the hal a little harder. If your goal is to optimize the total level of effort in the ecosystem, then I think it'd be useful both to preserve both the possibility of duplicating code across a "central" vs "chip-specific" boundary.

Finally, I'll suggest that it acts against the "release" goal of "getting the new functionality into the hands of downstream consumers as quickly as possible" by adding friction into the uptake flow for downstream consumers like myself.

[bjoernQ]

Thanks for your insights @sethp - I can only speak for myself but getting feedback from everyone is definitely a thing I want

Not really related to this RFC but anyways worth to mention

• the interrupt code should now be a bit more detangled - especially there are now two modules which should separate the RV chips without PLIC and those with PLIC - I totally agree that the initial code adding support for C6 wasn't ideal
• rust-analyzer config: I'm also often struggling with it - what seems to work for me is configuring rust-analyzer.cargo.features in .vscode/settings.json but the important thing probably is, that we should document things like that (probably some README for people not just using the HAL but working on the HAL code itself)

We also want to / started to move SOC specific code to the soc module but there is probably more we can do

[Ben-PH]

I'm not so clear on what work/changes this RFC is implying, and what changes we might see at the crate-user level. For the benefit of myself, and anyone else not yet familiar enough with the current architecture, could we please get a sanity-check that I have a correct (enough?) understanding?

Current:

• The vast majority of implementation detail is contained within esp-hal-common. Where device-specific differences exist, #[cfg(.... is used.
• esp-hal-common exists in an unconfigured state on it's own. It becomes configured during the process of being imported by a platform-specific hal in the workspace
• there is a platform-specific crate in the workspace, which essentially acts as a proxy/configurator for esp-hal-common

...and @jessebraham am I on the right page about the sort of thing you had in mind at time of writing the PR?

Proposed (illustrative example)

• the project workspace only needs (currently-named) esp-hal-common to cover all platforms.
• all implementation details that currently exist in workspaces esp32{VARIANT}-hal gets moved to feature-gated modules inside esp-hal-common
• compiling esp-hal-common defaults to {SOME_SANE_CONFIG} for the purpose of docs.rs, cargo doc --open, file-exploration, etc.

Is this a fair representation of what's being proposed?

I have a couple of thoughts on the matter, but I don't charge ahead on misguided assumptions.

[MabezDev]

Now we are starting to work on the ULP/RISC-V co-processing core support, how would support for those work if we unified the hal? As it stands I think it would be pretty easy to add support for the ULP as a separate hal, because most of the work is done in the PACs (address aliasing etc).

How would it work if we unified it, and put the ULP support behind a feature in esp-hal-common? Currently, esp-hal-common can only have one feature activated, what if a user tried to enable the ulp hal and the main core hal? I think it might break things.

[bjoernQ]

I haven't really thought about this in depth and initially I was in belief at least the ULP things might get their own HAL independent of esp-hal-common since the RTC peripherals seem to be different enough plus the runtime is different and I'm also not confident it's easy to squeeze code, stack and data in 8k. On C6 things are a bit more relaxed - so for that it probably makes sense to try get it in esp-hal-common

In general, I think one would create a separate binary crate for whatever should run on ULP/LP core (like how esp-idf does it) and the main core will load the code into RTC/LP RAM and execute it. I have ideas how this could be even done in a way more convenient than how esp-idf does - but still I haven't started on that, yet

[MabezDev]

More thoughts(I'm just brain dumping in this thread at this point 😆), I think we'd need to unify the naming of DPORT/SYSTEM/PCR or find a different way to abstract this behaviour.