WIP: allow SPI transfers in excess of 65525 bytes #1704
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.