stm32xx-sys: add interrupt acking to gpio_irq_control

[hawkw]

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

[hawkw]

Hmm, so, after trying to integrate this change with my WIP branch #1696 (see c29e5ef), I'm not actually sure if combining the "enable/disable IRQ" and "check if IRQ was triggered" behaviors in the same API is always the Right Thing.

It's nice for cases where code intends to receive an interrupt in a loop, as in the Nucleo user button demo:

hubris/task/nucleo-user-button/src/main.rs

Lines 88 to 126 in 1d44ab8

	loop {
	// Call `Sys.gpio_irq_control` to enable our interrupt, returning
	// whether it has fired.
	let enable_mask = notifications::BUTTON_MASK;
	let disable_mask = 0;
	let fired = sys
	.gpio_irq_control(disable_mask, enable_mask)
	.unwrap_lite();
	ringbuf_entry!(Trace::GpioIrqControl {
	enable_mask,
	disable_mask,
	fired
	});
	// If the button has changed state, toggle the LED.
	if fired {
	ringbuf_entry!(Trace::LedToggle { led });
	user_leds.led_toggle(led).unwrap_lite();
	}
	// Wait for the user button to be pressed.
	'waiting: loop {
	let recvmsg = sys_recv_closed(
	&mut [],
	notifications::BUTTON_MASK,
	TaskId::KERNEL,
	)
	// Recv from the kernel never returns an error.
	.unwrap_lite();
	let notif = recvmsg.operation;
	ringbuf_entry!(Trace::Notification(notif));
	// If the notification is from the button, we're done waiting!
	if notif == notifications::BUTTON_MASK {
	break 'waiting;
	}
	}
	}

or (presumably) @bcantrill's change to record MAX5970 data when it raises its alert line, since such code will always want to unmask the interrupt to receive it again after being notified, and it's nice for that code to only have to do one IPC round-trip, rather than two.

However, for code that only wants to wait for a single interrupt and then go back to doing something else, such as wait_rot_irq in drv-stm32h7-sprot-server, the fact that checking whether an IRQ was triggered can only be done as part of an operation that either enables it or disables it is actually a bit awkward. In stm32h7-sprot-server, we also serve an Idol IPC interface, and doing this requires us to occasionally wait for the IRQ. But, we don't want the notification to go off when we're not in the code that actively waits for the interrupt, since we're trying to serve an IPC interface, and we don't want the code waiting for the interrupt to accidentally consume an IRQ notification that was posted a long time ago but not consumed.

In that case, what we really want is a way to check if the IRQ was triggered, leaving it disabled if it was triggered, but leaving it enabled if the notification was spurious. Right now, because we don't have that, we always perform the check by disabling the IRQ, and then re-enable it if it was not actually received. This means we actually have to do more IPCs than we would otherwise.

I think the interface from a6c9163, where I had a new Sys::gpio_irq_ack IPC that was separate from Sys::gpio_irq_control, was actually better for this use case. That design took a notification mask and a flag indicating whether to re-enable the IRQ if it had actually triggered, and returned whether it had triggered. This works well for both the "wait for multiple notifications in a loop" use-case, where we pass true to the re-enable argument, and in the "one-shot wait for a single notification" use case, where we can pass false to the re-enable argument, leaving the IRQ enabled if it didn't trigger, but not re-enabling it if it did. With this design, both the loop and one-shot use cases only need to perform one IPC, which seems ideal.

If we want to go back to that design, we could still hang this off of gpio_irq_control, or go back to having an additional IPC.

If we wanted to continue using gpio_irq_control, the potential approaches I can think of are:

• adding a third argument of a mask of notifications to check but not change the enabled/disabled state of (eww, three arguments, all of which are masks...)
• special-casing notifications that appear in both the enable and disable masks to just leave them alone but check their state (which seems weird and hard to document)

On the other hand, an additional IPC has the downside of "adding another IPC", complicating the interface and probably resulting in more code, but I think it results in a somewhat less weird API surface. And, it has the side benefit of letting us leave the gpio_irq_control IPC marked as idempotent, if we didn't make it consume the "interrupt pending" state anymore.

@cbiffle, I'd love to get your opinion on this (and maybe also @bcantrill's, as a potential consumer of this API).

[cbiffle]

Generally looks great! One opportunity for deduplication noted below.

I wish I could think of a better pattern to show in examples than unwrap_lite here, because I know people are just going to copy it, which will generate panic sites that aren't probably what they actually want. But I'm not sure what to suggest in the general case, so, this is probably a fine start.

[cbiffle]

Ought to be able to factor bit here out (it's duplicated on line 701)

[hawkw]

I wanted to avoid doing the bit shift in the case where we don't do anything with the entry. But, bit shifts are cheap...right?

[hawkw]

I wish I could think of a better pattern to show in examples than unwrap_lite here, because I know people are just going to copy it, which will generate panic sites that aren't probably what they actually want. But I'm not sure what to suggest in the general case, so, this is probably a fine start.

Hmm, that's a good point; in this case there is a way that a task could reasonably handle the death of the sys task: it could call gpio_irq_configure again to re-configure the IRQ and then re-enable it. But, this would still result in the loss of an IRQ that would have been pending at the time of the sys crash... and, arguably, just letting the client panic and get restarted is as effective of a way of re-configuring and enabling: it does generate panic code but it doesn't generate an additional set of configure/enable calls...I dunno.

[cbiffle]

Restarting the client is generally a reliable way of ensuring things get reconfigured, and can sometimes be the right call -- however, it's also quite expensive, and we've had cases of programs relying on it for mundane errors. It's also a bit of a fault-multiplier in that, if the task in question is also a server (and most are), it propagates the panic out to its callers, etc.

If we can conveniently show how to retry, that would be neat; if it would be a pain and make the example less useful, I'd say don't worry about it.