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

improve rcc and gpio docs #117

Merged
merged 6 commits into from
Aug 7, 2020
Merged

Conversation

TheZoq2
Copy link
Member

@TheZoq2 TheZoq2 commented Jul 20, 2020

This adds some documentation to the rcc and gpio documentation to make it clear how to aquire instances of their structs. Similar to what the f1 crate already has.

I don't have access to an f3 at the moment, so some of this may be wrong ;)

@teskje teskje self-requested a review July 20, 2020 11:24
Copy link
Collaborator

@strom-und-spiele strom-und-spiele left a comment

Choose a reason for hiding this comment

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

Nice project. Small code examples always help along.

However I'm not so sure about the documentation of the registers in the rcc module.
I was wondering what one gets from reading those.
If I see ahb being used in code I don't need this explanation (it's already there in the code)
If I'm fairly familiar with the HAL-doc, the headline gives me (almost all) the information I need. If I'm not familiar with the HAL-doc this would help. But then again I'd have to write an explanation like this for every struct in the docs, bloating it with redundant information.
Maybe a small introduction on "how to read the docs" is more efficient?

When building the docs to read it in context I also noticed that the index page is not really welcoming. Maybe improving this could be part of this PR and also a nice place to add said chapter.

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jul 20, 2020

I was wondering what one gets from reading those.

The way I tend to read the docs for HALs, and/or how I want people to read them, is to pick some feature they want to use and find a function that accomplishes that.

For example, someone wants to toggle a pin so they find the GPIO struct. The docs there tell you to call the split function, it takes AHB as input. However, the autogenerated docs don't tell you how to get your hand on AHB, which is what this adds.

In the F1 HAL, which I am much more familiar with, I kept running into the problem of not remembering where MAPR comes from. It sounds like it might be RCC, but it is not. For someone who isn't familiar with the crate at all, it would be even more difficult.

A "how to read the docs" section would be nice, but it would require people remembering where each struct comes from. Though perhaps one could link to that section instead of addding a bunch of dp = pac::Peripherals.take everywhere

Copy link
Collaborator

@teskje teskje left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Just a few minor nits.

@teskje
Copy link
Collaborator

teskje commented Jul 20, 2020

I see how the RCC docs are debatable. It would be nice if there was a better way to figure out from looking at the docs how a type can be constructed. And there is the potential issue of the docs becoming bloated and repetitive if we document that everywhere.

However, keep in mind that the stm32f3xx-hal is probably the one code base that most embedded Rust beginners interact with very early in their learning curve, since that's what's used by the Discovery book. As such I think it doesn't hurt to err on the side of having "too much" documentation. I would also argue that in general we are pretty far away from having too much documentation ;) As such I welcome this change.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Jul 20, 2020

Doc-Improvments are always much appreciated. I feel the same way, that the repetitive comments in the rcc module feel more like fighting against the limitation of documentation generation.

But these additions are rather small and I really appreciate these additions as it is really easy to get lost in the generated documentation. And RCC is the module everyone has to use, when working with this crate, which makes good and comprehensible documentation even more important.

@TheZoq2 TheZoq2 force-pushed the doc_improvements branch from 5d7d6a2 to e2c3fa5 Compare July 20, 2020 21:30
@TheZoq2
Copy link
Member Author

TheZoq2 commented Jul 20, 2020

Updated with the suggested length changes (though the last line is still quite long because of the URL.

For now, I added a link to the examples on this repo at the currently released version. This is what I do in the f1 hal as well, though it's not without flaws:

  • Requires updating on release. All the places where that's required can be found fairly easily using grep v0.4.3 on every release though
  • Will not be correct when people depend on the git version

@TheZoq2
Copy link
Member Author

TheZoq2 commented Jul 21, 2020

Oops, fixed :)

Copy link
Member

@Sh3Rm4n Sh3Rm4n left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Rebase needed to retrigger the CI, I guess?

@teskje
Copy link
Collaborator

teskje commented Jul 25, 2020

Hey @TheZoq2, there are still two open suggestions, have you seen these? They are pretty nitpicky so I'd be fine with merging as is. Just wanted to make sure you didn't miss them.

Co-authored-by: Jan Teske <jteske@posteo.net>
@TheZoq2
Copy link
Member Author

TheZoq2 commented Aug 1, 2020

I only see one open suggestion which I now fixed, did I miss the other?

@teskje teskje merged commit 4259e42 into stm32-rs:master Aug 7, 2020
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.

4 participants