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 USB driver #135

Merged
merged 2 commits into from
Nov 13, 2019
Merged

Add USB driver #135

merged 2 commits into from
Nov 13, 2019

Conversation

Disasm
Copy link
Member

@Disasm Disasm commented Nov 11, 2019

Cargo.toml Outdated
@@ -91,7 +98,7 @@ doc = []
rt = ["stm32f1/rt"]
stm32f100 = ["stm32f1/stm32f100", "device-selected"]
stm32f101 = ["stm32f1/stm32f101", "device-selected"]
stm32f103 = ["stm32f1/stm32f103", "device-selected"]
stm32f103 = ["stm32f1/stm32f103", "device-selected", "stm32-usbd"]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this rather be under the specific examples using it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how to make this convenient. Current solution is like "hal provides drivers for all the peripherals, so why not to enable USB". Maybe it's better to feature-gate USB support in order to reduce the dependency tree.

Copy link
Member

Choose a reason for hiding this comment

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

Why can't we just put it in the required-features entries of the examples?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this would work for stm32f1xx-hal, but here this stm32-usbd feature means that USB is guaranteed to work on this chip. If user provides this feature, they can possibly activate it for the chip with different configuration, which is not supported yet, or has a different register layout and the same peripheral name.
Maybe I should additionally feature-gate usb module for all the supported chips.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I'm not a big fan of restricting the user to "curated" configurations. Similar to the typically usable 128kB flash even on the "R8" versions, there are also projects which manage to use USB on lesser chips which are not supposed to have USB.

I'd rather say put in the documentation that we have USB support for the STM32F103 and if people experiment with other chips and get it to run, that's fine with me; if they fail and complain we can point them to the documentation.

In theory USB should work on the F102 and F103 unmodified, F105 and F107 do even have OTG so I'm not sure what's needed to get it to work there. There're e.g. Altera USBblaster clones using a STM32F101.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather think of them as of "proved" or "should work according to the datasheet" configurations. I believe that porting to other configurations is quite simple. Also user can just copy usb.rs to their project and fix it to comply with the project-specific peripheral. The same thing works for chips which are not supposed to have USB, but have it.
Both F105 and F107 need another driver, I need to finish and "convert" that driver first.

@Disasm Disasm marked this pull request as ready for review November 12, 2019 16:59
Copy link
Member

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

Could use some commit squashing and a CHANGELOG.md entry. Other than that looks good to me.

// Pull the D+ pin down to send a RESET condition to the USB bus.
let mut usb_dp = gpioa.pa12.into_push_pull_output(&mut gpioa.crh);
usb_dp.set_low();
delay(clocks.sysclk().0 / 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need that? I've removed it from my keyboard, and it still work fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is needed only for development. Without it host doesn't reset your device when you upload new firmware.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe that could be documented in the comment above.

@therealprof therealprof merged commit a62b168 into stm32-rs:master Nov 13, 2019
@Disasm
Copy link
Member Author

Disasm commented Nov 13, 2019

Oops, forgot to update docs.rs feature list. Will fix soon.

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.

None yet

4 participants