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

WIP: allow SPI transfers in excess of 65525 bytes #1704

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 2, 2024

No description provided.

hawkw and others added 26 commits March 29, 2024 15:16
Now that release 7 is closer to finalized and we've gained some
confidence in the kernel fixes for #1672, it would be nice to regain the
benefits of task packing and stop fretting so much about fragmentation.
This should help avoid the situation where someone unknowingly removes
or modifies this in a way that breaks racktest.
The comment in the `nucleo-user-button` demo task gets the order of
arguments to `sys.gpio_irq_control` backwards. Since this example task
is intended to demonstrate how to use this API, we should definitely fix
this. Thanks @bcantrill for catching it!
The current `Sys::gpio_irq_configure` IPC interface is a bit confusing,
as it takes two `bool`s for the rising and falling edge sensitivities,
as in:

```rust
sys.gpio_irq_configure(mask, true, false);
```

This isn't particularly clear for a reader, who then has to remember the
order of the arguments to determine which edge sensitivity is being
enabled here. This is a case of ["boolean blindness"][1].

This commit improves the interface by changing it to take an `enum` with
variants for `Rising`, `Falling`, and `Both`. Now, the same call to the
IPC looks like this:

```rust
sys.gpio_irq_configure(mask, Edge::Rising);
```

which should be much more obvious to a reader, who no longer needs to
remember the argument ordering to decypher calls to this function.

Also, this has the advantage of preventing the case where both rising
and falling edge sensitivities are disabled at compile-time. Currently,
this is a runtime error (reply fault), but with the new interface, a
caller can provide an invalid configuration if they hand-write the input
buffer rather than using the Idol client stub, so it should be harder to
mess up.

[1]: https://runtimeverification.com/blog/code-smell-boolean-blindness
task/sensor was configuring a timer to wake it every 1 Hz. Each time it
woke up, it would snooze its alarm clock and go back to sleep.

While I definitely sympathize with that desire, this is essentially dead
code that looks like a bug, so this commit removes it. I'm not sure what
the history here is.

In addition to removing noise from the code, this knocks 114 bytes off
the sensor task text size.
When enabled, this will turn any panics in a task into linker errors.
That's not ideal, but it's better than expecting people to manually read
disassembly.
These comments by 2020-me didn't super make sense to 2024-me, so I have
rewritten them to hopefully be a little more clear and reveal some
context that was previously implicit.
This is an alternate API for accessing the common "receive only kernel
notifications" pattern. That specific use of the sys_recv syscall is
defined as not being able to fail. Different users have handled that in
different ways -- some panic, some ignore the result. The latter embeds
the assumption of that behavior in many different places in the
codebase, a pattern I'm trying to keep an eye on.

This change adds a new sys_recv_notification userlib function that
bundles up this common pattern. Because of its central placement in
userlib, it seems like a more appropriate place to encode the assumption
about the syscall not failing, and so I've done that, using
unreachable_unchecked to explain it to the compiler.

This removes a panic site from every notification-wait in programs that
were previously using unwrap or unwrap_lite.
There should be no non-whitespace changes in this PR!

- Always use 4x spaces for indentation
- Don't include empty `args: {}`
- Don't include a space before `:` or after `(`
In the Giant Scary Nested Match we had to write to be able to address
the EXTI register array as an array through the PAC, I typo'd the number
base being used to select the field within the register, by reusing 2
from the line above. This is the _base 2 logarithm_ of the number we
actually wanted, 4, because the operations are shift and mod,
respectively.

Wheeee

Anyway, this fixes the situation where EXTI configuration of any pin p
such that p % 4 = 2 or 3 on any port other than A mysteriously senses
a different pin in port A instead.
Currently, there is no mechanism for a task woken by a notification
mapped to an EXTI GPIO interrupt to determine whether the pin it cares
about has actually changed state, rather than receiving a notification
posted by some other task (see #1700). In order to make this possible,
@cbiffle [suggested] that the `stm32xx-sys` task provide an IPC
interface that a task receiving an EXTI notification can call to ack
that the IPC actually was caused by a pin state transition.

This branch extends the `Sys::gpio_irq_control` IPC to return a bool if
any EXTI IRQs mapped to the notifications in a task's notification
maskhave triggered since the last time `gpio_irq_control` was called.

Adding this functionality to `gpio_irq_control` --- which a task that
wishes to receive another EXTI notification must call anyway --- allows
us to keep the number of IPC roundtrips the same when a task receives
such notifications in a loop. In order to support other cases, where a
task only cares about one such notification, the `gpio_irq_control`
interface has been changed to determine whether to enable or disable the
IRQs in the mask based on a second argument, rather than taking two
masks as it did previously, in order to allow a third operation which
*only* checks the state of the interrupt, leaving it enabled if it has
not been triggered but not re-enabling it if it has. We no longer have
the ability to both enable and disable disjunct sets of IRQs in one IPC,
but in practice, no task we have currently even *uses* more than one
GPIO IRQ anyway. And, getting rid of the double-mask interface also
fixes #1714.

I've implemented this by tracking a 16-bit bitmap in the `Sys` task that
mirrors the EXTI pending register. This is because `sys` must clear the
pending bit in order to receive another interrupt from _any_ EXTI
source.

Closes #1700
Closes #1714

[suggested]: 
  #1700 (comment)
At least in some configurations.
Also fixed _some_ new warnings reported by the toolchain, and many
places where rustfmt has apparently changed its mind about some things.

Removed size constraints for tasks that increased in size rather than
playing guess-the-number.

This leaves a number of warnings, which I'm hoping to divide up and not
have to fix myself.
*famous last words
This was adding 20 kiB (!) to the task because (1) it wasn't getting
inlined and (2) its non-inlined implementation contains a panic that
pulls in f32 formatting code, which has recently gotten really
expensive.
We've noticed the compiler tends to generate better code for integer
range matches of the form

    match x {
        0..=7 => a,
        7..=10 => b,
        _ => c,
    }

as compared to

    match x {
        0
        | 1
        | 2
        | 3
        | 4
        | 5
        | 6
        | 7 => a,
        // and so on
    }

Also, Clippy in recent nightly has started objecting to the "cascade of
digits" form. Rather than suppressing that warning, I thought I'd
convert the biggest matches in our codegen output to use ranges.

This commit uses rangemap to automatically consolidate adjacent ranges
of integers into a convenient match statement. I've also factored out
the code from the two places it was used.
This change causes SpUpdate to impl ComponentUpdater, instead of
sporting functions that look a lot _like_ ComponentUpdater but aren't
actually. This fixes a warning in the newest toolchain, which was
concerned that operations in the non-public trait were going unused
(which they were!).

In order to adapt ComponentUpdater to be able to describe all three
cases, I have added two associated types to it. UpdatePrepare
parameterizes the difference between RoT/HostFlash updates (which want
ComponentUpdatePrepare) and SP updates (which want a different
SpUpdatePrepare type). SubComponent models the fact that only the SP
seems to want a specifier for sub-components inside itself; the others
set this to ().
Clippy pointed out that this loop can't, well, loop. At all.

I'm kind of surprised earlier versions missed this.
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.

3 participants