Skip to content

Check clippy::large_stack_frames (blocked on next release) #241

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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+28 −13
Draft

Check clippy::large_stack_frames (blocked on next release) #241

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eb463c9
Check `clippy::large_stack_frames`
bjoernQ Oct 15, 2025
c0723d6
CHANGELOG.md
bjoernQ Oct 15, 2025
f779c58
Allow bigger stack frames for Trouble
bjoernQ Oct 15, 2025
9c1f663
Allow for `main`
bjoernQ Oct 15, 2025
c39ae6a
Remove the patches
bjoernQ Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- Enable https://rust-lang.github.io/rust-clippy/master/index.html#large_stack_frames with a threshold of 1024 (#241)

### Changed

### Fixed
Expand Down
5 changes: 5 additions & 0 deletions template/.clippy.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#IF option("ble-trouble")
stack-size-threshold = 2096
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a weird number 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah - maybe 3072 would be still better than nothing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be less surprised with 2048, I don't mind the actual value just 2048 + 48 is a weird one

Copy link
Contributor

@bugadani bugadani Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What if we allow the lint on the main function, instead of guessing a value that will break anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the vanilla freshly generated project fails with "this function may allocate 2056 bytes on the stack"

I was considering to allow it instead for ble-trouble - but even 3096 would still keep people from allocating 74k stack frames 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing, but memory in main() isn't that different from static variables, except that the linker doesn't know about it. My thinking is: people can and will happily allocate buffers in main, why not? But if we have a random number in place and their solution is to disable the check globally, then we didn't end up helping. On the other hand, if we allow main to be whatever it wants to be, the user will be less inclined to just disable the check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I guess this depends on how often people overflow the stack on the main core in fn main

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

true - especially to avoid static mut

I guess we can allow it with a reason explaining they should still be careful ..... only problem right now: it seems the #[esp_rtos::main] macro doesn't preserve the attributes 🤔

then we should probably fix that first and let this sit for a while until the next HAL release

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not ideal but Trouble needs more

#ELSE
#+stack-size-threshold = 1024
#ENDIF
16 changes: 9 additions & 7 deletions template/src/bin/async_main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
reason = "mem::forget is generally not safe to do with esp_hal types, especially those \
holding buffers for the duration of a data transfer."
)]
#![deny(clippy::large_stack_frames)]

use esp_hal::clock::CpuClock;
use esp_hal::timer::timg::TimerGroup;
Expand Down Expand Up @@ -88,9 +89,10 @@ async fn main(spawner: Spawner) -> ! {
//IF option("esp32") || option("esp32s2") || option("esp32s3")
esp_rtos::start(timg0.timer0);
//ELSE
let sw_interrupt = esp_hal::interrupt::software::SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
let sw_interrupt =
esp_hal::interrupt::software::SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
esp_rtos::start(timg0.timer0, sw_interrupt.software_interrupt0);
//ENDIF
//ENDIF

//IF option("defmt") || option("log")
info!("Embassy initialized!");
Expand All @@ -99,17 +101,17 @@ async fn main(spawner: Spawner) -> ! {
//ENDIF

//IF option("ble-trouble") || option("ble-bleps") || option("wifi")
let radio_init = esp_radio::init()
.expect("Failed to initialize Wi-Fi/BLE controller");
let radio_init = esp_radio::init().expect("Failed to initialize Wi-Fi/BLE controller");
//ENDIF
//IF option("wifi")
let (mut _wifi_controller, _interfaces) = esp_radio::wifi::new(&radio_init, peripherals.WIFI, Default::default())
.expect("Failed to initialize Wi-Fi controller");
let (mut _wifi_controller, _interfaces) =
esp_radio::wifi::new(&radio_init, peripherals.WIFI, Default::default())
.expect("Failed to initialize Wi-Fi controller");
//ENDIF
//IF option("ble-trouble")
// find more examples https://github.com/embassy-rs/trouble/tree/main/examples/esp32
let transport = BleConnector::new(&radio_init, peripherals.BT, Default::default()).unwrap();
let ble_controller = ExternalController::<_, 20>::new(transport);
let ble_controller = ExternalController::<_, 1>::new(transport);
let mut resources: HostResources<DefaultPacketPool, CONNECTIONS_MAX, L2CAP_CHANNELS_MAX> =
HostResources::new();
let _stack = trouble_host::new(ble_controller, &mut resources);
Expand Down
14 changes: 8 additions & 6 deletions template/src/bin/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
reason = "mem::forget is generally not safe to do with esp_hal types, especially those \
holding buffers for the duration of a data transfer."
)]
#![deny(clippy::large_stack_frames)]

use esp_hal::{
clock::CpuClock,
Expand Down Expand Up @@ -85,15 +86,16 @@ fn main() -> ! {
//IF option("esp32") || option("esp32s2") || option("esp32s3")
esp_rtos::start(timg0.timer0);
//ELSE
let sw_interrupt = esp_hal::interrupt::software::SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
let sw_interrupt =
esp_hal::interrupt::software::SoftwareInterruptControl::new(peripherals.SW_INTERRUPT);
esp_rtos::start(timg0.timer0, sw_interrupt.software_interrupt0);
//ENDIF
let radio_init = esp_radio::init()
.expect("Failed to initialize Wi-Fi/BLE controller");
//ENDIF
let radio_init = esp_radio::init().expect("Failed to initialize Wi-Fi/BLE controller");
//ENDIF
//IF option("wifi")
let (mut _wifi_controller, _interfaces) = esp_radio::wifi::new(&radio_init, peripherals.WIFI, Default::default())
.expect("Failed to initialize Wi-Fi controller");
let (mut _wifi_controller, _interfaces) =
esp_radio::wifi::new(&radio_init, peripherals.WIFI, Default::default())
.expect("Failed to initialize Wi-Fi controller");
//ENDIF
//IF option("ble-bleps")
let _connector = BleConnector::new(&radio_init, peripherals.BT, Default::default());
Expand Down