Skip to content

Commit

Permalink
Modernize UART interrupts (#2406)
Browse files Browse the repository at this point in the history
* Modernize UART interrupts

* Also derive Debug and Format for interrupt enums

* Fix changelog

* Touch up API-GUIDELINES
  • Loading branch information
bugadani authored Oct 28, 2024
1 parent 4e257f7 commit 8a23dbe
Show file tree
Hide file tree
Showing 8 changed files with 187 additions and 162 deletions.
12 changes: 6 additions & 6 deletions documentation/API-GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,14 @@ The following paragraphs contain additional recommendations.

## 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
- 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.
- 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`
- 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
- 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
Expand All @@ -45,6 +44,7 @@ The following paragraphs contain additional recommendations.
- 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)
- 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.

## Maintainability

Expand Down
4 changes: 3 additions & 1 deletion esp-hal/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `Pins::steal()` to unsafely obtain GPIO. (#2335)
- `I2c::with_timeout` (#2361)
- `Spi::half_duplex_read` and `Spi::half_duplex_write` (#2373)
- `Cpu::COUNT` and `Cpu::current()` (#?)
- `Cpu::COUNT` and `Cpu::current()` (#2411)
- `UartInterrupt` and related functions (#2406)

### Changed

Expand All @@ -31,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- The SPI driver has been rewritten to allow using half-duplex and full-duplex functionality on the same bus. See the migration guide for details. (#2373)
- Renamed `SpiDma` functions: `dma_transfer` to `transfer`, `dma_write` to `write`, `dma_read` to `read`. (#2373)
- Peripheral type erasure for UART (#2381)
- Changed listening for UART events (#2406)

### Fixed

Expand Down
42 changes: 42 additions & 0 deletions esp-hal/MIGRATING-0.21.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,45 @@ The `Spi<'_, SPI, HalfDuplexMode>::read` and `Spi<'_, SPI, HalfDuplexMode>::writ
)
.unwrap();
```

## UART event listening

The following functions have been removed:

- `listen_at_cmd`
- `unlisten_at_cmd`
- `listen_tx_done`
- `unlisten_tx_done`
- `listen_rx_fifo_full`
- `unlisten_rx_fifo_full`
- `at_cmd_interrupt_set`
- `tx_done_interrupt_set`
- `rx_fifo_full_interrupt_set`
- `reset_at_cmd_interrupt`
- `reset_tx_done_interrupt`
- `reset_rx_fifo_full_interrupt`

You can now use the `UartInterrupt` enum and the corresponding `listen`, `unlisten`, `interrupts` and `clear_interrupts` functions.

Use `interrupts` in place of `<INTERRUPT>_interrupt_set` and `clear_interrupts` in place of the old `reset_` functions.

`UartInterrupt`:
- `AtCmd`
- `TxDone`
- `RxFifoFull`

Checking interrupt bits is now done using APIs provided by [enumset](https://docs.rs/enumset/1.1.5/enumset/index.html). For example, to see if
a particular interrupt bit is set, use `contains`:

```diff
-serial.at_cmd_interrupt_set()
+serial.interupts().contains(UartInterrupt::AtCmd)
```

You can now listen/unlisten multiple interrupt bits at once:

```diff
-uart0.listen_at_cmd();
-uart0.listen_rx_fifo_full();
+uart0.listen(UartInterrupt::AtCmd | UartConterrupt::RxFifoFull);
```˛
3 changes: 2 additions & 1 deletion esp-hal/src/i2s.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ use crate::{
Mode,
};

#[derive(EnumSetType)]
#[derive(Debug, EnumSetType)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
/// Represents the various interrupt types for the I2S peripheral.
pub enum I2sInterrupt {
/// Receive buffer hung, indicating a stall in data reception.
Expand Down
3 changes: 2 additions & 1 deletion esp-hal/src/parl_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ use crate::{
const MAX_DMA_SIZE: usize = 32736;

/// Interrupts generated by the peripheral
#[derive(EnumSetType)]
#[derive(Debug, EnumSetType)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub enum ParlIoInterrupt {
/// Triggered when TX FIFO is empty. This interrupt indicates that there
/// might be error in the data sent by TX.
Expand Down
2 changes: 1 addition & 1 deletion esp-hal/src/spi/master.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ use crate::{

/// Enumeration of possible SPI interrupt events.
#[cfg(gdma)]
#[derive(EnumSetType)]
#[derive(Debug, EnumSetType)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub enum SpiInterrupt {
/// Indicates that the SPI transaction has completed successfully.
Expand Down
Loading

0 comments on commit 8a23dbe

Please sign in to comment.