From ba5d4730201adbebff14eba4a984a6d1365286e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Wed, 27 Nov 2024 13:03:06 +0100 Subject: [PATCH] Amend rust guidelines --- documentation/API-GUIDELINES.md | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/documentation/API-GUIDELINES.md b/documentation/API-GUIDELINES.md index 9a01689b5d..d92521a540 100644 --- a/documentation/API-GUIDELINES.md +++ b/documentation/API-GUIDELINES.md @@ -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 @@ -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 @@ -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.