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] (re)introduce async.await implementation #747

Merged
merged 3 commits into from
Feb 10, 2024

Conversation

ithinuel
Copy link
Member

@ithinuel ithinuel commented Jan 1, 2024

This re-introduces the async.await i2c driver that was once remove because of requiring nightly at the time.

Now that the necessary features have been stabilized, we can all enjoy a bit of async.await-ness :)

Requires

rp2040-hal/Cargo.toml Show resolved Hide resolved
rp2040-hal/src/async.rs Outdated Show resolved Hide resolved
rp2040-hal/src/async.rs Outdated Show resolved Hide resolved
rp2040-hal/src/i2c.rs Outdated Show resolved Hide resolved
@ithinuel
Copy link
Member Author

ithinuel commented Jan 1, 2024

This reminded me that we do not have doc nor the ability to really use IRQ based application for i2c. I shall fix that too in an upcoming PR.

/// if your board has a different frequency
const XTAL_FREQ_HZ: u32 = 12_000_000u32;

/// The function configures the RP2040 peripherals, reads the temperature from
Copy link
Member

Choose a reason for hiding this comment

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

It does not, in fact, do this.

.m_rx_full()
.enabled()
});
critical_section::with(|cs| WAKER.borrow_ref_mut(cs).take().map(Waker::wake));
Copy link
Member

Choose a reason for hiding this comment

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

What does this do? Can we write it out as multiple lines with intermediate variable names and some comments?

Copy link
Member

@thejpster thejpster Jan 1, 2024

Choose a reason for hiding this comment

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

I think I'd also prefer if let Some(waker) = borrowed_global_waker.take() { Waker::wake(waker) } or similar, even if clippy might say "yeah, but you can just use .map(Waker::wake)", because it's non-obvious where why you would grab a global and then map it to a different type, and then drop the value.

async fn write_iter_async<A, U>(&mut self, address: A, bytes: U) -> Result<(), Error>
where
U: IntoIterator<Item = u8>,
A: AddressMode + 'static + Into<u16>,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should try to keep inherent implementations free from embedded-hal defined types. If we use them in our own API, it'll be difficult to upgrade embedded-hal later.

We did that in a few places with embedded-hal 0.2, and it'll make moving everything to embedded-hal 1.0 more difficult.

Also, we do not even support 10bit address mode yet, afaik.

Copy link
Member Author

Choose a reason for hiding this comment

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

rather than the inherent method, should I impl the crate I made to cover the write_iter that was removed between 0.2.7 and 1.0 ?

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't that complicate things further? It would mean that users of those methods would need to depend on your crate as well and import the traits, before they can call the functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. I wish it hadn't been removed from the embedded-hal in the first place :/

I'll make this driver impl the trait and keep this dependency feature-gated.

rp2040-hal/examples/i2c_async.rs Outdated Show resolved Hide resolved
rp2040-hal/examples/i2c_async_irq.rs Outdated Show resolved Hide resolved
rp2040-hal/examples/i2c_async_irq.rs Outdated Show resolved Hide resolved
rp2040-hal/src/i2c.rs Outdated Show resolved Hide resolved
@thejpster
Copy link
Member

I'm happy once jannic is happy

@thejpster
Copy link
Member

If this is draft because we have questions around the async-bits, maybe we can update the non-async stuff to be e-h 1.0 compatible in another PR and then rebase this over it? I'm keen to get e-h 1.0 support out of the door because I have other projects stuck until that happens.

@ithinuel
Copy link
Member Author

ithinuel commented Jan 13, 2024

This is in draft because I'm updating i2c implementation as I'm also adding on-board-test suites for controller/target sync/async.
Please, let's proceed with #753 first and I'll rebase this PR on main after it's merged.

@ithinuel ithinuel force-pushed the introduce-async-i2c branch 7 times, most recently from 2c4d76b to 1edeee8 Compare January 29, 2024 23:23
@ithinuel ithinuel force-pushed the introduce-async-i2c branch 7 times, most recently from e6d3616 to ef6a686 Compare January 31, 2024 06:03
@ithinuel ithinuel marked this pull request as ready for review January 31, 2024 06:10
@ithinuel
Copy link
Member Author

Finally it is there !!

.github/workflows/build_and_test.yml Show resolved Hide resolved
- run: |
cd on-target-tests
cargo clippy -- -Dwarnings
- run: cargo clippy --workspace --examples
Copy link
Member

Choose a reason for hiding this comment

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

Why not -Dwarnings?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is already set in the RUSTFLAGS, but I may be confused.
Aren’t they redundant?

rp2040-hal/src/async_utils.rs Outdated Show resolved Hide resolved
rp2040-hal/examples/i2c.rs Show resolved Hide resolved
rp2040-hal/src/async_utils.rs Outdated Show resolved Hide resolved
}

fn read_and_clear_abort_reason(&mut self) -> Option<u32> {
#[inline]
fn read_and_clear_abort_reason(&mut self) -> Result<(), Error> {
Copy link
Member

Choose a reason for hiding this comment

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

The combination of function naming and return type causes some ambiguity: Intuitively, when a function called read_and_clear_abort_reason returns an abort reason, I'd call it a success, not an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is mostly to save some boiler plate code as there’s only Option::ok_or and no Option::err_or (Changing the Some reason into a Result::Err).

A better name could be used for this. Maybe check_abort_reason or simply check_abort ?

Copy link
Member

Choose a reason for hiding this comment

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

What about check_and_clear_abort_reason?
Making it obvious that the abort reason is cleared isn't wrong, so it can be kept in the name. And check_ doesn't imply that getting an abort reason is a successful result, in the same way as read_ does.

rp2040-hal/src/i2c/peripheral.rs Outdated Show resolved Hide resolved
rp2040-hal/src/i2c/peripheral.rs Outdated Show resolved Hide resolved
rp2040-hal/src/i2c/peripheral.rs Outdated Show resolved Hide resolved
rp2040-hal/src/i2c/peripheral.rs Outdated Show resolved Hide resolved
@jannic
Copy link
Member

jannic commented Feb 6, 2024

IMHO this pull request contains unrelated changes that could be discussed and merged separately.

Changes that could be separate pull request include:

  • unconditionally including defmt.x in the linker options
  • adding on-target-tests to the top-level workspace
  • some cleanups to the CI pipeline

Or am I missing something and these changes are inherently connected with the async i2c support?
(That's part of the problem: Because these changes are part of a bigger pull request, they are missing proper reasoning. If they were separate, there would probably be an explanation in the description of the pull request.)

They have been updated and now build fine
@ithinuel ithinuel force-pushed the introduce-async-i2c branch 2 times, most recently from 40e95d5 to be9b0ab Compare February 6, 2024 23:17
@ithinuel ithinuel changed the title (re)introduce async i2c. [i2c] (re)introduce async.await implementation Feb 6, 2024
@ithinuel
Copy link
Member Author

ithinuel commented Feb 6, 2024

@jannic I understand. I created a PR for each commit (and address the previous reviews)

@jannic
Copy link
Member

jannic commented Feb 7, 2024

@jannic I understand. I created a PR for each commit (and address the previous reviews)

Thank you!

BTW, as many review comments I made sound so negative: That's just because I'm actively looking for things that I don't particularly like. Overall, you did a great job, and I'm sorry that it takes me so long to complete the review. It's just a lot of changes, and I'm not experienced enough with async rust that I can quickly glance over the changes to understand what's going on.

//! | from GPIO (pico Pin) | to GPIO (pico Pin) |
//! | -------------------- | ------------------ |
//! | 0 (1) | 2 (4) |
//! | 1 (2) | 3 (5) |
Copy link
Member

Choose a reason for hiding this comment

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

Please also mention this in on-target-tests/README.md. It already describes the connection necessary for the SPI test, so the I2C connections should be there as well.

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.

I would be lying if I said I understood every detail of this pull request.
But as far as I understand it, it looks good.

As github shows all the changes in the pull request anyway, even if some of the commits were actually already merged by some other pull requests, I did not review #763 and #764 separately. Not sure what's the best way to review multiple changes individually, short of having completely separate branches.

Also adds "-Tdefmt.x" to .cargo/config.
This required for i2c async examples & on-target-tests.
Note this does not affect downstream dependents.
@ithinuel
Copy link
Member Author

If merging this PR does not auto close the two "preceding", I’ll just close them manually adding the reference to this PR in the comment.
I don’t really mind about the details as long as it’s deemed worthy to be merged and it makes it to main :)

@jannic jannic merged commit 07ba575 into rp-rs:main Feb 10, 2024
6 checks passed
@ithinuel ithinuel deleted the introduce-async-i2c branch February 10, 2024 13:41
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.

3 participants