Tidy, mainly documentation links

[ianrrees]

Summary

Mostly warning cleanup, a few typos etc.

I'm finding the documentation of feature-gated APIs to be quite awkward... Wondering if it might be easier to add dummy methods/types that are conditional on documentation builds with the feature not enabled, so that rustdoc has something to link to. Eg

#[cfg(all(doc, not(feature = "dma")))]
/// Documentation was built without `dma` feature enabled
pub mod dmac {}

[ianrrees]

For this push, I've abandoned the idea of cleaning up all the cargo doc warnings without the dma and async features enabled. There are quite a lot of warnings aside from the docs for those features, and it's not clear to me what the best approach is for the feature-related docs.

I'm tempted to suggest that we put a CI check in place to flag documentation warnings, though I suppose that would add a little bit of friction to documenting which isn't great...

[jbeaurivage]

No worries. I thought about it a bit, and I feel like it would be beneficial to have all docs enabled all the time, even if a specific feature is disabled. Ie, if the async feature is disabled, it would be nice if the docs would still get built. Maybe in an otherwise empty module, and a message prompting to enable the feature?

I'm not too sure what would be the best approach to achieve this. Thoughts?

[ianrrees]

The main snag I'm seeing, is when there's module level documentation like this:

//! Module level docs talk about the library
//! that has some sort of [`async-method`]
//! and maybe some sort of [`dma-type`]
//! [`async-method`] and [`dma-type`] can be used together!

And code like:

/// A hypothetical method that's not synchronous
#[cfg(feature="async")]
fn async-method() {}

...

#[cfg(feature="dma")]
mod dma {
   /// The type used for DMA
   struct dma-type {}

then cargo doc spits out a lot of warnings about dead links when it's run without those features. One option is to make the docs change according to the features, like this (same cfg_attr approach can handle larger blocks of text):

//! Module level docs talk about the library
//! that has some sort of
#![cfg_attr(not(feature = "async"), doc = "`async-method`")]
#![cfg_attr(feature = "async", doc = "[`async-method`]")]
//! and maybe some sort of
#![cfg_attr(not(feature = "dma"), doc = "`dma-type`")]
#![cfg_attr(feature = "dma", doc = "[`dma-type`]")]
//! #![cfg_attr(not(feature = "async"), doc = "`async-method`")]
#![cfg_attr(feature = "async", doc = "[`async-method`]")]
//! and
#![cfg_attr(not(feature = "dma"), doc = "`dma-type`")]
#![cfg_attr(feature = "dma", doc = "[`dma-type`]")]
can be used together!
//!

Another is to leave the docs unchanged, but add placeholders as in the original comment.

I guess after typing this, I'm wondering what the downside is to mostly leaving out the docs for the feature-gated stuff - in the case of people generating their own docs it's trivial to re-do with features enabled, and we can turn them on in CI.

[ianrrees]

Another related problem: we have some API differences depending on the target chip, so simply enabling gated features isn't quite enough.

[ianrrees]

n.b. we can combine cfg and hal_cfg like this:

    #[cfg(doc)]
    #[hal_cfg("eic-d5x")]
    /// This method is not present with the selected feature set, defined for documentation only
    pub fn into_future(self) {
        unimplemented!()
    }

[ianrrees]

This PR is now to a point where cargo doc --features="samd21g,dma,async" and cargo doc --features="samd51j,dma,async" will run without warnings on both stable and nightly.

I think it would be good to look at the documentation macro as discussed in matrix, before merging this.

n.b. there's a known bug in rustdoc which basically means that we can't reliably link to specific versions of dependencies - links to embedded-hal 0.2 will resolve to embedded-hal 1.0 for instance. I opened a bug for this, but don't get the impression it'll be fixed anytime soon.

[ianrrees]

I've extended the HAL macros to support not(), and used that to make stubs like so:

#[cfg(doc)]
#[hal_cfg(not("sercom0-d5x"))]
/// This trait is not present with the selected feature set, defined for
/// documentation only
pub trait IoSet {}

There was a little bit of discussion on Matrix about whether it's a good idea to support/use not() (we want to keep the configuration stuff as simple as possible); one option that we could consider is to create a new macro for this purpose and remove the not() from the HAL macros API. Eg:

#[hal_doc_not("sercom0-d5x")]
/// This trait is not present with the selected feature set, defined for
/// documentation only
pub trait IoSet {}

Personally, I feel this is a bit too deep in the weeds.

[ianrrees]

Now that we've got CI documentation generation working again, I'm keen to finish up this work. Just a few questions/tasks:

• Confirm we're OK with the macro changes, and using them to enable methods/types for documentation.
• Confirm the documentation changes look OK. Given the underlying Cargo/rustdoc issues prevent us from linking docs to embedded-hal v0.2 and v1.0 separately, some of the changes are a little academic but others probably not so much.
• Once the above two are resolved, I'd clean up the Git history and sort whatever rustfmt is complaining about.

[ianrrees]

@bradleyharden if you've got a chunk of time to skim over this, it would be nice to get the thumbs-up on the macro changes (and I think the only wording changes were in SERCOMv2 docs but it's been a while)

[jbeaurivage]

Hey @ianrrees, sorry I was just now able to get around to this. Some thoughts I have:

• I don't really have a problem with the new not() macro. It can be useful, and while it has the potential to introduce more complexity if misused, I think it's just up to us as contributors and maintainers to stay disciplined about how we handle inter-peripheral differences.
• I must admit, I'm not a huge fan of adding traits/methods/types just to satisfy rustdoc. Idk, something just feels off and untidy about it for some reason; I think it's the fact that the tooling that is supposed to help us is instead adding a burden and extra complexity. I don't think I have an alternative solution just yet... I just think we should consider whether it's worth it to have all these dummy items.
• Apart than that strange situation we're in, I'm happy with the other doc changes, thank you!
• If we manage to completely satisfy any rustdoc warnings, we'll definitely want to add a check in CI to prevent them from getting out of hand again.

[jbeaurivage]

Of course, another option is to merge this as-is while we look for a better solution (if we want to). We can always remove the dummy items later, I wouldn't consider their removal a breaking change since we don't consider them a part of the public API.

[ianrrees]

Yeah, I tend to agree about the dummy items being a bit ugly, however I don't think they have a negative impact other than adding a little bit of clutter. TBH, there are a couple things about the way Cargo/rustdoc handle documentation that aren't a great fit for our project, and to me this the dummy items seem like not the worst of those - I'd say the thing about not being able to document multiple versions of a crate with the same name is worse. The dummy items are the lest offensive of the options I could think of:

• Remove links to feature-gated items from our documentation
• Add dummy items
• Suppress warnings about broken links
• Ignore warnings about broken links

I'm also in favour of CI building docs and flagging any introduced issues with the documentation, so I don't think ignoring the warnings is viable once that's in place.

[jbeaurivage]

Sounds good to me. Did you want to add the extra CI doc checks in this PR? Otherwise I'm ready to merge when everything will be lined up.

[ianrrees]

I'm probably going to be a bit short on atsamd time for the next while, so would rather not hold this up for CI documentation checks. Unless, of course, someone else wants to do it :) . This work only fixes the doc warnings for some particular combinations of features, so there could be a little more docstring tidy-up to do as part of that CI work, depending on how the check is implemented.

This one might be best merged without squashing, so we can revert the dummy types more easily if we come up with a better solution.