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

Add BSP features for usb-logging and systick #65

Merged
merged 4 commits into from
Jun 30, 2020
Merged

Conversation

mciantyre
Copy link
Owner

See the discussions on #64. This PR adds two, default features for the teensy4-bsp:

  • "usb-logging" enables the USB logging capabilities. Depends on "systick".
  • "systick" enables the BSP's definition of the SYSTICK exception handler.

The goal is to keep features additive, as expected by cargo. At the same time, we keep the two features enabled by default for backwards compatibility.

#64 proposes that we add RTIC support behind a new feature, "rtic". If this is accepted, users will have to disable default features before safely enabling the "rtic" feature.

This also knocks-out #36, since we were messing with SYSTICK.

Each feature is enabled by default (since we're trying to maintain
backwards compatibility, for now). By disabling the "usb-logging"
feature, we don't have to depend on the teensy4-usb-sys, so that
becomes an optional dependency.
This is a breaking change. It closes #36.

Since there's a "systick" feature, we can more readily isolate the
SYSTICK code behind a conditionally-compiled systick module. We
expose a SysTick timer struct with a simple delay() method to provide
the same functionality. The new struct implements the blocking delay
trait of the embedded-hal.

Tested with 'make download_systick' and observing a blinking LED.
All examples build.
@mciantyre mciantyre marked this pull request as draft June 30, 2020 00:20
@mciantyre
Copy link
Owner Author

CC @mitchmindtree. Let me know if we'd rather keep this on the back-burner until we merge in RTIC support.

Copy link
Contributor

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Looks good! I'm happy to rebase on top if you're ready to move ahead with this 👍

@@ -227,6 +236,9 @@ pub struct Peripherals {
pub gpt2: hal::gpt::Unclocked,
/// DMA channels
pub dma: hal::dma::Unclocked,
/// The SysTick delay timer
#[cfg(feature = "systick")]
pub systick: SysTick,
Copy link
Contributor

Choose a reason for hiding this comment

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

Having feature-gated fields on structs can break stuff when adding their associated features due to rust's exhaustive pattern matching - I think this is normally avoided by keeping the fields private to disallow exhaustively matching on the struct and exposing them via methods instead. In our case however I think this is probably totally fine, especially as exposing only some peripherals via methods would inevitably lead to ownership issues that would be much more annoying to deal with heh!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great call. It sounds like we need to prevent exhaustive pattern matching, and a #[non_exhaustive] attribute on Peripherals might be perfect for that. That should let users enable features without introducing compilation errors.

Users who were using features, but want to disable features, would still need to guard against these members disappearing, just as they would have to deal with conditionally-compiled methods disappearing.

We don't need the log crate if we're not using usb-logging
This should force users who are *not* using default features safely
pattern-match against the `Peripherals` struct.
@mciantyre mciantyre marked this pull request as ready for review June 30, 2020 21:47
@mciantyre mciantyre merged commit 6657151 into master Jun 30, 2020
@mciantyre mciantyre deleted the bsp-features branch June 30, 2020 21:53
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.

2 participants