From b17222b32dea2f7cf6357b5d53bd1c27687cb73f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 7 Nov 2024 15:33:50 +0100 Subject: [PATCH 1/3] Tweak guidelines --- documentation/API-GUIDELINES.md | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/documentation/API-GUIDELINES.md b/documentation/API-GUIDELINES.md index b2e18e13eda..0827ae53de1 100644 --- a/documentation/API-GUIDELINES.md +++ b/documentation/API-GUIDELINES.md @@ -15,9 +15,19 @@ 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_pin_name(self, pin: impl Peripheral

+ 'd) -> Self)` or `fn with_pin_name(self, pin: impl Peripheral

+ 'd) -> Self)` +- If a driver supports multiple peripheral instances: + - 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 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. + - `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. ## Interoperability @@ -25,6 +35,7 @@ The following paragraphs contain additional recommendations. - 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::*`)! +- Prefer implementing common ecosystem traits, like the ones in `embedded-hal` or `embassy-embedded-hal`. ## API Surface @@ -45,6 +56,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. ## Maintainability @@ -56,6 +70,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: @@ -88,4 +111,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. From 8077170f217510f3037d2058a5060df01cf29cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 7 Nov 2024 16:31:47 +0100 Subject: [PATCH 2/3] Restore builder rule --- documentation/API-GUIDELINES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/documentation/API-GUIDELINES.md b/documentation/API-GUIDELINES.md index 0827ae53de1..90a5344b5eb 100644 --- a/documentation/API-GUIDELINES.md +++ b/documentation/API-GUIDELINES.md @@ -28,6 +28,7 @@ The following paragraphs contain additional recommendations. - `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 driver construction. ## Interoperability From 10bfcaf209cba5bb1ad879c3325dbf3e1188dbb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 7 Nov 2024 17:32:13 +0100 Subject: [PATCH 3/3] Address review comments --- documentation/API-GUIDELINES.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/documentation/API-GUIDELINES.md b/documentation/API-GUIDELINES.md index 90a5344b5eb..c0336070789 100644 --- a/documentation/API-GUIDELINES.md +++ b/documentation/API-GUIDELINES.md @@ -16,12 +16,13 @@ The following paragraphs contain additional recommendations. ## Construction and Destruction of Drivers - 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_pin_name(self, pin: impl Peripheral

+ 'd) -> Self)` or `fn with_pin_name(self, pin: impl Peripheral

+ 'd) -> Self)` -- If a driver supports multiple peripheral instances: +- If a driver requires pins, those pins should be configured using `fn with_signal_name(self, pin: impl Peripheral

+ 'd) -> Self)` or `fn with_signal_name(self, pin: impl Peripheral

+ '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. @@ -36,7 +37,7 @@ The following paragraphs contain additional recommendations. - 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::*`)! -- Prefer implementing common ecosystem traits, like the ones in `embedded-hal` or `embassy-embedded-hal`. +- Consider implementing common ecosystem traits, like the ones in `embedded-hal` or `embassy-embedded-hal`. ## API Surface