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

Add on-target-tests and prepare for async implementation #25

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

ithinuel
Copy link
Member

@ithinuel ithinuel commented Feb 18, 2024

Using 2.2kOhm pull-ups on SDA & STL, this impl can reach the target 400KHz in release mode.

Write operation on a 10bit address:
Waveform of an I2C write operation on a 10bit address

Read operation on a 10bit address:
Waveform of an I2C read operation on a 10bit address

The tests run the bus at 200kHz to avoid requiring external pull-ups.

@ithinuel ithinuel force-pushed the prepare-for-async branch 2 times, most recently from fcd05de to bc58e47 Compare February 19, 2024 00:40
Copy link
Member

@jannic jannic left a comment

Choose a reason for hiding this comment

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

While I didn't have time for a full review yet, my first impression is very good. Having tests running on real hardware is a great improvement for such a crate!

src/i2c_cmd.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@jannic jannic left a comment

Choose a reason for hiding this comment

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

Very nicely readable code, great work!
I can't claim that I followed every step in the implemented I²C state machine, but scanning through it, it all looked very convincing. Together with the working on-target test, I have no doubts that this is fine.
Very useful approach to verify the I²C communication against the hardware peripheral on the same chip!

@ithinuel
Copy link
Member Author

Thank you !
I started looking at adding the async pio support but it is much less straightforward.

I am busy with something right now and for a little while so the next update will probably take a couple of months :(

@jannic
Copy link
Member

jannic commented Mar 11, 2024

I think we should do a release based on rp2040-hal 0.10.0, as it is needed for updating the BSPs.
And even without async support, I think the tests are a good addition.
What do you think about merging this pr and creating a release, and deferring async support for now?

@ithinuel
Copy link
Member Author

I think that’s a great idea :)

@jannic jannic merged commit c24a3d3 into rp-rs:main Mar 15, 2024
2 checks passed
@ithinuel ithinuel deleted the prepare-for-async branch March 15, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants