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

fix(embassy-init): move out peripherals required for drivers earlier #337

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ROMemories
Copy link
Collaborator

@ROMemories ROMemories commented Jul 1, 2024

Description

Previously, it would have been possible for user tasks to take peripherals out of OptionalPeripherals before the required peripherals had been extracted for drivers.
This now takes the peripherals before starting the user tasks, making this impossible for tasks.

Issues/PRs references

Depends on #533.

Open Questions

None.

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@ROMemories ROMemories added the arch Architecture support label Jul 1, 2024
@ROMemories ROMemories requested a review from kaspar030 July 1, 2024 14:52
@@ -34,14 +36,10 @@ async fn wifi_cyw43_task(runner: Runner<'static, Output<'static>, CywSpi>) -> !
runner.run().await
}

pub async fn device<'a, 'b: 'a>(
mut p: &'a mut OptionalPeripherals,
pub async fn init<'a>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I renamed this function for consistency with other driver modules; this is not a breaking change as this module is private.

@ROMemories ROMemories force-pushed the fix/extract-driver-peripherals-earlier-in-init branch 2 times, most recently from 69c482f to 22a7c9e Compare July 1, 2024 15:02
Previously, it would have been possible for user tasks to take
peripherals out of `OptionalPeripherals` *before* the required
peripherals had been extracted for drivers.
This now takes the peripherals before starting the user tasks, making
this scenario impossible.
@ROMemories ROMemories force-pushed the fix/extract-driver-peripherals-earlier-in-init branch from 22a7c9e to d2cbe4e Compare July 1, 2024 15:13
@ROMemories ROMemories marked this pull request as draft July 1, 2024 15:17
@ROMemories
Copy link
Collaborator Author

Marking as draft as this requires a separate PR to improve the macro error message, that would need to be landed first.

@@ -34,7 +32,9 @@ impl<'a> Driver<'a> for UsbDriver {
}
}

pub fn driver(_peripherals: &mut arch::OptionalPeripherals) -> UsbDriver {
crate::define_peripherals!(Peripherals {});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This definition is what triggers the different macro error message. As the error message is clearer this way, this doesn't seem like an issue.

@ROMemories ROMemories marked this pull request as ready for review July 2, 2024 07:16
@ROMemories
Copy link
Collaborator Author

Unmarking as draft as the different error message is only due to defining Peripherals in the dummy usb module; it doesn't make sense to move that change to a separate PR.

@kaspar030
Copy link
Collaborator

I'm not sure I agree with this direction.

  1. this makes some parts of the system special ("ours"), demoting user code. IMO this shouldn't be the case, we should rather try harder to build our components on what we offer to users.
  2. this is, imo, clumsier to use. Now there's an extra define_peripherals!() for a lot of cases where there is only one (or even no) peripheral used.

@ROMemories
Copy link
Collaborator Author

this makes some parts of the system special ("ours"), demoting user code

I'm not sure I understand your comment; this only changes part of the driver initialization, fixing bugs in case users enabled specific drivers through Cargo features (e.g., usb), but also attempted to extract the required peripherals in users tasks, in which case the following would panic:

pub fn driver(peripherals: &mut arch::OptionalPeripherals) -> UsbDriver {
let usbd = peripherals.USBD.take().unwrap();

By moving out peripherals earlier, before user tasks, this prevents this panic in drivers; instead the users' .take() would return a None for those peripherals, as they would already be taken.

this is, imo, clumsier to use. Now there's an extra define_peripherals!() for a lot of cases where there is only one (or even no) peripheral used

This is only driver initialization; the Peripherals struct only need to be defined for drivers that require at least one peripheral on one of the architectures.

@kaspar030
Copy link
Collaborator

By moving out peripherals earlier, before user tasks, this prevents this panic in drivers; instead the users' .take() would return a None for those peripherals, as they would already be taken.

Hmya, the user code would then most likely panic in its .unwrap() of that None, we successfully protected "system" (our) code. That's what I mean. There'll be a panic in any case.

This practically moves back taking / splitting of the peripherals back into a central function, but in a way that "users" cannot replicate, b/c they're bound to tasks that only get the full OptionalPeripherals.

We're basically chickening out of handling our .take().unwrap()s properly. So, I'm not convinced this is desirable.

@kaspar030
Copy link
Collaborator

So, I'm not convinced this is desirable.

What I think might be preferable would be a way to track where peripherals were taken (e.g., modify "take()" to trace!() where it was called if nudged to to so by, e.g., sth like CONFIG_PERIPH_TRACE=PA15,USBD).

@kaspar030
Copy link
Collaborator

user tasks

we could also have two task lists, one for those that need to hook into the regular init, and one "default" that is only started after all regular init has taken place.

@ROMemories
Copy link
Collaborator Author

Hmya, the user code would then most likely panic in its .unwrap() of that None, we successfully protected "system" (our) code. That's what I mean. There'll be a panic in any case.

This practically moves back taking / splitting of the peripherals back into a central function, but in a way that "users" cannot replicate, b/c they're bound to tasks that only get the full OptionalPeripherals.

These early peripherals extractions are feature-gated the same way the associated drivers are. When users enable driver Cargo features, this implies asking us to handle the required peripherals; so if users also try to obtain the peripherals required for that driver, it's a bug in users' code. Users still have full access to all peripherals, unless they enable driver Cargo features.

@kaspar030
Copy link
Collaborator

Looking at this again, I think it is a good idea. Please rebase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch Architecture support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants