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

Remove the single byte read/write inherent functions #2915

Merged
merged 11 commits into from
Jan 10, 2025
2 changes: 2 additions & 0 deletions esp-hal/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `DmaTxBuf::{compute_chunk_size, compute_descriptor_count, new_with_block_size}` (#2543)

- The `prelude` module has been removed (#2845)
- SPI: Removed `pub fn read_byte` and `pub fn write_byte` (#2915)

- Removed all peripheral instance type parameters and `new_typed` constructors (#2907)

Expand Down Expand Up @@ -198,6 +199,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Many peripherals are now disabled by default and also get disabled when the driver is dropped (#2544)

- Config: Crate prefixes and configuration keys are now separated by `_CONFIG_` (#2848)
- UART: `read_byte` and `write_byte` made private. (#2915)

### Fixed

Expand Down
23 changes: 23 additions & 0 deletions esp-hal/MIGRATING-0.22.md
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,18 @@ e.g.
+ .with_tx(peripherals.GPIO2);
```

`write_byte` and `read_byte` are now private.
JurajSadel marked this conversation as resolved.
Show resolved Hide resolved

e.g.

```dif
- while let nb::Result::Ok(_c) = serial.read_byte() {
- cnt += 1;
- }
+ let mut buff = [0u8; 64];
+ let cnt = serial.read_bytes(&mut buff);
```

## Spi `with_miso` has been split

Previously, `with_miso` set up the provided pin as an input and output, which was necessary for half duplex.
Expand Down Expand Up @@ -466,6 +478,17 @@ The Address and Command enums have similarly had their variants changed from e.g
+ Command::_1Bit
```

`write_byte` and `read_byte` were removed and `write_bytes` and `read_bytes` can be used as replacement.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could expand on this a little bit - like mentioning to use a one byte sized buffer and checking the returned length


e.g.

```rust
let mut byte = [0u8; 1];
spi.read_bytes(&mut byte);
```

`driver.write_bytes(&[data])?` and `driver.read_bytes(&mut buffer)?`
JurajSadel marked this conversation as resolved.
Show resolved Hide resolved

## GPIO Changes

The GPIO drive strength variants are renamed from e.g. `I5mA` to `_5mA`.
Expand Down
50 changes: 10 additions & 40 deletions esp-hal/src/spi/master.rs
Original file line number Diff line number Diff line change
Expand Up @@ -504,20 +504,6 @@ where
}
}

/// Read a byte from SPI.
///
/// Sends out a stuffing byte for every byte to read. This function doesn't
/// perform flushing. If you want to read the response to something you
/// have written before, consider using [`Self::transfer`] instead.
pub fn read_byte(&mut self) -> nb::Result<u8, Error> {
self.driver().read_byte()
}

/// Write a byte to SPI.
pub fn write_byte(&mut self, word: u8) -> nb::Result<(), Error> {
self.driver().write_byte(word)
}

/// Write bytes to SPI.
pub fn write_bytes(&mut self, words: &[u8]) -> Result<(), Error> {
self.driver().write_bytes(words)?;
Expand All @@ -526,6 +512,11 @@ where
Ok(())
}

/// Read bytes from SPI.
pub fn read_bytes(&mut self, words: &mut [u8]) -> Result<(), Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to take a bit of care here. UART's read_bytes is not blocking, SPI's read_bytes is blocking. As the peripherals work differently, it makes logical sense, but I'm not sure if we want the function use the same name, but do different things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be resolved in #2882 which is a bit stalled due to this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please elaborate how it will be resolved, that PR still has a non-blocking pub fn read_bytes(&mut self, buf: &mut [u8]) -> usize {. Non-blocking in the sense that it doesn't fill up the buffer, but returns early if there is not enough data to read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could add some documentation or rename, it can also be done here, no hard opinions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo this is fine as is, and if anything we should change uart's read_bytes back to being blocking and introduce a nb API post 1.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit more documentation wouldn't hurt probably. "Read bytes from SPI" is very brief

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit more documentation wouldn't hurt probably. "Read bytes from SPI" is very brief

Well, it matches write_bytes and transfer 😆

Copy link
Contributor

@bugadani bugadani Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do the UART changes once these PRs land.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave a non-blocking one around, in my application I don't know how much data the transmitter is going to send so I need the non-blocking version to just read what is available 😅

self.driver().read_bytes(words)
}

/// Sends `words` to the slave. Returns the `words` received from the slave
pub fn transfer<'w>(&mut self, words: &'w mut [u8]) -> Result<&'w [u8], Error> {
self.driver().transfer(words)
Expand Down Expand Up @@ -2139,11 +2130,14 @@ mod ehal1 {
Dm: DriverMode,
{
fn read(&mut self) -> nb::Result<u8, Self::Error> {
self.driver().read_byte()
let mut buffer = [0u8; 1];
self.driver().read_bytes(&mut buffer)?;
Ok(buffer[0])
}

fn write(&mut self, word: u8) -> nb::Result<(), Self::Error> {
self.driver().write_byte(word)
self.driver().write_bytes(&[word])?;
Ok(())
}
}

Expand Down Expand Up @@ -2912,30 +2906,6 @@ impl Driver {
});
}

fn read_byte(&self) -> nb::Result<u8, Error> {
if self.busy() {
return Err(nb::Error::WouldBlock);
}

let reg_block = self.register_block();
Ok(u32::try_into(reg_block.w(0).read().bits()).unwrap_or_default())
}

fn write_byte(&self, word: u8) -> nb::Result<(), Error> {
if self.busy() {
return Err(nb::Error::WouldBlock);
}

self.configure_datalen(0, 1);

let reg_block = self.register_block();
reg_block.w(0).write(|w| w.buf().set(word.into()));

self.start_operation();

Ok(())
}

#[cfg_attr(place_spi_driver_in_ram, ram)]
fn fill_fifo(&self, chunk: &[u8]) {
// TODO: replace with `array_chunks` and `from_le_bytes`
Expand Down
21 changes: 10 additions & 11 deletions esp-hal/src/uart.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@
//!
//! // Each component can be used individually to interact with the UART:
//! tx.write_bytes(&[42u8]).expect("write error!");
//! let byte = rx.read_byte().expect("read error!");
//! let mut byte = [0u8; 1];
//! rx.read_bytes(&mut byte);
//! # }
//! ```
//!
Expand Down Expand Up @@ -197,10 +198,8 @@
//! let mut serial = SERIAL.borrow_ref_mut(cs);
//! let serial = serial.as_mut().unwrap();
//!
//! let mut cnt = 0;
//! while let nb::Result::Ok(_c) = serial.read_byte() {
//! cnt += 1;
//! }
//! let mut buf = [0u8; 64];
//! let cnt = serial.read_bytes(&mut buf);
//! writeln!(serial, "Read {} bytes", cnt).ok();
//!
//! let pending_interrupts = serial.interrupts();
Expand Down Expand Up @@ -826,8 +825,8 @@ where
self.uart.info().apply_config(config)
}

/// Read a byte from the UART
pub fn read_byte(&mut self) -> nb::Result<u8, Error> {
// Read a byte from the UART
fn read_byte(&mut self) -> nb::Result<u8, Error> {
cfg_if::cfg_if! {
if #[cfg(esp32s2)] {
// On the ESP32-S2 we need to use PeriBus2 to read the FIFO:
Expand Down Expand Up @@ -1133,8 +1132,8 @@ where
sync_regs(register_block);
}

/// Write a byte out over the UART
pub fn write_byte(&mut self, word: u8) -> nb::Result<(), Error> {
// Write a byte out over the UART
fn write_byte(&mut self, word: u8) -> nb::Result<(), Error> {
JurajSadel marked this conversation as resolved.
Show resolved Hide resolved
self.tx.write_byte(word)
}

Expand All @@ -1143,8 +1142,8 @@ where
self.tx.flush()
}

/// Read a byte from the UART
pub fn read_byte(&mut self) -> nb::Result<u8, Error> {
// Read a byte from the UART
fn read_byte(&mut self) -> nb::Result<u8, Error> {
self.rx.read_byte()
}

Expand Down
15 changes: 8 additions & 7 deletions examples/src/bin/ieee802154_sniffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ fn main() -> ! {
let mut cnt = 0;
let mut read = [0u8; 2];
loop {
let c = nb::block!(uart0.read_byte()).unwrap();
if c == b'r' {
let mut buf = [0u8; 1];
while uart0.read_bytes(&mut buf) == 0 {}

if buf[0] == b'r' {
continue;
}

read[cnt] = c;
read[cnt] = buf[0];
cnt += 1;

if cnt >= 2 {
Expand Down Expand Up @@ -74,10 +76,9 @@ fn main() -> ! {
println!("@RAW {:02x?}", &frame.data);
}

if let nb::Result::Ok(c) = uart0.read_byte() {
if c == b'r' {
software_reset();
}
let mut buf = [0u8; 1];
if uart0.read_bytes(&mut buf) > 0 && buf[0] == b'r' {
software_reset();
}
}
}
8 changes: 4 additions & 4 deletions hil-test/tests/uart_regression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ mod tests {
uart::{self, UartRx, UartTx},
};
use hil_test as _;
use nb::block;

#[test]
fn test_that_creating_tx_does_not_cause_a_pulse() {
Expand All @@ -27,7 +26,8 @@ mod tests {
.with_rx(rx);

// start reception
_ = rx.read_byte(); // this will just return WouldBlock
let mut buf = [0u8; 1];
_ = rx.read_bytes(&mut buf); // this will just return WouldBlock

unsafe { tx.set_output_high(false, esp_hal::Internal::conjure()) };

Expand All @@ -38,8 +38,8 @@ mod tests {

tx.flush().unwrap();
tx.write_bytes(&[0x42]).unwrap();
let read = block!(rx.read_byte());
while rx.read_bytes(&mut buf) == 0 {}

assert_eq!(read, Ok(0x42));
assert_eq!(buf[0], 0x42);
}
}
6 changes: 3 additions & 3 deletions hil-test/tests/uart_tx_rx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use esp_hal::{
Blocking,
};
use hil_test as _;
use nb::block;

struct Context {
rx: UartRx<'static, Blocking>,
Expand Down Expand Up @@ -45,9 +44,10 @@ mod tests {

ctx.tx.flush().unwrap();
ctx.tx.write_bytes(&byte).unwrap();
let read = block!(ctx.rx.read_byte());
let mut buf = [0u8; 1];
while ctx.rx.read_bytes(&mut buf) == 0 {}

assert_eq!(read, Ok(0x42));
assert_eq!(buf[0], 0x42);
}

#[test]
Expand Down
Loading