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

Mark unstable modules, make macros private #2900

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

playfulFence
Copy link
Contributor

@playfulFence playfulFence commented Jan 7, 2025

Description

closes #2499
potentially closes #2921

Marked the rest of unstable modules.
Makes macros module private (otherwise, it'll cause a huge and noisy diff like crate::before_snippet! -> crate::macros::before_snippet and so on) as it was public only to reexport a couple of things from procmacros.

Adding small "default" branch in esp_hal::init function (caused by instability of watchdog and psram items of Config)

Since rom is also unstable, we need that bad-looking pub use construction for regi2c_write and regi2c_write_mask, which will be removed when rom is stabilized

@playfulFence playfulFence changed the title Mark unstable modules Mark unstable modules, make macros private Jan 7, 2025
esp-hal/src/lib.rs Outdated Show resolved Hide resolved
esp-hal/src/rom/mod.rs Outdated Show resolved Hide resolved
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.

I'll review the code separately soon, but for now I just generated the docs to take a look around.

I believe the following modules still need to be unstable:

  • efuse
  • asynch
  • lp_core

The clock changes are handled in so we don't need to worry about that one: #2899

@playfulFence
Copy link
Contributor Author

@MabezDev

I believe the following modules still need to be unstable:

  • efuse
  • asynch
  • lp_core

Oops, forgot about them. Then psram too probably 🤔

@playfulFence playfulFence added the skip-changelog No changelog modification needed label Jan 7, 2025
@playfulFence
Copy link
Contributor Author

playfulFence commented Jan 7, 2025

skip-changelog due to changes in esp-hal-embassy which don't require a changelog entry

@bugadani
Copy link
Contributor

bugadani commented Jan 7, 2025

What's the reason to make all macros private? Part of that module is public reexports, and we're now reexporting reexported macros?

@@ -234,8 +234,8 @@ impl RegisterAccess for crate::peripherals::ADC1 {
fn set_init_code(data: u16) {
let [msb, lsb] = data.to_be_bytes();

crate::regi2c_write_mask!(I2C_SAR_ADC, ADC_SAR1_INITIAL_CODE_HIGH_ADDR, msb as u32);
crate::regi2c_write_mask!(I2C_SAR_ADC, ADC_SAR1_INITIAL_CODE_LOW_ADDR, lsb as u32);
crate::rom::regi2c_write_mask!(I2C_SAR_ADC, ADC_SAR1_INITIAL_CODE_HIGH_ADDR, msb as u32);
Copy link
Contributor

Choose a reason for hiding this comment

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

This macro should probably live in an internal_analog or similar module, rom seems somewhat random to me - it's expanding into a rom function, sure, but if that would be the driving force behind code organization we could just have a big rust.rs file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this - should it get fixed in this PR or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm also not sure 😄

@playfulFence
Copy link
Contributor Author

What's the reason to make all macros private? Part of that module is public reexports, and we're now reexporting reexported macros?

After a long fight me and @bugadani decided to leave things as they are because another options (pub unstable macros module) don't seem to be working (also, macros are cursed)

@playfulFence playfulFence marked this pull request as ready for review January 8, 2025 11:59
@@ -156,28 +154,28 @@ pub use xtensa_lx_rt::{self, entry};
#[cfg(any(esp32, esp32s3))]
pub use self::soc::cpu_control;
#[cfg(efuse)]
#[instability::unstable]
#[cfg_attr(not(feature = "unstable"), allow(unused))]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably something that can be fixed in instability.

@playfulFence
Copy link
Contributor Author

playfulFence commented Jan 9, 2025

UPD: marked entry, handler and ram as unstable only for docs since "truly" marking them as "unstable" leads to a huge amount of errors, not sure what to do with it. It must be used, but esp_riscv_rt nor xtensa_lx_rt, nor procmacros not gonna be 1.0.

Also, it doesn't make sense to stabilize procmacros without stable interrupts.

UPD:
Just

#[instability::unstable]
#[cfg_attr(not(feature = "unstable"), allow(unused))]
pub use procmacros::{handler, ram};

seems to be resolving the issue with procmacros

@bugadani
Copy link
Contributor

bugadani commented Jan 9, 2025

entry quite obviously needs to be stable - if you can't write main, how would you use the crate?

@playfulFence
Copy link
Contributor Author

playfulFence commented Jan 9, 2025

Exactly, that's what I'm talking about 😅

Which means, we need to either

  1. stabilize esp_riscv_rt and xtensa_lx_rt too
  2. make users export it separately

@playfulFence playfulFence marked this pull request as draft January 9, 2025 14:02
@MabezDev
Copy link
Member

MabezDev commented Jan 9, 2025

esp-hal-procmacros is basically tied to esp-hal, the only reason they're in separate a crate is due to compiler limitations, so we'll actually need to make esp-hal-procmacros 1.0 too. I hadn't considered this initially, but this should be fine though, because everything that's in there can also be marked as unstable, and we can do the following:

  • doc(hide) the rt crates
    • Optional: we may want to create a doc hidden __macro_impl_details module to consolidate the hidden things that macros might need to touch.
  • A new main macro that uses the hidden rt crates, basically #[main] will simply expand to #[arch_rt::entry]
    • because users don't interact with code generated by these macros, we can use them as long as we don't change the behavior of them.
  • Add some diagnostics to tell the user to use #[esp_hal_embassy::main] if they try and write an async main.

I suggest we do this in a follow up PR though, as this will involve a bunch more changes. I'll open an issue for this.

@playfulFence playfulFence marked this pull request as ready for review January 9, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark procmacros as unstable Mark unstable APIs
4 participants