Reworked blocking i2c slave interface #389

davidv1992 · 2025-04-11T12:25:48Z

Main motivators are:

More consistent interface that is better misuse resistant.
Easier to extend with support for Automatic Ack/Nack of addresses in async variant.
Better support for cancelation and capable of better surfacing what the hardware can actually do.

The code changes also fixes some issues in 10-bit addressing. It should now match the behaviour specified in UM10204

davidv1992 · 2025-04-11T12:28:09Z

This is only the rewrite of the blocking interface. If this design is considered reasonable I can extend it to the async side and actually do the support around automatic address ack/nack. The choice for separate types for the blocking and async slave interfaces is that both will need different code to run on drop, which cannot be done another way because of restrictions on Drop implementations.

tdittr

A few thoughts for now, going to finish the review next week.

src/i2c/slave.rs

tdittr · 2025-04-11T14:32:24Z

src/i2c/slave.rs

In general I think we should improve the granularity of the returned errors to make debugging easier.

All these errors are essentially identical (bus is in a strange state), so there isn't really anything to separate them.

tdittr

I like the new API, however it does feel a bit rough around the edges.

Should we maybe implement the async version and improve the documentation before merging?

tdittr · 2025-04-14T13:25:37Z

src/i2c/slave.rs

-            i2c.stat().write(|w| w.slvdesel().deselected());
-            return Ok(Response::Complete(xfer_count));
+            // inhibit drop
+            core::mem::forget(self);


Nit-pick: I would replace with a destructuring assignment.

Suggested change

core::mem::forget(self);

let BlockingI2cSlaveWrite{..} = self;

Or alternatively have a Self::defuse method.

src/i2c/slave.rs

tdittr · 2025-04-14T13:41:13Z

examples/rt685s-evk/src/bin/i2c-slave.rs

I like this example for showing a real-world use-case 👍

jerrysxie

@davidv1992 Still going through the review, just one question about the overall goals and next steps:

For all the ODP I2C slave HALs, the plan is to have a HW agnostic traits interface to be implemented by both embassy-imxrt and embassy-npcx, is this a step toward that direction? The I2C traits are eventually to be upstreamed to embedded-hal, but meanwhile we should probably have it as a independent crate in ODP until the upstreaming can happen.
We do have some clients that relies on this i2c slave interface already. Merging this will require a bit coordination.

Copilot

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

davidv1992 · 2025-04-16T06:59:59Z

@jerrysxie Yes, this is aimed to be a step in that direction. The exact shape of a hardware agnostic trait is still in the design phase, but something along these lines is what we are trending towards. The main pain points right now (though those are less relevant for the two crates you mention) is that some hardware has quite limited i2c slave support.

As for this requiring some coordination to merge, that is not entirely unexpected. My main goal at this stage is to gather feedback on the overall shape of the design in preparation for doing the same reworking of the interface in the async case. So far, reactions seem positive, but if on your end you feel different about it it would be nice to catch that before I have sunk a bunch of work into the async version, as it's likely going to be somewhat more complicated to implement.

jerrysxie · 2025-04-24T15:24:30Z

@jerrysxie Yes, this is aimed to be a step in that direction. The exact shape of a hardware agnostic trait is still in the design phase, but something along these lines is what we are trending towards. The main pain points right now (though those are less relevant for the two crates you mention) is that some hardware has quite limited i2c slave support.

As for this requiring some coordination to merge, that is not entirely unexpected. My main goal at this stage is to gather feedback on the overall shape of the design in preparation for doing the same reworking of the interface in the async case. So far, reactions seem positive, but if on your end you feel different about it it would be nice to catch that before I have sunk a bunch of work into the async version, as it's likely going to be somewhat more complicated to implement.

Hi, @davidv1992! One more question regarding the plan to add support for Automatic Ack/Nack of addresses in async variant. I tried this but the code becomes really messy, because the NXP auto-ack combined with DMA requires the code to set up DMA ahead of time. For auto-ack of a write, we have to set up DMA for write and have DMA ready to go before listening. If it is a write, everything proceeds as expected. But it is a read, then we have to abort the write DMA and manually ack the address and set up read DMA again. It made the code super messy. How do we plan to deal with that?

davidv1992 · 2025-05-07T06:37:03Z

Hi, @davidv1992! One more question regarding the plan to add support for Automatic Ack/Nack of addresses in async variant. I tried this but the code becomes really messy, because the NXP auto-ack combined with DMA requires the code to set up DMA ahead of time. For auto-ack of a write, we have to set up DMA for write and have DMA ready to go before listening. If it is a write, everything proceeds as expected. But it is a read, then we have to abort the write DMA and manually ack the address and set up read DMA again. It made the code super messy. How do we plan to deal with that?

@jerrysxie Sorry for the late response, missed this before my vacation. Yes there is some mess here, and there is a lot of complications due to potential race conditions between the software and the hardware, but the way I manage it currently is by having the rest of the code more streamlined so that the expectation doesn't match scenario becomes less of an alternate path. I am still in the process of cleaning up the code, but everything is implemented right now.

github-actions · 2025-05-08T08:14:03Z

Cargo Vet Audit Passed

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

davidv1992 · 2025-05-08T09:03:32Z

@jerrysxie the CI fails, but this seems to be an issue with the setup of the pipeline, not the code. Would it be possible for you to look into this?

jeffglaum · 2025-05-20T17:50:51Z

@JamesHuard , @felipebalbi , @RobertZ2011 can you guys please review and provide feedback? Thanks.

Main motivators are: - More consistent interface that is better misuse resistant. - Easier to extend with support for Automatic Ack/Nack of addresses in async variant. - Better support for cancelation and capable of better surfacing what the hardware can actually do. The code changes also fixes some issues in 10-bit addressing. It should now match the behaviour specified in UM10204

This is done through the expect_read and expect_write interfaces, as automatic acknowledgement requires the dma to be in place beforehand.

This register contains both write-to-trigger bits and bits that represent semi-permanent state. This commit ensures we don't accidentaly write 1s to the write-to-trigger bits, as the hardware doesn't clear these automatically.

davidv1992 · 2025-05-30T07:28:02Z

@jeffglaum You have assigned this one back to me, but it is unclear what is required from me at this point. Could you clarify that?

felipebalbi

I didn't attempt to run the examples yet, but the code looks easy enough to read. Since you're using auto-ack, did you manage to quantify any changes to bus utilization?

felipebalbi · 2025-06-10T18:03:15Z

src/i2c/slave.rs

    }
 }

-impl<'a> I2cSlave<'a, Blocking> {


Just curious: why remove the Mode type parameter here?

jeffglaum · 2025-06-12T00:20:25Z

@jeffglaum You have assigned this one back to me, but it is unclear what is required from me at this point. Could you clarify that?

Just assigning to you, to complete the PR (looks like there are still open discussion points which are blocking the merge). Thanks.

jeffglaum · 2025-09-16T15:03:20Z

@davidv1992 are you intending to complete this PR (looks like it had approvals) or to abandon? Thanks.

davidv1992 requested review from JamesHuard, RobertZ2011, felipebalbi, jerrysxie, madeleyneVaca and tullom as code owners April 11, 2025 12:25

davidv1992 force-pushed the i2c-new-interface branch from b99d4a3 to f60674d Compare April 11, 2025 12:29

tdittr reviewed Apr 11, 2025

View reviewed changes

tdittr reviewed Apr 14, 2025

View reviewed changes

jerrysxie added this to Embedded Controller Apr 14, 2025

jerrysxie reviewed Apr 14, 2025

View reviewed changes

jerrysxie requested a review from Copilot April 14, 2025 23:13

Copilot AI reviewed Apr 14, 2025

View reviewed changes

davidv1992 force-pushed the i2c-new-interface branch from 98f39fd to 32cfbe2 Compare April 17, 2025 08:47

davidv1992 marked this pull request as draft April 24, 2025 09:25

davidv1992 force-pushed the i2c-new-interface branch 4 times, most recently from 2f7a2e6 to 77854a9 Compare April 24, 2025 12:25

davidv1992 force-pushed the i2c-new-interface branch 2 times, most recently from 67618e8 to d240c12 Compare April 25, 2025 14:13

akshat2112 self-requested a review April 30, 2025 23:48

davidv1992 force-pushed the i2c-new-interface branch from 044a9e0 to c351e90 Compare May 7, 2025 07:58

davidv1992 marked this pull request as ready for review May 7, 2025 07:58

davidv1992 force-pushed the i2c-new-interface branch from f08aef0 to e84bb4f Compare May 8, 2025 08:12

github-actions bot added the cargo vet PR requiring auditor review label May 8, 2025

davidv1992 requested review from jerrysxie and tdittr May 8, 2025 08:14

davidv1992 force-pushed the i2c-new-interface branch from f404fa7 to bc756a7 Compare May 9, 2025 09:53

jeffglaum moved this to In review in Embedded Controller May 12, 2025

davidv1992 added 8 commits May 21, 2025 08:53

Reworked async i2c slave interface to match blocking one.

428acc2

Ensure a deselect is still triggered on dropped i2c slave reads.

0d8dd98

Split i2c listen functions in preparations for implement auto(n)ack.

67149de

Implemented support for automatic address acknowledgement.

4f696d5

This is done through the expect_read and expect_write interfaces, as automatic acknowledgement requires the dma to be in place beforehand.

Small cleanup work around i2c slave driver.

a4399e4

Fixed i2c slave slvctl register handling.

500700d

This register contains both write-to-trigger bits and bits that represent semi-permanent state. This commit ensures we don't accidentaly write 1s to the write-to-trigger bits, as the hardware doesn't clear these automatically.

Added extensive testing for async i2c slave interface.

8cfc384

davidv1992 force-pushed the i2c-new-interface branch from bc756a7 to 8cfc384 Compare May 21, 2025 06:55

jeffglaum assigned davidv1992 May 28, 2025

davidv1992 mentioned this pull request May 30, 2025

Use the embedded-hal-i2c traits for i2c targets #440

Draft

felipebalbi approved these changes Jun 10, 2025

View reviewed changes

src/i2c/slave.rs

}

}

impl<'a> I2cSlave<'a, Blocking> {

Copy link

Contributor

felipebalbi Jun 10, 2025

Choose a reason for hiding this comment

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

Just curious: why remove the Mode type parameter here?

JamesHuard approved these changes Jun 10, 2025

View reviewed changes

jeffglaum modified the milestones: IMXRT HAL Code Quality and Enhancements, 0. IMXRT HAL Code Quality Nov 17, 2025

	core::mem::forget(self);
	let BlockingI2cSlaveWrite{..} = self;

Reworked blocking i2c slave interface #389

Are you sure you want to change the base?

Reworked blocking i2c slave interface #389

Uh oh!

Conversation

davidv1992 commented Apr 11, 2025

Uh oh!

davidv1992 commented Apr 11, 2025

Uh oh!

tdittr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdittr Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

davidv1992 May 8, 2025

Choose a reason for hiding this comment

Uh oh!

tdittr left a comment

Choose a reason for hiding this comment

Uh oh!

tdittr Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdittr Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

jerrysxie left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

davidv1992 commented Apr 16, 2025

Uh oh!

jerrysxie commented Apr 24, 2025

Uh oh!

davidv1992 commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Cargo Vet Audit Passed

Uh oh!

davidv1992 commented May 8, 2025

Uh oh!

jeffglaum commented May 20, 2025

Uh oh!

davidv1992 commented May 30, 2025

Uh oh!

felipebalbi left a comment

Choose a reason for hiding this comment

Uh oh!

felipebalbi Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

jeffglaum commented Jun 12, 2025

Uh oh!

jeffglaum commented Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davidv1992 commented May 7, 2025 •

edited

Loading

github-actions bot commented May 8, 2025 •

edited

Loading