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

Tweak guidelines #2482

Merged
merged 3 commits into from
Nov 8, 2024
Merged
Changes from all commits
Commits
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
32 changes: 29 additions & 3 deletions documentation/API-GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,29 @@ The following paragraphs contain additional recommendations.

## Construction and Destruction of Drivers

- Drivers take peripherals and pins via the `PeripheralRef` pattern - they don't consume peripherals/pins.
- Drivers should take peripherals via the `PeripheralRef` pattern - they don't consume peripherals directly.
- If a driver requires pins, those pins should be configured using `fn with_signal_name(self, pin: impl Peripheral<P = InputConnection> + 'd) -> Self)` or `fn with_signal_name(self, pin: impl Peripheral<P = OutputConnection> + 'd) -> Self)`
- If a driver supports multiple peripheral instances (for example, I2C0 is one such instance):
- The peripheral instance type should be positioned as the last type parameter of the driver type.
- The peripheral instance type should default to a type that supports any of the peripheral instances.
- The author is encouraged to use `crate::any_peripheral` to define the "any" peripheral instance type.
- The driver should implement a `new` constructor that automatically converts the peripheral instance into the any type, and a `new_typed` that preserves the peripheral type.
- If a driver only supports a single peripheral instance, no instance type parameter is necessary.
- 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 should include a `crate::Mode` type parameter.
- By default, constructors should configure the driver for blocking mode. The driver should implement `into_async` (and a matching `into_blocking`) function that reconfigures the driver.
- `into_async` should configure the driver and/or the associated DMA channels. This most often means enabling an interrupt handler.
bugadani marked this conversation as resolved.
Show resolved Hide resolved
- `into_blocking` should undo the configuration done by `into_async`.
- The asynchronous driver implemntation should also expose the blocking methods (except for interrupt related functions).
- Consider adding a `Drop` implementation resetting the peripheral to idle state.
- Consider using a builder-like pattern for configuration which must be done during initialization.
MabezDev marked this conversation as resolved.
Show resolved Hide resolved
- 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`.

## API Surface

Expand All @@ -45,6 +58,9 @@ The following paragraphs contain additional recommendations.
- For example starting a timer is fine for `&self`, worst case a timer will be started twice if two parts of the program call it. You can see a real example of this [here](https://github.com/esp-rs/esp-hal/pull/1500#pullrequestreview-2015911974)
- 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.
Dominaezzz marked this conversation as resolved.
Show resolved Hide resolved

## Maintainability

Expand All @@ -56,6 +72,15 @@ The following paragraphs contain additional recommendations.
- All `Future` objects (public or private) must be marked with ``#[must_use = "futures do nothing unless you `.await` or poll them"]``.
- Prefer `cfg_if!` over multiple exclusive `#[cfg]` attributes. `cfg_if!` visually divides the options, often results in simpler conditions and simplifies adding new branches in the future.

## 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:
Expand Down Expand Up @@ -88,4 +113,5 @@ Modules should have the following documentation format:
```
#![doc = concat!("[ESP-IDF documentation](https://docs.espressif.com/projects/esp-idf/en/latest/", crate::soc::chip!(), "/api-reference/peripherals/etm.html)")]
```
- Documentation examples should be short and basic, for more complex scenarios, create an example.
- 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 should be short and basic. For more complex scenarios, create an example.