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.

Sign up for GitHub

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

Jump to bottom

Amend rust guidelines #2617

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Amend rust guidelines #2617

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions documentation/API-GUIDELINES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,19 @@ This is a living document - make sure to check the latest version of this docume
> [!NOTE]
> Not all of the currently existing code follows this guideline, yet.

In general, the [Rust API Guidelines](https://rust-lang.github.io/api-guidelines) apply to all projects in the ESP-RS GitHub organization where possible. (`C-RW-VALUE` and `C-SERDE` do not apply)
In general, the [Rust API Guidelines](https://rust-lang.github.io/api-guidelines) apply to all projects in the ESP-RS GitHub organization where possible.
- Especially for public API but if possible also for internal APIs.

Especially for public API but if possible also for internal APIs.
## Amendments to the Rust API Guidelines

The following paragraphs contain additional recommendations.
- `C-RW-VALUE` and `C-SERDE` do not apply.
- `C-COMMON-TRAITS`:
The set of traits to implement depend on the type. If nothing here applies, use your best judgement.
- Driver structures: `Debug/Display`, `PartialEq/Eq`
- Driver configuration: `Default`, `Debug/Display`, `PartialEq/Eq`, `Clone/Copy`, `Hash`
- `Clone/Copy` depends on the size and contents of the structure. They should generally be implemented, unless there is a good reason not to.
- The `Default` configuration needs to make sense for a particular driver, and applying the default configuration must not fail.
- Error types: `Debug/Display`, `PartialEq/Eq`, `Clone/Copy`, `Hash`

## Construction and Destruction of Drivers

Expand All @@ -29,17 +37,19 @@ The following paragraphs contain additional recommendations.
- `into_blocking` must undo the configuration done by `into_async`.
- The asynchronous driver implemntation must also expose the blocking methods (except for interrupt related functions).
- Drivers must have a `Drop` implementation resetting the peripheral to idle state. There are some exceptions to this:
- GPIO where common usage is to "set and drop" so they can't be changed
- GPIO where common usage is to "set and drop" so they can't be changed
- Where we don't want to disable the peripheral as it's used internally, for example SYSTIMER is used by `time::now()` API. See `KEEP_ENABLED` in src/system.rs
- Consider using a builder-like pattern for driver construction.

## Interoperability

- `cfg` gated `defmt` derives and impls are added to new structs and enums.
- see [this example](https://github.com/esp-rs/esp-hal/blob/df2b7bd8472cc1d18db0d9441156575570f59bb3/esp-hal/src/spi/mod.rs#L15)
- e.g. `#[cfg_attr(feature = "defmt", derive(defmt::Format))]`
- Don't use `log::XXX!` macros directly - use the wrappers in `fmt.rs` (e.g. just `info!` instead of `log::info!` or importing `log::*`)!
- Consider implementing common ecosystem traits, like the ones in `embedded-hal` or `embassy-embedded-hal`.
- Where the guidelines suggest implementing `Debug`, `defmt::Format` should also be implemented.
- The `defmt::Format` implementation needs to be gated behind the `defmt` feature.
- see [this example](https://github.com/esp-rs/esp-hal/blob/df2b7bd8472cc1d18db0d9441156575570f59bb3/esp-hal/src/spi/mod.rs#L15)
- e.g. `#[cfg_attr(feature = "defmt", derive(defmt::Format))]`
- Implementations of common, but unstable traits (e.g. `embassy_embedded_hal::SetConfig`) need to be gated with the `unstable` feature.

## API Surface

Expand All @@ -54,8 +64,8 @@ The following paragraphs contain additional recommendations.
- Design APIs in a way that they are easy to use.
- Driver API decisions should be assessed individually, don't _not_ just follow embedded-hal or other ecosystem trait crates. Expose the capabilities of the hardware. (Ecosystem traits are implemented on top of the inherent API)
- Avoid type states and extraneous generics whenever possible
- These often lead to usability problems, and tend to just complicate things needlessly - sometimes it can be a good tradeoff to make a type not ZST
- Common cases of useless type info is storing pin information - this is usually not required after configuring the pins and will bloat the complexity of the type massively. When following the `PeripheralRef` pattern it's not needed in order to keep users from re-using the pin while in use
- These often lead to usability problems, and tend to just complicate things needlessly - sometimes it can be a good tradeoff to make a type not ZST
- Common cases of useless type info is storing pin information - this is usually not required after configuring the pins and will bloat the complexity of the type massively. When following the `PeripheralRef` pattern it's not needed in order to keep users from re-using the pin while in use
- Avoiding `&mut self` when `&self` is safe to use. `&self` is generally easier to use as an API. Typical applications of this are where the methods just do writes to registers which don't have side effects.
- Maintain order consistency in the API, such as in the case of pairs like RX/TX.
- If your driver provides a way to listen for interrupts, the interrupts should be listed in a `derive(EnumSetType)` enum as opposed to one function per interrupt flag.
Expand Down