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 timer implementation #5

Merged
merged 7 commits into from
Dec 18, 2018
Merged

Conversation

david-sawatzke
Copy link
Member

@david-sawatzke david-sawatzke commented Dec 18, 2018

@david-sawatzke
Copy link
Member Author

I've also edited the travis config to automatically build the examples

@therealprof
Copy link
Member

@david-sawatzke Nice, I'll have a look later.

NB: I'm planning to move the examples out of this HAL impl crate so don't put too much time into them. ;)

@HarkonenBade
Copy link
Member

Out of interest, why are you planning on removing the examples?

@therealprof
Copy link
Member

@HarkonenBade Because the require appropriate hardware wiring to do anything useful and with the support of multiple chips they're going to be a pain to maintain to work on all of them.

I basically see two options: open up a workspace and put them into a (device specific) sub-crate or simply move them to a (hardware specific) board support crate. Most of the examples are identically available in my https://github.com/therealprof/nucleo-f042k6 crate anyway, the original ones in this crate only exist for historic reasons.

@david-sawatzke
Copy link
Member Author

I don't think this is necessary (at least for the basic examples). You have a baseline of pins and peripherals (gpioa and a few gpiob pins, as well as usart1, i2c1, spi1) that examples can rely on to work (it would require reworking/removing some examples). I like having the examples with the hal, as it gives potential users an easy way to see how something is programmed (instead of having read the currently very lacking documentation) and basing their own projects on that .

@therealprof
Copy link
Member

@david-sawatzke I totally agree that we should have examples, but not necessarily in this crate. The examples were a way for me to test the HAL implementation in the early days on custom hardware (a STM32F042F6P6 on a custom PCB with a few added peripherals) they're not guaranteed to run on anything else and will likely cause interesting (and hard to debug) errors if e.g. the memory.x does not match the available RAM/flash on the chip etc.

The proper way of getting started with a new device is using a BSP crate which provides an useful environment and out-of-the-box working examples for the chosen dev-board or to set up a new custom environment by following the instructions in the book.

I haven't thought too much about the new structure but since people are adding examples here I wanted to give you a heads up.

@therealprof
Copy link
Member

@david-sawatzke

(instead of having read the currently very lacking documentation)

What do you feel is lacking? We should definitely improve documentation wherever possible.

@HarkonenBade
Copy link
Member

Yeah, I can definitely say that I found the examples very useful for working out how to make use of the hal crate. If we were to remove them then i'd be very interested in seeing doc examples per peripheral that show code for using that peripheral in practice. Also potentially a bit of doc somewhere high level showing the normal boilerplate outline.

@HarkonenBade
Copy link
Member

A guide I had been working with for another crate I published was advising that every public function should have doc comments and a snippet of example code. Kinda in the same way that the std lib docs are done.

@therealprof
Copy link
Member

Yeah, I can definitely say that I found the examples very useful for working out how to make use of the hal crate.

Sure. I do find it much more useful to go tell the people: Go buy a Nucleo-F042 and use this BSP crate to get a nice selection of out-of-the-box examples you can directly run on it and use as a template for your own projects instead of people buying random boards using one of the supported chips and having to figure out by themselves how to compile and run some basic examples.

If we were to remove them then i'd be very interested in seeing doc examples per peripheral that show code for using that peripheral in practice.

Absolutely. I'll take PRs turning examples into inline documentation. ;)

Also potentially a bit of doc somewhere high level showing the normal boilerplate outline.

You mean like the embedded book? ;)

@therealprof
Copy link
Member

A guide I had been working with for another crate I published was advising that every public function should have doc comments and a snippet of example code. Kinda in the same way that the std lib docs are done.

That would be fantastic to have, but I do lack the time to do everything on my own and people seem to commit PRs which also don't include them 😉.

@HarkonenBade
Copy link
Member

HarkonenBade commented Dec 18, 2018

Yeah, I can definitely say that I found the examples very useful for working out how to make use of the hal crate.

Sure. I do find it much more useful to go tell the people: Go buy a Nucleo-F042 and use this BSP crate to get a nice selection of out-of-the-box examples you can directly run on it and use as a template for your own projects instead of people buying random boards using one of the supported chips and having to figure out by themselves how to compile and run some basic examples.

Fair, I suppose in my case I had already acquired hardware and was puzzling as to how to get bits of code running. Also more than just 'how to get stuff running on the micro' I found the examples useful for 'so how am I actually meant to use the interface to this peripheral' as with the multiple layers of bits the pure reference docs can be quite difficult to sift through to find how to actually operate something.

If we were to remove them then i'd be very interested in seeing doc examples per peripheral that show code for using that peripheral in practice.

Absolutely. I'll take PRs turning examples into inline documentation. ;)

I will try and put together some sensible bits for the GPIO section soon ish.
Also as a bonus this will give us some doc tests we could potentially run in travis to make sure that stuff still compiles at least.

Also potentially a bit of doc somewhere high level showing the normal boilerplate outline.

You mean like the embedded book? ;)

This is a fair point. Could a link to that be put in the README maybe? It might be nice to have a 'Getting Started' section in the readme that points you at helpful resources.

@therealprof
Copy link
Member

I will try and put together some sensible bits for the GPIO section soon ish.
Also as a bonus this will give us some doc tests we could potentially run in travis to make sure that stuff still compiles at least.

Much appreciated!

This is a fair point. Could a link to that be put in the README maybe? It might be nice to have a 'Getting Started' section in the readme that points you at helpful resources.

Yes. We should start keeping those ideas in tickets since they also would be perfect beginner issues.

@HarkonenBade
Copy link
Member

A guide I had been working with for another crate I published was advising that every public function should have doc comments and a snippet of example code. Kinda in the same way that the std lib docs are done.

That would be fantastic to have, but I do lack the time to do everything on my own and people seem to commit PRs which also don't include them 😉.

If you were feeling evil you could add the #[deny(missing_docs)] attribute to the crate ;)

@david-sawatzke
Copy link
Member Author

I added a bit of documentation to the timers, but I haven't found a way to only build doc tests

@therealprof
Copy link
Member

@david-sawatzke cargo test does not work?

@david-sawatzke
Copy link
Member Author

error[E0463]: can't find crate for `test`

Needs a --doc (rust-lang/cargo#1789)

@therealprof
Copy link
Member

@david-sawatzke I'm seeing the same. But --doc alone doesn't test anything here. This might require some tinkering with Cargo.toml. I don't have time right now but I'd start by looking into other crates how they're doing it.

@HarkonenBade
Copy link
Member

@david-sawatzke I'm seeing the same. But --doc alone doesn't test anything here. This might require some tinkering with Cargo.toml. I don't have time right now but I'd start by looking into other crates how they're doing it.

It would be expected that --doc doesn't actually test anything currently. Given that the test in timers is marked no_run.

@therealprof
Copy link
Member

It would be expected that --doc doesn't actually test anything currently. Given that the test in timers is marked no_run.

no_run means "compile only" which is exactly what we want.

@HarkonenBade
Copy link
Member

hmm, I am also having issues with trying to get doc tests to be actually compiled while writing some bits for the GPIO modules. I can write evidently broken code in a doc test and running cargo test --doc --features="stm32f042 rt" produces no compilation errors.

@david-sawatzke
Copy link
Member Author

I've tested the example and it seems to work fine with tim1 and systick, so it's probably ready to merge (excluding the doc test issues).

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.

LGTM. I'm not too excited about the tons of non-timer related changed in this PR, especially since they try to establish facts I'm not happy about but I'll let this slide this time; please try to stay on topic.

@therealprof therealprof merged commit 7f7a7d6 into stm32-rs:master Dec 18, 2018
@therealprof
Copy link
Member

@HarkonenBade Added a ticket about the doc test problem: rust-lang/cargo#6460

@david-sawatzke
Copy link
Member Author

Sorry about that, I wanted the examples to build for stm32f030 for testing purposes and it kind of grew from there

@HarkonenBade
Copy link
Member

@therealprof Nice, hopefully we get some traction there. Also sorry for contributing to the testing based distraction.

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.

3 participants