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

Unstabilize CPU-related functionality, merge modules #3099

Merged
merged 13 commits into from
Feb 7, 2025

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Feb 5, 2025

Multi-core operations, as well as CPU reset are no longer stable. CPU-related operations are now contained in the cpu module (as well as system reset, which may be slightly awkward but keeping that alone in a separate reset module seemed equally weird).

This PR also fixed CPU reset. In esp-idf, the rom function is defined as void esp_rom_software_reset_cpu(int cpu_no); and we were missing the cpu_no parameter. Because of this change, the function is no longer marked as stable.

I think these were the last bits in esp-hal that weren't supposed to be stable.

@bugadani bugadani force-pushed the cpu branch 4 times, most recently from fb66996 to aa24ba5 Compare February 5, 2025 12:41
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.

Sorry to bikeshed, I feel like this would be better placed in a system or soc module.

Which also brought my attention to https://docs.esp-rs.org/esp-hal/esp-hal/0.23.1/esp32s3/esp_hal/system/index.html which probably should be hidden completely I think?

esp-hal/src/cpu.rs Outdated Show resolved Hide resolved
@bugadani
Copy link
Contributor Author

bugadani commented Feb 5, 2025

Good shout, although that system module is equally pointless IMHO Radio clocks belong to maybe clock, RadioPeripherals(?) and PeripheralIter belongs to peripherals.

@MabezDev
Copy link
Member

MabezDev commented Feb 5, 2025

Good shout, although that system module is equally pointless IMHO Radio clocks belong to maybe clock, RadioPeripherals(?) and PeripheralIter belongs to peripherals.

Yep, I agree with all those points!

esp-hal/src/cpu.rs Outdated Show resolved Hide resolved
@bugadani bugadani force-pushed the cpu branch 2 times, most recently from 6ca612a to 1813d51 Compare February 5, 2025 12:59
@bugadani bugadani marked this pull request as draft February 5, 2025 13:09
@bugadani bugadani added the skip-changelog No changelog modification needed label Feb 5, 2025
@bugadani bugadani force-pushed the cpu branch 4 times, most recently from a4954df to d798995 Compare February 6, 2025 14:32
@bugadani
Copy link
Contributor Author

bugadani commented Feb 6, 2025

I moved everything into system, and removed the EnumIter thing that was a perma-stable-public implementation of the doc(hidden) Peripheral enum.

The Peripheral enum can and should probably be generated based on data in the soc::peripherals module, but for now it should be fine as it is.

@bugadani bugadani marked this pull request as ready for review February 6, 2025 16:38
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

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.

LGTM, just one question that I either can't tell from the diff or has some other effect I'm not considering

esp-hal/src/system.rs Show resolved Hide resolved
@MabezDev MabezDev added this pull request to the merge queue Feb 7, 2025
Merged via the queue into esp-rs:main with commit 392d5cc Feb 7, 2025
27 checks passed
@bugadani bugadani deleted the cpu branch February 7, 2025 15:01
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.

3 participants