Skip to content

Conversation

usbalbin
Copy link
Member

No description provided.

@usbalbin usbalbin marked this pull request as ready for review July 27, 2025 11:25
Comment on lines 76 to 80
}

// Echo back in upper case
for c in buf[0..count].iter_mut() {
if 0x61 <= *c && *c <= 0x7a {
Copy link
Member Author

Choose a reason for hiding this comment

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

Any ideas for solving this?

error: current MSRV (Minimum Supported Rust Version) is `1.78.0` but this item is stable since `1.87.0`
  --> examples/usb_serial.rs:72:32
   |
72 |                 if let Ok(s) = str::from_utf8(&buf[0..count]) {
   |                                ^^^^^^^^^^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
note: the lint level is defined here
  --> examples/usb_serial.rs:2:9

Copy link
Member

Choose a reason for hiding this comment

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

We could bump up the MSRV? We haven't published a crate yet...

Copy link
Member

@astapleton astapleton Jul 28, 2025

Choose a reason for hiding this comment

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

Also, instead of doing this uppercasing manually, you could just call c.to_ascii_uppercase(): https://doc.rust-lang.org/stable/core/primitive.char.html#method.to_ascii_uppercase (and use map instead of a for loop)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh nice: )

Copy link
Member Author

Choose a reason for hiding this comment

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

Any suggestions for a MSRV?

Copy link
Member

Choose a reason for hiding this comment

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

Might as well bump it to 1.88?

@usbalbin
Copy link
Member Author

Note that this is only tested on a Nucleo-H533RE

src/usb.rs Outdated
Comment on lines 101 to 106
// Enable USB peripheral
rcc.apb2enr().modify(|_, w| w.usben().set_bit());

// Reset USB peripheral
rcc.apb2rstr().modify(|_, w| w.usbrst().set_bit());
rcc.apb2rstr().modify(|_, w| w.usbrst().clear_bit());
Copy link
Member

Choose a reason for hiding this comment

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

How about:

rec::Usb { _marker: PhantomData }.reset().enable();

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, thanks

src/usb.rs Outdated

fn enable() {
cortex_m::interrupt::free(|_| {
let rcc = unsafe { &*stm32::RCC::ptr() };
Copy link
Member

Choose a reason for hiding this comment

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

nit: move this closer to where it's first used.

@usbalbin
Copy link
Member Author

Note: I have not tested this since b6c7432

Copy link
Member

@astapleton astapleton left a comment

Choose a reason for hiding this comment

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

A few more comments

src/usb.rs Outdated
/// Data negative pin
pin_dm: DmPin,
/// Data positive pin
pin_dp: DpPin,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to hang on to these pins? Doesn't looks like they get used anywhere. Could just consume them in the new function so they can't get used anywhere else (e.g. SPI driver does this).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need to hang on to these pins? [...]

I dont think we do

src/usb.rs Outdated
Comment on lines 61 to 69
impl fmt::Debug for UsbDevice {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Peripheral")
.field("usb", &"USB")
.field("pin_dm", &self.pin_dm)
.field("pin_dp", &self.pin_dp)
.finish()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

this is out of date. how about just using the Debug derive for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually don not use the pins for anything once they are put into the struct. As in we have no release method. Should I remove them from the struct?

Copy link
Member Author

Choose a reason for hiding this comment

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

fields `pin_dm` and `pin_dp` are never read
`UsbDevice` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
`#[warn(dead_code)]` on by default

Copy link
Member

Choose a reason for hiding this comment

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

Yep. Latest changes look good.

src/usb.rs Outdated
fn format(&self, f: defmt::Formatter) {
defmt::write!(
f,
"Peripheral {{ usb: USB, pin_dm: {}, pin_dp: {}}}",
Copy link
Member

Choose a reason for hiding this comment

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

Name is out of date

Copy link
Member

@astapleton astapleton left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks for doing this! Looks good to me.

@astapleton astapleton merged commit b82da58 into stm32-rs:master Jul 29, 2025
33 checks passed
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