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.

Sign up for GitHub

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

Jump to bottom

fix for 11-usart #267

Closed
wants to merge 1 commit into from
Closed

fix for 11-usart #267

wants to merge 1 commit into from

Conversation

zhao-lang
Copy link

@paulkernfeld should fix #264

11-usart was panicking on the older version of cortex-m, updating to 0.6.3 fixes that, but introduces incompatibilities with the f3 crate. I replaced the f3 crate with the newer stm32f3xx-hal. Also had to add memory.x to project root in order to get things to compile, maybe there's a better solution for that?

@zhao-lang zhao-lang requested a review from a team as a code owner September 4, 2020 02:02
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @andre-richter (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources labels Sep 4, 2020
@eldruin
Copy link
Member

eldruin commented Sep 4, 2020

FYI, we are looking at a major rewrite of this book since the STM32F3Discovery boards have become difficult to buy. We may choose the micro:bit board as a replacement. See #263

@andre-richter
Copy link
Member

andre-richter commented Sep 10, 2020 •
edited
Loading

I am not familiar with the cortex-m stuff. Temporarily switching my assignment with @adamgreig. Adam, please see if you can review or delegate this. Thanks :)

@bitdivine bitdivine mentioned this pull request Oct 11, 2020
Open
@bitdivine
Copy link

bitdivine commented Oct 11, 2020 •
edited
Loading

@andre-richter Is someone else available to review this? I assume that @adamgreig is busy. I have tested the PR and it works with one caveat: The call to usart1.tdr.write in src/main.rs needs to be wrapped in an unsafe block.

@luiswirth luiswirth mentioned this pull request Nov 28, 2020
Closed
winksaville added a commit to winksaville/rust-embedded-discovery that referenced this pull request Jan 3, 2021
@winksaville
Tweak PR267
d7214af 
PR267 is here:
  rust-embedded#267

I forked rust-embedded/discovery, then cloned my fork from master
and added upstream to my clone so the remotes for it are:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git remote -v
  origin  git@github.com:winksaville/rust-embedded-discovery.git (fetch)
  origin  git@github.com:winksaville/rust-embedded-discovery.git (push)
  upstream        git@github.com:rust-embedded/discovery.git (fetch)
  upstream        git@github.com:rust-embedded/discovery.git (push)

I then fetched PR267 with:
  git fetch upstream pull/267/head:fix-for-11-usart-using-pr267-and-tweaks
  git checkout fix-for-11-usart-using-pr267-and-tweaks

And then modified src/main.rs as follows:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git diff src/main.rs
  diff --git a/src/11-usart/src/main.rs b/src/11-usart/src/main.rs
  index 16c26e8..2357754 100644
  --- a/src/11-usart/src/main.rs
  +++ b/src/11-usart/src/main.rs
  @@ -1,4 +1,4 @@
  -#![deny(unsafe_code)]
  +//#![deny(unsafe_code)]
   #![no_main]
   #![no_std]

  @@ -6,11 +6,12 @@
   use aux11::{entry, iprint, iprintln};

   #[entry]
  +#[allow(unused_variables)]
   fn main() -> ! {
       let (usart1, mono_timer, itm) = aux11::init();

       // Send a single character
  -    usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X')));
  +    unsafe { usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X'))); }

       loop {}
   }
winksaville added a commit to winksaville/rust-embedded-discovery that referenced this pull request Jan 22, 2021
@winksaville
Tweak PR267
c89e100 
PR267 is here:
  rust-embedded#267

I forked rust-embedded/discovery, then cloned my fork from master
and added upstream to my clone so the remotes for it are:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git remote -v
  origin  git@github.com:winksaville/rust-embedded-discovery.git (fetch)
  origin  git@github.com:winksaville/rust-embedded-discovery.git (push)
  upstream        git@github.com:rust-embedded/discovery.git (fetch)
  upstream        git@github.com:rust-embedded/discovery.git (push)

I then fetched PR267 with:
  git fetch upstream pull/267/head:fix-for-11-usart-using-pr267-and-tweaks
  git checkout fix-for-11-usart-using-pr267-and-tweaks

And then modified src/main.rs as follows:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git diff src/main.rs
  diff --git a/src/11-usart/src/main.rs b/src/11-usart/src/main.rs
  index 16c26e8..2357754 100644
  --- a/src/11-usart/src/main.rs
  +++ b/src/11-usart/src/main.rs
  @@ -1,4 +1,4 @@
  -#![deny(unsafe_code)]
  +//#![deny(unsafe_code)]
   #![no_main]
   #![no_std]

  @@ -6,11 +6,12 @@
   use aux11::{entry, iprint, iprintln};

   #[entry]
  +#[allow(unused_variables)]
   fn main() -> ! {
       let (usart1, mono_timer, itm) = aux11::init();

       // Send a single character
  -    usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X')));
  +    unsafe { usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X'))); }

       loop {}
   }
winksaville added a commit to winksaville/rust-embedded-discovery that referenced this pull request Jan 24, 2021
@winksaville
Tweak PR267
0506054 
PR267 is here:
  rust-embedded#267

I forked rust-embedded/discovery, then cloned my fork from master
and added upstream to my clone so the remotes for it are:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git remote -v
  origin  git@github.com:winksaville/rust-embedded-discovery.git (fetch)
  origin  git@github.com:winksaville/rust-embedded-discovery.git (push)
  upstream        git@github.com:rust-embedded/discovery.git (fetch)
  upstream        git@github.com:rust-embedded/discovery.git (push)

I then fetched PR267 with:
  git fetch upstream pull/267/head:fix-for-11-usart-using-pr267-and-tweaks
  git checkout fix-for-11-usart-using-pr267-and-tweaks

And then modified src/main.rs as follows:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git diff src/main.rs
  diff --git a/src/11-usart/src/main.rs b/src/11-usart/src/main.rs
  index 16c26e8..2357754 100644
  --- a/src/11-usart/src/main.rs
  +++ b/src/11-usart/src/main.rs
  @@ -1,4 +1,4 @@
  -#![deny(unsafe_code)]
  +//#![deny(unsafe_code)]
   #![no_main]
   #![no_std]

  @@ -6,11 +6,12 @@
   use aux11::{entry, iprint, iprintln};

   #[entry]
  +#[allow(unused_variables)]
   fn main() -> ! {
       let (usart1, mono_timer, itm) = aux11::init();

       // Send a single character
  -    usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X')));
  +    unsafe { usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X'))); }

       loop {}
   }
winksaville added a commit to winksaville/rust-embedded-discovery that referenced this pull request Jan 28, 2021
@winksaville
Tweak PR267
ae13fd5 
PR267 is here:
  rust-embedded#267

I forked rust-embedded/discovery, then cloned my fork from master
and added upstream to my clone so the remotes for it are:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git remote -v
  origin  git@github.com:winksaville/rust-embedded-discovery.git (fetch)
  origin  git@github.com:winksaville/rust-embedded-discovery.git (push)
  upstream        git@github.com:rust-embedded/discovery.git (fetch)
  upstream        git@github.com:rust-embedded/discovery.git (push)

I then fetched PR267 with:
  git fetch upstream pull/267/head:fix-for-11-usart-using-pr267-and-tweaks
  git checkout fix-for-11-usart-using-pr267-and-tweaks

And then modified src/main.rs as follows:
  wink@3900x:~/prgs/rust/tutorial/embedded-discovery/src/11-usart (fix-for-11-usart-using-pr267-and-tweaks)
  $ git diff src/main.rs
  diff --git a/src/11-usart/src/main.rs b/src/11-usart/src/main.rs
  index 16c26e8..2357754 100644
  --- a/src/11-usart/src/main.rs
  +++ b/src/11-usart/src/main.rs
  @@ -1,4 +1,4 @@
  -#![deny(unsafe_code)]
  +//#![deny(unsafe_code)]
   #![no_main]
   #![no_std]

  @@ -6,11 +6,12 @@
   use aux11::{entry, iprint, iprintln};

   #[entry]
  +#[allow(unused_variables)]
   fn main() -> ! {
       let (usart1, mono_timer, itm) = aux11::init();

       // Send a single character
  -    usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X')));
  +    unsafe { usart1.tdr.write(|w| w.tdr().bits(u16::from(b'X'))); }

       loop {}
   }
@adamgreig
Copy link
Member

Ugh, sorry, this totally fell off my radar. I think there's two issues merging this as-is:

  1. It requires an additional change to 11-usart/src/main.rs to put the call to tdr().bits() into an unsafe block (and also removing the #[deny(unsafe_code)] attribute at the top), but sticking that unsafe code in seems quite confusing to newcomers and would need explaining (why do we need it here but not elsewhere, etc)
  2. I guess memory.x was being provided by the HAL, but we shouldn't then stick a new one in the repository root. We could probably place it inside the 11-usart/ directory along with a build.rs script to copy it to the linker directory.

For the second point I think we can just swap the dependency to the new stm32f3-discovery crate and update the exports to stm32f3_discovery::stm32f3xx_hal::..., so we don't need a memory.x at all.

However I think we still need the unsafe block which isn't great... looking into it it seems like an error in the stm32f3 crate where the TDR register is said to be safe for any value 0-255 but as it's a nine-bit register it should actually go to 0x1ff. Dunno why that's not been caught before to be honest... but it won't be a quick update, so for now I guess we're stuck with the unsafe block.

@wpd wpd mentioned this pull request Apr 23, 2021
Merged
bors bot added a commit that referenced this pull request Apr 24, 2021
@bors @wpd
Merge #328
944e708 
328: Fix breaking change in  cortex-m 0.5.11. r=adamgreig a=wpd

Fix it by sticking with cortex-m 0.5.6.

See discussion at #327.  This is a much simpler (and certainly more hackish) solution than is proposed in #267.  But it doesn't require `unsafe` code nor a custom `memory.x` file, and might fill the gap until the book is rewritten.

Co-authored-by: Patrick Doyle <wpdster@gmail.com>
bors bot added a commit that referenced this pull request Apr 27, 2021
@bors @wpd
Merge #328
65e4bd3 
328: Fix breaking change in  cortex-m 0.5.11. r=adamgreig a=wpd

Fix it by sticking with cortex-m 0.5.6.

See discussion at #327.  This is a much simpler (and certainly more hackish) solution than is proposed in #267.  But it doesn't require `unsafe` code nor a custom `memory.x` file, and might fill the gap until the book is rewritten.

Co-authored-by: Patrick Doyle <wpdster@gmail.com>
bors bot added a commit that referenced this pull request Apr 27, 2021
@bors @wpd
Merge #328
3132ecd 
328: Fix breaking change in  cortex-m 0.5.11. r=adamgreig a=wpd

Fix it by sticking with cortex-m 0.5.6.

See discussion at #327.  This is a much simpler (and certainly more hackish) solution than is proposed in #267.  But it doesn't require `unsafe` code nor a custom `memory.x` file, and might fill the gap until the book is rewritten.

Co-authored-by: Patrick Doyle <wpdster@gmail.com>
This was referenced May 27, 2021
Merged
Merged
bors bot added a commit to stm32-rs/stm32-rs that referenced this pull request May 28, 2021
@bors @sirhcel
Merge #558
5208696 
558: Fix write constraints for USART RDR and TDR registers r=adamgreig a=sirhcel

The STM32F303 has 9 bit wide RDR and TDR registers and according to the SVDs this is the case for all other devices with this peripheral too. Make accessing TDR via tdr().bits() safe again by providing the appropriate constraints.

I stumbled upon this issue in rust-embedded/discovery#267 (comment).

The currently exisiting bitWidths have been quick-and-dirty checked using:
```bash
svd $ xmllint --xpath ".//field[(name = 'RDR' or name = 'TDR') and bitWidth != 9]" --format *.svd *.svd.patched
```
This gave not output while searching for `bitWidth = 9` did.

Co-authored-by: Christian Meusel <christian.meusel@posteo.de>
This was referenced May 31, 2021
Closed
Closed
This was referenced Jun 16, 2021
Merged
Merged
bors bot added a commit that referenced this pull request Jun 21, 2021
@bors @sirhcel
Merge #354
2b73ce6 
354: Fix and clean up 11-usart by bumping dependencies r=adamgreig a=sirhcel

This is an alternate version of #267 which fixes the panic by bumping dependencies. It also addresses the previously unsafe access to the USARTs transmit and receive data registers (TDR and RDR, stm32-rs/stm32-rs#558) by bumping the BSP to the brand spanking new release 0.7.0 which comes with a fixed PAC and HAL. Thank you very much @adamgreig, @Sh3Rm4n, and @rubberduck203 for your patience and support along this journey!

This PR also bumps non-critical dependencies to their actual releases. For example `heapless`, which makes use of const generics nowadays.

I did not succeed in getting `mdbook test` to actually check the example code. I moved it over to the `examples` directory and included it in the Markdown files so that it can be checked with `cargo build --target thumbv7em-none-eabihf --examples`.

Co-authored-by: Christian Meusel <christian.meusel@posteo.de>
@adamgreig
Copy link
Member

This has now been fixed by #354, thank you for starting the work in this PR!

@adamgreig adamgreig closed this Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@adamgreig adamgreig

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

11-usart None on Peripherals take
6 participants
@zhao-lang @rust-highfive @eldruin @andre-richter @bitdivine @adamgreig