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

Remove final panics from stm32xx-sys on h7 and keep it that way #1711

Merged
merged 1 commit into from
Apr 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 1 addition & 1 deletion app/demo-stm32h7-nucleo/app-h743.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ request_reset = ["hiffy"]

[tasks.sys]
name = "drv-stm32xx-sys"
features = ["h743", "exti"]
features = ["h743", "exti", "no-panic"]
priority = 1
uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"]
start = true
Expand Down
2 changes: 1 addition & 1 deletion app/demo-stm32h7-nucleo/app-h753.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ request_reset = ["hiffy"]

[tasks.sys]
name = "drv-stm32xx-sys"
features = ["h753", "exti"]
features = ["h753", "exti", "no-panic"]
priority = 1
uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"]
start = true
Expand Down
2 changes: 1 addition & 1 deletion app/gimlet/base.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ notifications = ["eth-irq", "mdio-timer-irq", "wake-timer", "jefe-state-change"]

[tasks.sys]
name = "drv-stm32xx-sys"
features = ["h753", "exti"]
features = ["h753", "exti", "no-panic"]
priority = 1
uses = ["rcc", "gpios", "system_flash", "syscfg", "exti"]
start = true
Expand Down
2 changes: 1 addition & 1 deletion app/psc/base.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ request_reset = ["hiffy", "control_plane_agent"]

[tasks.sys]
name = "drv-stm32xx-sys"
features = ["h753"]
features = ["h753", "no-panic"]
priority = 1
max-sizes = {flash = 2048, ram = 2048}
uses = ["rcc", "gpios", "system_flash"]
Expand Down
2 changes: 1 addition & 1 deletion app/sidecar/base.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ request_reset = ["hiffy", "control_plane_agent"]

[tasks.sys]
name = "drv-stm32xx-sys"
features = ["h753"]
features = ["h753", "no-panic"]
priority = 1
max-sizes = {flash = 2048, ram = 2048}
uses = ["rcc", "gpios", "system_flash"]
Expand Down
1 change: 1 addition & 0 deletions drv/stm32xx-sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ g070 = ["family-stm32g0", "stm32g0/stm32g070", "drv-stm32xx-sys-api/g070", "drv-
g0b1 = ["family-stm32g0", "stm32g0/stm32g0b1", "drv-stm32xx-sys-api/g0b1", "drv-stm32xx-gpio-common/model-stm32g0b1"]

no-ipc-counters = ["idol/no-counters"]
no-panic = ["userlib/no-panic"]
Copy link
Member

Choose a reason for hiding this comment

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

What's the motivation for making this a task feature, rather than just always enabling it? It seems a bit awkward to have to enable no-panic in the app.tomls, when the sys task will never panic, and this only controls the elimination of the panicking code. Since the task is built independently of other tasks, we don't need to worry about e.g. feature unification enabling no-panic for everyone, so I'd kind of prefer for the app.toml author to not have to think about this at all.

I'm open to being convinced otherwise about this, though, if you think there's a real use-case for not enabling no-panic

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 sys task may never panic, but the compiler is only known to reliably remove all of the panics on some of our ARMv7-M builds. I haven't tested it at all on v6-M. So for now, it's conditional.

Definitely interested in forcing it on later once we've convinced ourselves it won't be annoying.


# Enable external interrupt controller support.
exti = ["dep:hubris-num-tasks", "dep:counters"]
Expand Down
64 changes: 43 additions & 21 deletions drv/stm32xx-sys/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ fn main() -> ! {
// API the way we use peripherals.
let syscfg = unsafe { &*device::SYSCFG::ptr() };

for (i, entry) in generated::EXTI_DISPATCH_TABLE.iter().enumerate() {
for (i, entry) in dispatch_table_iter() {
// Process entries that are filled in...
if let &Some(ExtiDispatch { port, .. }) = entry {
let register = i >> 2;
Expand Down Expand Up @@ -684,8 +684,10 @@ impl idl::InOrderSysImpl for ServerImpl<'_> {
let mut slot_mask = 0u16;

for (i, _) in exti_dispatch_for(rm.sender, mask) {
// What bit do we touch for this entry?
let bit = 1 << i;
// What bit do we touch for this entry? (Mask is to ensure
// that the compiler understands this shift cannot
// overflow.)
let bit = 1 << (i & 0xF);
cbiffle marked this conversation as resolved.
Show resolved Hide resolved

// Record that these bits meant something.
slot_mask |= bit;
Expand Down Expand Up @@ -774,15 +776,19 @@ impl idl::InOrderSysImpl for ServerImpl<'_> {
let mut used_bits = 0u32;

for (i, entry) in exti_dispatch_for(rm.sender, mask) {
// (Mask is to ensure that the compiler understands this
// shift cannot overflow.)
let imask = 1 << (i & 0xF);

used_bits |= entry.mask;

// Set or clear Rising Trigger Selection
// Register bit according to the rising flag
self.exti.rtsr1.modify(|r, w| {
let new_value = if edge.is_rising() {
r.bits() | (1 << i)
r.bits() | imask
} else {
r.bits() & !(1 << i)
r.bits() & !imask
};
unsafe {
w.bits(new_value)
Expand All @@ -793,9 +799,9 @@ impl idl::InOrderSysImpl for ServerImpl<'_> {
// Register bit according to the rising flag
self.exti.ftsr1.modify(|r, w| {
let new_value = if edge.is_falling() {
r.bits() | (1 << i)
r.bits() | imask
} else {
r.bits() & !(1 << i)
r.bits() & !imask
};
unsafe {
w.bits(new_value)
Expand Down Expand Up @@ -849,17 +855,18 @@ fn exti_dispatch_for(
task: TaskId,
mask: u32,
) -> impl Iterator<Item = (usize, &'static ExtiDispatch)> {
generated::EXTI_DISPATCH_TABLE
.iter()
.enumerate()
.filter_map(move |(i, entry)| {
let entry = entry.as_ref()?;
if task.index() == entry.task.index() && mask & entry.mask != 0 {
Some((i, entry))
} else {
None
}
})
// This is semantically equivalent to iter.enumerate, but winds up handing
// the compiler very different code that avoids an otherwise-difficult panic
// site on an apparently-overflowing addition (that was not actually capable
// of overflowing).
dispatch_table_iter().filter_map(move |(i, entry)| {
let entry = entry.as_ref()?;
if task.index() == entry.task.index() && mask & entry.mask != 0 {
Some((i, entry))
} else {
None
}
})
}

impl NotificationHandler for ServerImpl<'_> {
Expand Down Expand Up @@ -898,9 +905,13 @@ impl NotificationHandler for ServerImpl<'_> {

let mut bits_to_acknowledge = 0u16;

for (pin_idx, entry) in
generated::EXTI_DISPATCH_TABLE.iter().enumerate()
{
for pin_idx in 0..16 {
// TODO: this sure looks like it should be using
// iter.enumerate, doesn't it? Unfortunately that's not
// currently getting inlined by rustc, resulting in rather
// silly code containing panics. This is significantly
// smaller.
let entry = &generated::EXTI_DISPATCH_TABLE[pin_idx];
if pending_and_enabled & 1 << pin_idx != 0 {
// A channel is pending! We need to handle this
// basically like the kernel handles native hardware
Expand Down Expand Up @@ -1214,6 +1225,17 @@ cfg_if! {
}
}

#[cfg(feature = "exti")]
#[inline(always)]
fn dispatch_table_iter(
) -> impl Iterator<Item = (usize, &'static Option<ExtiDispatch>)> {
// TODO: this sure looks like it should be using iter.enumerate, doesn't it?
// Unfortunately that's not currently getting inlined by rustc, resulting in
// rather silly code containing panics. This is significantly smaller.
(0..generated::EXTI_DISPATCH_TABLE.len())
.zip(&generated::EXTI_DISPATCH_TABLE)
}

include!(concat!(env!("OUT_DIR"), "/notifications.rs"));

mod idl {
Expand Down
Loading