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

I2c: attempt empty writes #2506

Merged
merged 6 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions esp-hal/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- TWAI should no longer panic when receiving a non-compliant frame (#2255)
- OneShotTimer: fixed `delay_nanos` behaviour (#2256)
- Fixed unsoundness around `Efuse` (#2259)
- Empty I2C writes to unknown addresses now correctly fail with `AckCheckFailed`. (#2506)

### Removed

Expand Down
133 changes: 72 additions & 61 deletions esp-hal/src/i2c/master/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1855,30 +1855,51 @@ impl Driver<'_> {
.write(|w| w.rxfifo_full().clear_bit_by_one());
}

/// Executes an I2C write operation.
fn start_write_operation(
&self,
address: u8,
bytes: &[u8],
start: bool,
stop: bool,
) -> Result<usize, Error> {
self.reset_fifo();
self.reset_command_list();
let cmd_iterator = &mut self.register_block().comd_iter();

if start {
add_cmd(cmd_iterator, Command::Start)?;
}

self.setup_write(address, bytes, start, cmd_iterator)?;

add_cmd(
cmd_iterator,
if stop { Command::Stop } else { Command::End },
)?;
let index = self.fill_tx_fifo(bytes);
self.start_transmission();

Ok(index)
}

/// Executes an I2C read operation.
/// - `addr` is the address of the slave device.
/// - `bytes` is the data two be sent.
/// - `buffer` is the buffer to store the read data.
/// - `start` indicates whether the operation should start by a START
/// condition and sending the address.
/// - `stop` indicates whether the operation should end with a STOP
/// condition.
/// - `will_continue` indicates whether there is another read operation
/// following this one and we should not nack the last byte.
/// - `cmd_iterator` is an iterator over the command registers.
fn write_operation_blocking(
fn start_read_operation(
&self,
address: u8,
bytes: &[u8],
buffer: &mut [u8],
start: bool,
stop: bool,
will_continue: bool,
) -> Result<(), Error> {
self.clear_all_interrupts();

// Short circuit for zero length writes without start or end as that would be an
// invalid operation write lengths in the TRM (at least for ESP32-S3) are 1-255
if bytes.is_empty() && !start && !stop {
return Ok(());
}

// Reset FIFO and command list
self.reset_fifo();
self.reset_command_list();

Expand All @@ -1887,14 +1908,41 @@ impl Driver<'_> {
if start {
add_cmd(cmd_iterator, Command::Start)?;
}
self.setup_write(address, bytes, start, cmd_iterator)?;

self.setup_read(address, buffer, start, will_continue, cmd_iterator)?;

add_cmd(
cmd_iterator,
if stop { Command::Stop } else { Command::End },
)?;
let index = self.fill_tx_fifo(bytes);
self.start_transmission();
Ok(())
}

/// Executes an I2C write operation.
/// - `addr` is the address of the slave device.
/// - `bytes` is the data two be sent.
/// - `start` indicates whether the operation should start by a START
/// condition and sending the address.
/// - `stop` indicates whether the operation should end with a STOP
/// condition.
/// - `cmd_iterator` is an iterator over the command registers.
fn write_operation_blocking(
&self,
address: u8,
bytes: &[u8],
start: bool,
stop: bool,
) -> Result<(), Error> {
self.clear_all_interrupts();

// Short circuit for zero length writes without start or end as that would be an
// invalid operation write lengths in the TRM (at least for ESP32-S3) are 1-255
if bytes.is_empty() && !start && !stop {
return Ok(());
}

let index = self.start_write_operation(address, bytes, start, stop)?;
// Fill the FIFO with the remaining bytes:
self.write_remaining_tx_fifo_blocking(index, bytes)?;
self.wait_for_completion_blocking(!stop)?;
Expand Down Expand Up @@ -1927,23 +1975,7 @@ impl Driver<'_> {
return Ok(());
}

// Reset FIFO and command list
self.reset_fifo();
self.reset_command_list();

let cmd_iterator = &mut self.register_block().comd_iter();

if start {
add_cmd(cmd_iterator, Command::Start)?;
}

self.setup_read(address, buffer, start, will_continue, cmd_iterator)?;

add_cmd(
cmd_iterator,
if stop { Command::Stop } else { Command::End },
)?;
self.start_transmission();
self.start_read_operation(address, buffer, start, stop, will_continue)?;
self.read_all_from_fifo_blocking(buffer)?;
self.wait_for_completion_blocking(!stop)?;
Ok(())
Expand Down Expand Up @@ -1972,22 +2004,7 @@ impl Driver<'_> {
return Ok(());
}

// Reset FIFO and command list
self.reset_fifo();
self.reset_command_list();

let cmd_iterator = &mut self.register_block().comd_iter();
if start {
add_cmd(cmd_iterator, Command::Start)?;
}
self.setup_write(address, bytes, start, cmd_iterator)?;
add_cmd(
cmd_iterator,
if stop { Command::Stop } else { Command::End },
)?;
let index = self.fill_tx_fifo(bytes);
self.start_transmission();

let index = self.start_write_operation(address, bytes, start, stop)?;
// Fill the FIFO with the remaining bytes:
self.write_remaining_tx_fifo(index, bytes).await?;
self.wait_for_completion(!stop).await?;
Expand Down Expand Up @@ -2020,19 +2037,7 @@ impl Driver<'_> {
return Ok(());
}

// Reset FIFO and command list
self.reset_fifo();
self.reset_command_list();
let cmd_iterator = &mut self.register_block().comd_iter();
if start {
add_cmd(cmd_iterator, Command::Start)?;
}
self.setup_read(address, buffer, start, will_continue, cmd_iterator)?;
add_cmd(
cmd_iterator,
if stop { Command::Stop } else { Command::End },
)?;
self.start_transmission();
self.start_read_operation(address, buffer, start, stop, will_continue)?;
self.read_all_from_fifo(buffer).await?;
self.wait_for_completion(!stop).await?;
Ok(())
Expand Down Expand Up @@ -2067,6 +2072,9 @@ impl Driver<'_> {
start: bool,
stop: bool,
) -> Result<(), Error> {
if buffer.is_empty() {
return self.write_operation_blocking(address, &[], start, stop);
}
let chunk_count = buffer.len().div_ceil(I2C_CHUNK_SIZE);
for (idx, chunk) in buffer.chunks(I2C_CHUNK_SIZE).enumerate() {
self.write_operation_blocking(
Expand Down Expand Up @@ -2110,6 +2118,9 @@ impl Driver<'_> {
start: bool,
stop: bool,
) -> Result<(), Error> {
if buffer.is_empty() {
return self.write_operation(address, &[], start, stop).await;
}
let chunk_count = buffer.len().div_ceil(I2C_CHUNK_SIZE);
for (idx, chunk) in buffer.chunks(I2C_CHUNK_SIZE).enumerate() {
self.write_operation(
Expand Down
25 changes: 21 additions & 4 deletions hil-test/tests/i2c.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#![no_main]

use esp_hal::{
i2c::master::{Config, I2c, Operation},
i2c::master::{Config, Error, I2c, Operation},
Async,
Blocking,
};
Expand All @@ -24,6 +24,9 @@ fn _async_driver_is_compatible_with_blocking_ehal() {
fn _with_ehal(_: impl embedded_hal::i2c::I2c) {}
}

const DUT_ADDRESS: u8 = 0x77;
const NON_EXISTENT_ADDRESS: u8 = 0x6b;

#[cfg(test)]
#[embedded_test::tests]
mod tests {
Expand All @@ -44,17 +47,31 @@ mod tests {
Context { i2c }
}

#[test]
#[timeout(3)]
fn empty_write_returns_ack_error_for_unknown_address(mut ctx: Context) {
assert_eq!(
ctx.i2c.write(NON_EXISTENT_ADDRESS, &[]),
Err(Error::AckCheckFailed)
);
assert_eq!(ctx.i2c.write(DUT_ADDRESS, &[]), Ok(()));
}

#[test]
#[timeout(3)]
fn test_read_cali(mut ctx: Context) {
let mut read_data = [0u8; 22];

// have a failing read which might could leave the peripheral in an undesirable
// state
ctx.i2c.write_read(0x55, &[0xaa], &mut read_data).ok();
ctx.i2c
.write_read(NON_EXISTENT_ADDRESS, &[0xaa], &mut read_data)
.ok();

// do the real read which should succeed
ctx.i2c.write_read(0x77, &[0xaa], &mut read_data).ok();
ctx.i2c
.write_read(DUT_ADDRESS, &[0xaa], &mut read_data)
.ok();

assert_ne!(read_data, [0u8; 22])
}
Expand All @@ -67,7 +84,7 @@ mod tests {
// do the real read which should succeed
ctx.i2c
.transaction(
0x77,
DUT_ADDRESS,
&mut [Operation::Write(&[0xaa]), Operation::Read(&mut read_data)],
)
.ok();
Expand Down