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

5 add docs for crate usage #14

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

HusseinAbdelhamid
Copy link
Contributor

No description provided.

@HusseinAbdelhamid HusseinAbdelhamid linked an issue Sep 3, 2024 that may be closed by this pull request
@HusseinAbdelhamid
Copy link
Contributor Author

Should wait till PR #16 and #18 are merged first. Then will adjust the doc examples accordingly

Copy link
Member

@marius-meissner marius-meissner left a comment

Choose a reason for hiding this comment

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

A good start and nice examples. But the docs should be reorganized a bit:

  1. Move the docs of the can module to crate level, as these act as a nice starting point for the user
  2. Add comment description to all the modules
  3. Add shorter + deeper descriptions and examples to the corresponding modules. In detail:
    a) can module: Explain how to configure the CAN controller (e.g. FIFO configuraiton)
    b) message module: Explain the two CAN types (CAN FD and CAN 2.0) and add short examples (just how to construct them)

Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated
@@ -11,6 +11,8 @@ pub mod status;

pub mod filter;
pub mod message;

pub mod example;
Copy link
Member

Choose a reason for hiding this comment

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

Please add an example feature flag for. So the library user has the ability to remove the example code for prod. usage.

See LTC681X crate as reference:
https://github.com/atlas-aero/rt-LTC681X/blob/6c4407091672e7dd2e91dda18d0a686246e4fbf2/Cargo.toml#L25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added now, example feature included by default as in rt-LTC681x crate

@HusseinAbdelhamid
Copy link
Contributor Author

HusseinAbdelhamid commented Sep 5, 2024

  1. Move the docs of the can module to crate level, as these act as a nice starting point for the user

So the doc example should be in a separate file?

@marius-meissner
Copy link
Member

So the doc example should be in a separate file?

The intent was, to move the docs of can.rs to lib.rs. So that these docs are shown on the first page of the docs.

Furthermore, you might combine the RX and TX example into one. It is common practice that only one complete example is shown on the first documentation page. And then detailed examples are shown in the individual sub-pages or modules.

@HusseinAbdelhamid
Copy link
Contributor Author

Should be ready for another review now, I included the changes from #20 as well.

Copy link
Member

@marius-meissner marius-meissner left a comment

Choose a reason for hiding this comment

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

Really nicely elaborated documentation that will make it easier for both internal and external developers to get started.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,121 @@
use alloc::vec;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a short description, that the example code may be removed by disabling the example feature flag.

src/can.rs Outdated Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/message.rs Outdated Show resolved Hide resolved
@marius-meissner
Copy link
Member

Any updates on this?
As soon this PR is done, we could release the first version to crates.io. I assume this will help with development of our internal software.

@HusseinAbdelhamid
Copy link
Contributor Author

Any updates on this? As soon this PR is done, we could release the first version to crates.io. I assume this will help with development of our internal software.

It's close to being finished, just need to update docs to match latest commits

@HusseinAbdelhamid
Copy link
Contributor Author

Should be updated with latest PRs, added a couple of comments and an example for constructing a CAN FD message as well

Copy link
Member

@marius-meissner marius-meissner left a comment

Choose a reason for hiding this comment

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

src/message.rs Outdated
Comment on lines 25 to 29
//!
//!use bytes::Bytes;
//!use mcp2517::message::{CanFd,TxMessage};
//!use embedded_can::{Id,StandardId};
//!
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//!
//!use bytes::Bytes;
//!use mcp2517::message::{CanFd,TxMessage};
//!use embedded_can::{Id,StandardId};
//!
//!# use bytes::Bytes;
//!# use mcp2517::message::{CanFd,TxMessage};
//!# use embedded_can::{Id,StandardId};
//!#

@HusseinAbdelhamid
Copy link
Contributor Author

HusseinAbdelhamid commented Oct 1, 2024

Comments addressed now, should be ready for review

Copy link
Member

@marius-meissner marius-meissner left a comment

Choose a reason for hiding this comment

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

99% done, the only previous open comment:
#14 (comment)

src/lib.rs Outdated Show resolved Hide resolved
src/message.rs Outdated Show resolved Hide resolved
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.

Add docs for crate usage
2 participants