Skip to content

[RFC] move to cargo-generate; start with QEMU #45

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

Merged
merged 9 commits into from
Sep 17, 2018
Merged

[RFC] move to cargo-generate; start with QEMU #45

merged 9 commits into from
Sep 17, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Aug 29, 2018

I was going to propose moving to cargo-generate now by having the default
values work for the LM3S6965 microcontroller which QEMU supports and also to
update the documentation to start out with QEMU but while implementing this it
occurred to me that this documentation (see README.md) may be a better fit for
the embedded book and should be there instead of here. Which begs the question
what should the documentation of this template consist of?

thoughts? ^ @rust-embedded/resources @rust-embedded/cortex-m

Oh, and the rationale for starting with QEMU is that (a) way more people will be
able to try it out as it doesn't require hardware; (b) there's less chance of
things going wrong because there are no device specific steps, there are no
permissions to fiddle with (see OpenOCD on Linux), etc.; and (c) we don't expose
new users to so many new concepts at once (cross compiling and debugging vs that
plus "what is OpenOCD even doing?", "what is this interface/*.cfg file?", etc).
Basically, newcomers will have an easier time all around. The downside is that
there would be a new dependency to install: QEMU.

@japaric japaric requested a review from a team as a code owner August 29, 2018 23:01
@korken89
Copy link
Contributor

Overall this LGTM, good update!

But I have one small nitpick.
In my opinion the QEMU parts are more fit for the book, learning how to simulate an MCU is not the purpose of a quickstart guide. I'd however agree that a link to an emulation section in the book would be useful for those interested.

@therealprof
Copy link
Contributor

@japaric Sounds good to me, I like it.

@japaric
Copy link
Member Author

japaric commented Sep 4, 2018

@sekineh hey. I just want to inform you that I'll be adding a section about running bare metal programs with QEMU to the embedded book based on what I've written in this PR. I know you have been working with QEMU recently so I'm letting you know in case you want to, at any time, share with us any cool tricks you have learned or give feedback on the text. I'll be including the exit syscall stuff you showed me in the revised version of this PR's text.

@japaric
Copy link
Member Author

japaric commented Sep 8, 2018

The latest commit upgrades all the examples to Rust 2018 and moves out the text to the book. See rust-embedded/book#20

therealprof
therealprof previously approved these changes Sep 8, 2018
Copy link
Contributor

@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.

LGTM

korken89
korken89 previously approved these changes Sep 8, 2018
Copy link
Contributor

@korken89 korken89 left a comment

Choose a reason for hiding this comment

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

+1

entry!(main);
use cortex_m_rt::entry;
use cortex_m_semihosting::hio::{self, HStdout};
use stm32f30x::{interrupt, Interrupt};
Copy link
Contributor

@therealprof therealprof Sep 8, 2018

Choose a reason for hiding this comment

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

I would prefer if we could turn the device specific use lines into generic ones and define the crates to be used in the end in src/lib.rs, aliasing the device specific peripheral registers and HAL to device and hal respectively so (in Rust 2018) those would then read

use crate::device::<whatever>;
use crate::hal::<whatever>;

instead in the examples. This makes it a lot easier to try them on different devices.

Copy link

@sekineh sekineh Oct 1, 2018

Choose a reason for hiding this comment

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

I don’t think example programs should depend on src/lib.rs of this crate.

Users typically use examples in their own crates. In that case, they can’t touch src/lib.rs of cortex-m-rt (Edited).

In other words, I expect I can do the following thing:

  1. copy one of the examples into their own main.rs.
  2. edit cargo.toml .
  3. build successfully

Adding one indirection of ‘use’ clauses will do harm in this use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite see your point here I'm afraid. That's exactly what I would hope to enable by generally using universal names instead of calling device specific crates because those are the ones that one always has to change in each source file. If you want to use an example in your own crate you just have to alias the correct crates in your src/lib.rs and if the hardware is similar enough it will just run.

@japaric japaric dismissed stale reviews from korken89 and therealprof via 728d57d September 14, 2018 17:04
@japaric
Copy link
Member Author

japaric commented Sep 14, 2018

Question: Should we remove all the non-essential files from this repository? That is LICENSE-*, the ci directory, etc. Right now these would appear in projects made using cargo-generate. (To be fair, they also appeared in copies made using cargo-clone.)

Given that the bulk of the documentation will be in the book; this seems fine to me.

Thoughts?

book.rust-embedded.org is pointing to an old version
@therealprof
Copy link
Contributor

@japaric +1

@japaric
Copy link
Member Author

japaric commented Sep 17, 2018

I have minified the repo and bumped the dependencies.

Since there's no clear ETA for the beta I would suggest landing this now. The text has been updated to note that the beta is not out yet.

As there's no .travis.yml anymore I have modified the branch protection to not require bors. One of you will still have to approve this PR before it can be merged (just press the green merge button afterwards).

Copy link
Contributor

@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.

Still LGTM

@japaric japaric merged commit a7f68f4 into master Sep 17, 2018
@japaric japaric deleted the generate branch September 17, 2018 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants