i2c::slave: implement embedded-mcu-hal i2c::target trait

[felipebalbi]

Summary

This PR implements the embedded_mcu_hal::i2c::target trait on the existing I2cSlave driver and adds two on-target examples that exercise it.

The driver gains two trait implementations:

• embedded_mcu_hal::i2c::target::blocking::I2c for I2cSlave<'_, Blocking>
• embedded_mcu_hal::i2c::target::asynch::I2c for I2cSlave<'_, Async>

Each is implemented twice — once for SevenBitAddress and once for TenBitAddress — with a runtime guard that returns ErrorKind::Other when the wrong impl is driven against the configured address.

The existing inherent listen / respond_to_write / respond_to_read API and the Command / Response enums are preserved verbatim. The trait impls are layered on top via private helpers that return richer internal events / terminations, mapped down to either flavour at the boundary. The existing i2c-slave.rs and i2c-slave-async.rs examples build and behave unchanged.

What's new

• recover() as an inherent method on both flavours: aborts in-flight DMA, NAKs any pending byte, disables DMA arming, clears the deselect latch, and drops any pending remediation flag. The configured address(es) and slven bit are preserved across recover, matching the trait contract.
• PendingEdge bookkeeping that surfaces a Sr (repeated START) terminator from a previous respond_to_* as a Request::RepeatedStart(prev_addr) on the next listen() call — for trait callers only; the inherent API is unaffected.
• Probe → Stop(addr) mapping: a zero-byte write surfaces as Request::Stop(addr) with no preceding Write, documented in the module rustdoc.
• Error::kind() mapping for every TransferError variant + UnsupportedConfiguration, with unit tests in #[cfg(test)] mod tests.

Bug fix included

Mid-implementation HIL testing surfaced a pre-existing-style bug in the new async DMA read helper that this PR introduces: the poll_fn was missing the symmetric counterpart of the write path's !dma_channel.is_active() && is_slave_receive() wake condition. When DMA finished draining the supplied buffer but the controller kept clocking SCL for more bytes, the hardware did not raise slvpending (it just floated SDA so the controller clocked 0xFF from the bus pull-ups), and the future slept until something external aborted the transaction.

The fix adds the missing !dma_channel.is_active() && is_slave_transmit() wake arm plus the matching post-await InternalTermination::Continued(buf_len) → ReadStatus::NeedMore mapping. This is the second commit in the PR for ease of review.

Examples

Two new rt685s-evk examples (FC2 / PIO0_18 SCL, PIO0_17 SDA — same wiring as i2c-slave-async.rs, so any existing controller test rig works unchanged):

• examples/rt685s-evk/src/bin/i2c-slave-async-target-trait.rs
• examples/rt685s-evk/src/bin/i2c-slave-target-trait.rs

Each example wires the trait's listen → match Request → respond_* → loop loop and handles every Request / WriteStatus / ReadStatus variant.

HIL validation

Verified against an RT685S-EVK driven by a Pico de Gallo USB-to-I2C bridge. The full test matrix passes:

• 7-bit address scan finds the slave at 0x20.
• 8-byte write → WriteStatus::Stopped(8).
• 8-byte read → ReadStatus::Complete(8), payload matches [0..7].
• 4-byte write → Sr → 8-byte read in a single transaction → Restarted(4) + RepeatedStart(0x20) + Read(0x20) + Complete(8).
• 24-byte write → three BufferFull(8) cycles + Stopped(0).
• 4-byte read of an 8-byte buffer → EarlyStop(4).
• 16-byte read of an 8-byte buffer → NeedMore(8) + Complete(8) (the bug-fix scenario above).

Dep bump

embedded-mcu-hal is bumped from 0.2.0 to 0.3.0, which is the first release containing the i2c::target trait module.

Other notes

• No new feature flags.
• No changes to register access, unsafe blocks, clock setup, or pin muxing.
• Clippy clean on both the mimxrt685s and mimxrt633s feature combos.
• All existing examples in examples/rt685s-evk/ still build.

[github-actions]

Cargo Vet Audit Passed

cargo vet has passed in this PR. No new unvetted dependencies were found.

[Copilot]

Pull request overview

Implements the embedded_mcu_hal::i2c::target I2C target/slave traits for the existing I2cSlave driver (blocking + async, 7-bit + 10-bit), adds a recover() API to re-baseline the peripheral after cancelled/wedged transfers, and introduces new on-target examples demonstrating the trait-based API. Also bumps embedded-mcu-hal to 0.3.0 to pick up the new target trait module.

Changes:

• Add embedded_mcu_hal::i2c::target::{blocking, asynch}::I2c implementations for I2cSlave (7-bit and 10-bit) and supporting internal event/termination plumbing.
• Add recover() for both blocking and async flavours, plus pending-edge bookkeeping to surface RepeatedStart via the trait API.
• Add two rt685s-evk examples using the target trait API; bump embedded-mcu-hal dependency to 0.3.0 (and lockfiles accordingly).

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/i2c/slave.rs	Adds target-trait implementations, internal event/termination mapping, pending edge tracking, and recover() for blocking/async.
examples/rt685s-evk/src/bin/i2c-slave-target-trait.rs	New blocking example exercising the target trait API.
examples/rt685s-evk/src/bin/i2c-slave-async-target-trait.rs	New async example exercising the target trait API.
examples/rt685s-evk/Cargo.toml	Bumps embedded-mcu-hal to 0.3.0 for the examples workspace.
examples/rt685s-evk/Cargo.lock	Lockfile updates due to the dependency bump.
Cargo.toml	Bumps embedded-mcu-hal to 0.3.0 for the main crate.
Cargo.lock	Lockfile updates due to the dependency bump.

[kurtjd]

There seems to be quite a bit of code duplication. Looks like you've defined several *_inner methods which are mostly copy and paste of the existing methods they mirror with slightly different return types to be used by the trait impls. Is it not possible to just use the exusting methods and just map their output in the trait impls instead of duplicating code?

[Copilot]

Pull request overview

Copilot reviewed 8 out of 13 changed files in this pull request and generated 6 comments.

[jerrysxie]

I had to drop the partial read condition from the original implementation due to a HW race condition, in stress testing, the slv_state() would go to addressed instead of transmitted so the async operation gets stuck:
felipebalbi@62dc59c, and I cannot find a workaround even after working with NXP.

[felipebalbi]

The change here is on the read side, not write. Haven't seen this particular race condition. I'll try to add a quick test to my hacky tests (with pico de gallo).

[jerrysxie]

Yeah, this is in respond_to_read_internal(). I was hitting this race condition consistently in long term stressing test with release binary.

[felipebalbi]

unable to hit it. Has been running for quite a while now.

[felipebalbi]

I'll bring Mole tomorrow for testing, I guess it's really the only way to give this a solid shot. Really cannot reproduce with pico de gallo.

[felipebalbi]

Mole has been running since 6AM today, no errors.

[jerrysxie]

Running i2c-loopback-async.rs with this changes to trigger case where i2c target does not provide enough data on single call of respond_to_read() on IMXRT in loopback mode produces a panic:

diff --git a/examples/rt685s-evk/src/bin/i2c-loopback-async.rs b/examples/rt685s-evk/src/bin/i2c-loopback-async.rs
index fbdbf50..1086100 100644
--- a/examples/rt685s-evk/src/bin/i2c-loopback-async.rs
+++ b/examples/rt685s-evk/src/bin/i2c-loopback-async.rs
@@ -16,7 +16,7 @@ const ADDR: u8 = 0x20;
const MASTER_BUFLEN: usize = 8;
// slave buffer has to be 1 bigger than master buffer because master does not
// handle end of read properly
-const SLAVE_BUFLEN: usize = MASTER_BUFLEN + 1;
+const SLAVE_BUFLEN: usize = MASTER_BUFLEN / 2;
const SLAVE_ADDR: Option<Address> = Address::new(ADDR);
bind_interrupts!(struct Irqs {

9140.448752 [INFO ] Read @ "SevenBit(32)" (i2c_loopback_async src/bin/i2c-loopback-async.rs:43)
9140.448841 [INFO ] Response to read got 4 bytes, more bytes to fill (i2c_loopback_async src/bin/i2c-loopback-async.rs:54)
9140.448934 [INFO ] Response to read got 4 bytes, more bytes to fill (i2c_loopback_async src/bin/i2c-loopback-async.rs:54)
9140.485323 [ERROR] panicked at src/bin/i2c-loopback-async.rs:45:68:
called `Result::unwrap()` on an `Err` value: Transfer(WriteFail) (panic_probe panic-probe-1.0.0/src/lib.rs:104)
Firmware exited unexpectedly: Exception
WARN probe_rs_debug::debug_info: UNWIND: Error while checking for exception context. The stack trace will not include the calling frames. : Probe(Register("No Stack Pointer register. Please report this as a bug."))
Core 0
    Frame 0: HardFault_ @ 0x08007a7e
       /home/jxie/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/cortex-m-rt-0.7.5/src/lib.rs:1103:1
    Frame 1: UNWIND: Error while checking for exception context. The stack trace will not include the calling frames. : Probe(Register("No Stack Pointer register. Please report this as a bug.")) @ 0x08007a7e

The line that triggered this error

        // Verify that we are ready for transmit
        if !i2c.stat().read().slvstate().is_slave_transmit() {
            return Err(TransferError::WriteFail.into());
        }

This corresponds to the hardware race condition that we engaged with NXP before:

We are seeing some inconsistencies on the SLVSTATE behavior:
* In some instance, for I2C writes where slave is in transmit mode. After DMA is completed, we get the DMA interrupt. Master has not NACK any data. SLVSTATE is addressed (0x0) instead of transmit (0x2).
* In some instances, for I2C writes where slave is in transmit mode. After DMA is completed, we get the DMA interrupt. Master is generating NACK and STOP with a little bit of delay. Initially we observed SLVSTATE is set to transmit (2) and SLVSEL is set, then SLVSTATE changes to addressed (0) briefly and SLVSEL is set, then finally SLVSTATE stays as addressed and then SLVDESEL is set and SLVSEL is cleared.

We don't have a 100% guarantee that SLVSTATE will be in transmit

        poll_fn(|cx| {
            let i2c = self.info.regs;

            self.info.waker.register(cx.waker());
            dma_channel.get_waker().register(cx.waker());

            let stat = i2c.stat().read();
            // Master sent a stop or nack
            if stat.slvdesel().is_deselected() {
                return Poll::Ready(());
            }
            // We need SW intervention
            if stat.slvpending().is_pending() {
                return Poll::Ready(());
            }
            // DMA drained but the master still expects more bytes.
            // Without this guard the future sleeps forever because the
            // hardware does not raise slvpending once DMA is armed and
            // running dry — SDA just floats and the master clocks 0xFF
            // until something else aborts the transaction.
            if !dma_channel.is_active() && stat.slvstate().is_slave_transmit() {  <-- this slvstate is not guaranteed to be in the state we expect
                return Poll::Ready(());
            }

            Poll::Pending
        })
        .await;

Maybe we should consider not supporting Response::ReadContinued(xfer_count) case on imxrt?

[felipebalbi]

There seems to be quite a bit of code duplication. Looks like you've defined several *_inner methods which are mostly copy and paste of the existing methods they mirror with slightly different return types to be used by the trait impls. Is it not possible to just use the exusting methods and just map their output in the trait impls instead of duplicating code?

I'll do it, but it will be a breaking change for the existing API due to Request not carrying the address and usize fields we need.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 60 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 18 changed files in this pull request and generated 10 comments.

[jerrysxie]

nit: newline?

[jerrysxie]

There is no possible way for resp_read to returned Response::Stopped(n) in the current code, right?

Why is Response::Restarted(n) -> mcu_target::ReadStatus::EarlyStop(n)? We can technically exhaust the DMA and still get a restart? Just trying to understand the terminology here.

[jerrysxie]

So we are skipping the listen() step if we restart? So this would be an optimization?

Wouldn't we not skip this transition in listen() to advance the i2c state machine if that is the case?

        // Disable DMA
        i2c.slvctl().write(|w| w.slvdma().disabled());

        // Check whether we already have a matched address and just waiting
        // for software ack/nack
        if !i2c.stat().read().slvpending().is_pending() {
            self.poll_sw_action().await;
        }

        if i2c.stat().read().slvstate().is_slave_address() {
            i2c.slvctl().write(|w| w.slvcontinue().continue_());
        } else {
            // If we are already past the addressed phase and in transmit or receive, that means we are already in the
            // next state, most likely due to calling listen() before the previous transaction is completed and leading
            // to state transition out of order. We can tolerate that, so we just move onto the next state.
        }

        // Poll for HW to transitioning from addressed to receive/transmit
        self.poll_sw_action().await;

I guess in this case, we left the DMA enabled and the i2c state machine auto transition to the valid state anyway?

[jerrysxie]

What if after we restarted, we change direction? If we skip listen() and check for directions, do we even know that we change direction?

[jerrysxie]

Oh, I see, for repeated start, the client would have call listen() again to get the direction. Nevermind.

[jerrysxie]

Hmmm, this feels a bit weird, is this the expected sequence of events for start -> read -> start -> read

1. listen() -> Request::Read(addr)
2. respond_to_read() -> Response::Restarted(n) -> ReadStatus::EarlyStop(n)
3. listen() -> Request::RepeatedStart(prev_addr)
4. listen() -> Request::Read(addr)

[jerrysxie]

Slave is sending data to the master, so this nack here does not make sense.

Do we ever hit this branch in testing? With DMA enabled, the slv pending bit is usually never set in my observation.

I left the pending flag check in there previously just as a catch-all in case if something strange ever happens.

[jerrysxie]

Hmmm, this feels a bit weird, is this the expected sequence of events for start -> read -> start -> read

1. listen() -> Request::Read(addr)
2. respond_to_read() -> Response::Restarted(n) -> ReadStatus::EarlyStop(n)
3. listen() -> Request::RepeatedStart(prev_addr)
4. listen() -> Request::Read(addr)