discovery icon indicating copy to clipboard operation
discovery copied to clipboard

sugestions for 5 roulette

Open DaQue opened this issue 7 years ago • 9 comments

First I'm a absolute beginner so I hope you understand these suggestions are to help other absolute beginners and probably seem obvious to a more advanced user and do not imply something is "broken".

First it It could be made clear the the first build of the src/05-led-roulette/main.rs does not actually make the actual roulette led spin as in the photo when compiled and uploaded. Its something you work up to by the end of the chapter. I flashed the src/05-led-roulette/main.rs and spent a lot of time trying to figure out what I did wrong because I didn't see the roulette leds spinning output. I looked at the source and had no idea how it could make it do that.

Second the warning "NOTE Be sure to compile this crate without optimizations" is 100% accurate but explaining that the provided Config.toml file does not enable optimizations so don't change it would have been helpful.

Next, I think this one may already be fixed in the next release by looking at the source to rust-embedded/discovery is I had to change (gdb) break led_roulette::main to (gdb) break main.

A confidence building option for something to add before the roulette chapter would be a super simple just build a supplied blink one led main.rs file and flash and run it using a script if possible. Next have them change one variable for a delay (i.e blink much faster) so you can tell you modified the program and everything still works. This would handle making sure openOCD and gdb worked with out any of the details to be filled in later.

DaQue avatar Oct 05 '18 07:10 DaQue

@DaQue These are good suggestions. Would you like to take a spin at pull requests addressing these shortcomings?

therealprof avatar Oct 05 '18 07:10 therealprof

Actually I've never done a pull request and I am not confident that the specific document updates I would propose would be up to the over all quality of the project.

DaQue avatar Oct 05 '18 07:10 DaQue

Do not be afraid, there's always a first time and I'll be happy to guide you.

therealprof avatar Oct 05 '18 07:10 therealprof

I'll look up what's actually goes in a pill request and give it a go this weekend. I've talked to you on iirc and may chat about it there.

DaQue avatar Oct 05 '18 10:10 DaQue

@DaQue It's not too complicated, especially with documentation.

Basically you'll have to fork the repository, then you click on the file you'd like to modify, click on the pen icon in the upper right, make the changes you desire (and you can check correct rendering by switching between edit and view mode on the fly), the you'll go to the bottom of the edit page where you can commit your changes and create a branch. Once the change is committed and the branch branched, GitHub will suggest you submit a pull request. You simply click the button, give the change a useful title and maybe a short explanation and submit.

Don't worry, it's completely non-destructive.

therealprof avatar Oct 05 '18 12:10 therealprof

Just checking. It looks like the two pull requested I did for this got submitted ok. Let me know if not. I’ll review my original comments to see if I addressed all of them I could.

DaQue avatar Oct 06 '18 20:10 DaQue

@DaQue I see only 1 PR (https://github.com/rust-embedded/discovery/pull/118) which already got merged.

therealprof avatar Oct 06 '18 21:10 therealprof

ok I'm not sure what I did differently. I deleted my old fork repository and made a new one with some minor changes about optimizations. I think I did it right this time. You gave good directions, I guess I just had to read them again.

DaQue avatar Oct 07 '18 04:10 DaQue

No need to delete the forks, after a PR is merged you'll automatically get the options to delete the branch (in the PR) so you don't have outdated branches hanging around.

therealprof avatar Oct 07 '18 09:10 therealprof