# `esp-hal` developers' guidelines

## About

This is a living document - make sure to check the latest version of this document.

> [!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.
  - Especially for public API but if possible also for internal APIs.

## Amendments to the Rust API Guidelines

- `C-RW-VALUE` and `C-SERDE` do not apply.
- `C-COMMON-TRAITS`:
  The set of traits to implement depend on the type and use case. In esp-hal, we can highlight a few such use cases and provide recommendations what should be implemented. If nothing here applies, use your best judgement.
  - Driver structures: `Debug`
  - Driver configuration: `Default`, `Debug`, `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`, `PartialEq/Eq`, `Clone/Copy`, `Hash`, `Error`, `Display`

## Construction and Destruction of Drivers

- If a driver requires pins, those pins should be configured using `fn with_signal_name(self, pin: impl PeripheralInput<'d>) -> Self` or `fn with_signal_name(self, pin: PeripheralOutput<'d>) -> Self`
  - The `with_signal_name` functions should be placed in an `impl` block that is generic over the driver mode (i.e. they shouldn't be constrained to just `Blocking` or just `Async`) unless there is a technical reason to do otherwise.
- If a driver supports multiple peripheral instances (for example, I2C0 is one such instance):
  - The driver should not be generic over the peripheral instance.
  - The author must to use `crate::any_peripheral` to define the "any" peripheral instance type.
  - The driver must implement a `new` constructor that automatically converts the peripheral instance into the any type.
    - The `Instance` trait should have an `IntoAnyPeripheral` supertrait. `IntoAnyPeripheral` is generated by the `any_peripheral!` macro. The actual name of the trait depends on the name of the `AnyPeripheral`.
    - The constructor should take `fn new(peri: impl Instance + 'd)` and call `degrade()` to type-erase the peripheral.
- If a driver is configurable, configuration options should be implemented as a `Config` struct in the same module where the driver is located.
  - The driver's constructor should take the config struct by value, and it should return `Result<Self, ConfigError>`.
  - The `ConfigError` enum should be separate from other `Error` enums used by the driver.
  - The driver should implement `fn apply_config(&mut self, config: &Config) -> Result<(), ConfigError>`.
  - In case the driver's configuration is infallible (all possible combinations of options are supported by the hardware), the `ConfigError` should be implemented as an empty `enum`.
  - Configuration structs should
    - Use the [Builder Lite](https://matklad.github.io/2022/05/29/builder-lite.html) pattern. Prefer deriving `procmacros::BuilderLite` in order to automatically implement the pattern.
    - Implement `Copy` if possible.
    - The fields should be private. The `BuilderLite` macro generates setters and getters automatically.
    - If the configuration is expected to be applied more than once (as, for example, different device configurations on a shared SPI bus may be) and calculating register values based on the configuration is costly, the configuration struct should precompute and store the result of those calculations.
- If a driver implements both blocking and async operations, or only implements blocking operations, but may support asynchronous ones in the future, the driver's type signature must include a `crate::Mode` type parameter.
- By default, constructors must configure the driver for blocking mode. The driver must implement `into_async` (and a matching `into_blocking`) function that reconfigures the driver.
  - `into_async` must configure the driver and/or the associated DMA channels. This most often means enabling an interrupt handler.
  - `into_blocking` must undo the configuration done by `into_async`.
- The asynchronous driver implementation 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
  - 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
  - A driver doesn't need to do anything special for deinitialization and has a `PeripheralGuard` field which implements the disabling and resetting of the peripheral.
- Consider using a builder-like pattern for driver construction.

## Interoperability

- 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

- API documentation must be provided for every new driver and API.
- Private details should not leak into the public API, and should be made private where technically possible.
  - Implementation details that _need_ to be public should be marked with `#[doc(hidden)]` and a comment as to why it needs to be public.
    - For the time being, this includes any `Instance` traits, and `State` or `Info` structs as well.
  - Functions which technically need to be public but shouldn't be callable by the user need to be sealed.
    - see [this example in Rust's core library](https://github.com/rust-lang/rust/blob/044a28a4091f2e1a5883f7fa990223f8b200a2cd/library/core/src/error.rs#L89-L100)
- Any public traits, that **must not** be implemented downstream need to be `Sealed`.
- Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics.
- Follow naming conventions in order to be consistent across drivers - take inspiration from existing drivers.
- 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.
- 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.
- If a driver only implements a subset of a peripheral's capabilities, it should be placed in the `peripheral::subcategory` module.
  - For example, if a driver implements the slave-mode I2C driver, it should be placed into `i2c::slave`.
  - This helps us reducing the need of introducing breaking changes if we implement additional functionalities.
- Avoid abbreviations and contractions in the API, where possible.
  - Saving a few characters may introduce ambiguity, e.g `SpiTransDone`, is it `Transmit` or `Transfer`?
  - Common abbreviations, that are well understood such as `Dma` are perfectly fine.
- Config options should not affect the public API.
  - If you add a new configuration option using `esp-config`, the added option should not alter the public API of the package being configured.

## Maintainability

- Avoid excessive use of macros unless there is no other option; modification of the PAC crates should be considered before resorting to macros.
- Every line of code is a liability. Take some time to see if your implementation can be simplified before opening a PR.
- If you are porting code from ESP-IDF (or anything else), please include a link WITH the commit hash in it, and please highlight the relevant line(s) of code
- If necessary provide further context as comments (consider linking to code, PRs, TRM - make sure to use permanent links, e.g. include the hash when linking to a Git repository, include the revision, page number etc. when linking to TRMs)
- Prefer line comments (//) to block comments (/* ... */)
- Generally, follow common "good practices" and idiomatic Rust style
- All `Future` objects (public or private) must be marked with ``#[must_use = "futures do nothing unless you `.await` or poll them"]``.
- Prefer `cfg_if!` (or, if the branches just pick between separate values of the same variable, `cfg!()`) over multiple exclusive `#[cfg]` attributes. `cfg_if!`/`cfg!()` visually divide the options, often results in simpler conditions and simplifies adding new branches in the future.
- When marking an API as `unstable`:
  - Prefer to use `#[instability::unstable]`.
  - Use the attribute on each public function instead of inherent impl blocks.
- The documentation should contain no more than three primary impl blocks (excluding trait implementations):
  - Blocking: Should be listed first, as it serves as the entry point for creating most drivers.
  - Async: Should appear second in the documentation.
  - Both: A combined impl block for both Blocking and Async.
- Methods inside the impl blocks should be grouped by functionality.

## Driver implementation

- If a common `Instance` trait is used for multiple peripherals, those traits should not have any logic implemented in them.
- The `Instance` traits should only be used to access information about a peripheral instance.
- The internal implementation of the driver should be non-generic over the peripheral instance. This helps the compiler produce smaller code.
- The author is encouraged to return a static shared reference to an `Info` and a `State` structure from the `Instance` trait.
  - The `Info` struct should describe the peripheral. Do not use any interior mutability.
  - The `State` struct should contain counters, wakers and other, mutable state. As this is accessed via a shared reference, interior mutability and atomic variables are preferred.

## Modules Documentation

Modules should have the following documentation format:
```rust
//! # Peripheral Name (Peripheral Acronym)
//!
//! ## Overview
//! Small description of the peripheral, see ESP-IDF docs or TRM
//!
//! ## Configuration
//! Explain how can the peripheral be configured, and which parameters can be configured
//!
//! ## Usage
//! Explain if we implement any external traits
//!
//! ## Examples
//!
//! ### Name of the Example
//! Small description of the example if needed
//! ```rust, no_run
#[doc = crate::before_snippet!()]
//! ...
//! # Ok(())
//! # }
//! ```
//!
//! ## Implementation State
//! List unsupported features
```
- If any of the headers is empty, remove it
- When possible, use ESP-IDF docs and TRM as references and include links if possible.
  - In case of referencing an ESP-IDF link make it chip-specific, for example:
    ```
    #![doc = concat!("[ESP-IDF documentation](https://docs.espressif.com/projects/esp-idf/en/latest/", crate::soc::chip!(), "/api-reference/peripherals/etm.html)")]
    ```
  - In case of referencing a TRM chapter, use the `crate::trm_markdown_link!()` macro. If you are referring to a particular chapter, you may use `crate::trm_markdown_link!("#chapter_anchor")`.
- Documentation examples must be short
  - But must also provide value beyond what the rustdoc generated docs show
    - Showing a snippet of a slightly more complex interaction, for example inverting the signals for a driver
    - Showing construction if it is more complex, or requires some non-obvious precursor steps. Think about this for drivers that take a generic instance to construct, rustdoc doesn't do a good job of showing what concrete things can be passed into a constructor.
  - they are `no_run` - we cannot run them on a target but we can make sure the contained code compiles
  - avoid `unwrap` etc. - use the `?` operator where possible
  - the `before_snippet!()` provides some necessary boiler plate for you
  - For more complex scenarios, create an example.
- Use rustdoc syntax for linking to other documentation items instead of markdown links where possible
  - https://doc.rust-lang.org/rustdoc/write-documentation/linking-to-items-by-name.html

## Breaking changes

We check our stable API surface using semver-checks. To facilitate these checks, we do a number of preprocessing steps to ensure we're only checking _our_ stable API. Therefore the API baseline is stored in `$krate/api-baseline/`. We may want to allow breaking changes, in the case of fixing soundness issues etc. In this case, all that is required is to simply run `cargo xcheck semver-check generate-baseline`.

### Compiler updates

Compiler updates force the regeneration of the API baseline due to the dependency on the unstable rustdoc format. Regeneration **must** occur on the same commit as the previous generation in this case, to ensure no breaking changes are silently introduced. Checkout the last commit of generation, generate the baseline and move them out the repo. Checkout your PR branch and add them back there.