diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 1853c1abd..ce4e6b483 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -7,6 +7,8 @@ To help us review it efficiently, please ensure you've gone through the followin - [ ] I have updated existing examples or added new ones (if applicable). - [ ] I have used `cargo xtask fmt-packages` command to ensure that all changed code is formatted correctly. - [ ] My changes were added to the [`CHANGELOG.md`](https://github.com/esp-rs/esp-hal/blob/main/esp-hal/CHANGELOG.md) in the **_proper_** section. +- [ ] My changes are in accordance to the [esp-rs API guidelines](https://github.com/esp-rs/esp-hal/API-GUIDELINES.md) + #### Extra: - [ ] I have read the [CONTRIBUTING.md guide](https://github.com/esp-rs/esp-hal/blob/main/CONTRIBUTING.md) and followed its instructions. diff --git a/API-GUIDELINES.md b/API-GUIDELINES.md new file mode 100644 index 000000000..82184cbc6 --- /dev/null +++ b/API-GUIDELINES.md @@ -0,0 +1,54 @@ +# esp-rs API 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. (`C-RW-VALUE` and `C-SERDE` do not apply) + +Especially for public API but if possible also for internal APIs. + +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 +- 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 + +- `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::*`) + +## API Surface + +- Add `#[deny(missing_docs)]` to new modules or when reworking a larger part of a module. In the end we will require this for whole crates. +- API documentation shouldn't be an afterthought +- Private details shouldn't 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. +- 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. 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. + - 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) + +## 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 your 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) +- Generally follow common "good practices" and idiomatic Rust style diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 13533a7c7..069ae5fac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,6 +23,8 @@ To get an overview of the project, please read the [README]. Here are some resou ## Getting Started +Before adding or changing code you might want to review the [esp-rs API guidelines](./API-GUIDELINES.md) + ### Issues #### Create a New Issue diff --git a/README.md b/README.md index 00f49a2c8..43caf080c 100644 --- a/README.md +++ b/README.md @@ -86,3 +86,5 @@ at your option. Unless you explicitly state otherwise, any contribution intentionally submitted for inclusion in the work by you, as defined in the Apache-2.0 license, shall be dual licensed as above, without any additional terms or conditions. + +If you consider to contribute please make sure you checked the [contributing guide](./CONTRIBUTING.md) and the [API guidelines](https://github.com/esp-rs/esp-hal/API-GUIDELINES.md)