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

Rework initialization #194

Merged
merged 9 commits into from Jun 2, 2023
Merged

Rework initialization #194

merged 9 commits into from Jun 2, 2023

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Jun 1, 2023

Closes #193

  • don't have duplicate code for the initialize function for RV and Xtensa
  • make it possible to "dynamically" initialize for BLE, Wifi or both (depending on features) at runtime
  • make it in a way that the call to initialize cannot get forgotten
  • make it possible to have EspNow AND a WiFi at the same time
  • simplifying the async feature selection

Comment on lines 119 to 123
#[cfg(any(feature = "esp32c3", feature = "esp32c2", feature = "esp32c6"))]
/// Initialize for using WiFi / BLE
/// This will initialize internals and also initialize WiFi and BLE
pub fn initialize(
systimer: Alarm<Target, 0>,
rng: hal::Rng,
radio_clocks: hal::system::RadioClockControl,
clocks: &Clocks,
) -> Result<(), InitializationError> {
init_heap();
type Timer = Alarm<Target, 0>;

#[cfg(feature = "esp32c6")]
if clocks.cpu_clock != MegahertzU32::MHz(160) {
return Err(InitializationError::WrongClockConfig);
}
#[cfg(any(feature = "esp32", feature = "esp32s3", feature = "esp32s2"))]
type Timer = hal::timer::Timer<hal::timer::Timer0<hal::peripherals::TIMG1>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we give those types a public visibility, so that if someone were to wrap initialize() inside another function, they wouldn't have to redefine those types again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean just for convenience? 🤔 Sure that shouldn't be a problem. Not sure if the naming is a bit too generic then - probably better to rename this if we make it public

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.

Overall it looks good! Just a comment about the cfgs.

I hope in the future we can move towards a more implicit initialization, i.e we track the state of which drivers have been created and initialize internally, but this is a good first step towards that.

esp-wifi/src/ble/btdm.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.

LGTM!

@bjoernQ bjoernQ merged commit 947a81c into main Jun 2, 2023
12 checks passed
@bjoernQ bjoernQ deleted the rework-initialization branch June 2, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise Initialization Code
3 participants