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

Ensure the PAC crates are never exposed publicly #2525

Open
MabezDev opened this issue Nov 13, 2024 · 8 comments
Open

Ensure the PAC crates are never exposed publicly #2525

MabezDev opened this issue Nov 13, 2024 · 8 comments

Comments

@MabezDev
Copy link
Member

MabezDev commented Nov 13, 2024

The PACs will probably never be 1.0, therefore we need to keep it out of the public API in esp-hal.

This is currently exposed in a few places

  • interrupt::enable* takes pac::Interrupt
  • Some Instance traits expose * const peripheral::RegisterBlock
  • esp_hal::peripherals::* implement Deref, which deref to the pac types
@bugadani
Copy link
Contributor

bugadani commented Nov 13, 2024

Some Instance traits expose * const peripheral::RegisterBlock

This is annoying because even if we newtyped the RegisterBlock, if third parties try to write a driver using the same Instance (for example an SCCB driver over I2C), they would need access to the PAC.

I guess if we can live with multiple PACs in the same project, we can "just" expose the address, and make the user convert it to "a" PAC peripheral, but I'm worried this might cause problems.

@MabezDev
Copy link
Member Author

I guess if we can live with multiple PACs in the same project, we can "just" expose the address, and make the user convert it to "a" PAC peripheral, but I'm worried this might cause problems.

I think upstream rust would have a stroke at this comment, this completely eliminates pointer provenance :D.

I think this is a use-case we want to support though, SCCB is the perfect example tbh.

@bugadani
Copy link
Contributor

I think upstream rust would have a stroke at this comment, this completely eliminates pointer provenance :D.

svd2rust does that already, we're casting a random number to a typed pointer in the PACs.

@MabezDev
Copy link
Member Author

svd2rust does that already, we're casting a random number to a typed pointer in the PACs.

True, it's no different. I guess it just feels less bad when machine-generated code is doing it 😅.

@Dominaezzz
Copy link
Collaborator

Maybe just mark the PAC exposing APIs as unstable indefinitely?

I feel like the moment one needs to write a 3rd party driver, they need to touch the PAC, period.
Obviously hal drivers should prevent the need for 3rd party drivers were possible and expose sensible customization points were practical, but ultimately it's hard to predict everything users may ever want to customize.
(Perhaps we need a good definition of what a 3rd party driver is, as I see SCCB to be a wrapper rather than a 3rd party driver.)
There should probably be some guidelines around how 3rd party drivers interact with the hal.

Adding to the list of exposed PAC is https://github.com/esp-rs/esp-hal/blob/main/esp-hal/src/pcnt/channel.rs#L12 . (My IDE never seems to do the right thing with this, so I'll be happy to see it refactored)

@jessebraham
Copy link
Member

I think this issue needs to be expanded, as this is not specific only to the PACs. We additionally re-export riscv/esp-riscv-rt and xtensa-lx/xtensa-lx-rt as well, none of which are stable releases. There may be additional packages for which this is the case as well, will need to do some further investigation.

@bugadani
Copy link
Contributor

Just because the list contains a ?: We're reexporting (almost) all peripherals because the create_peripheral! macro creates types that deref to the PAC peripherals. This means almost all register-related API is publicly exposed.

@MabezDev
Copy link
Member Author

I think this issue needs to be expanded, as this is not specific only to the PACs. We additionally re-export riscv/esp-riscv-rt and xtensa-lx/xtensa-lx-rt as well, none of which are stable releases. There may be additional packages for which this is the case as well, will need to do some further investigation.

I opened #2589. As far as I know, the rt crates aren't exposed in the public API (which is what this issue is tracking), but I agree there are some interactions we need to be aware of with these dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

4 participants