Skip to content

Implement embedded-hal 1.0 traits#431

Merged
qwandor merged 10 commits intonrf-rs:masterfrom
qwandor:embedded-hal
Mar 6, 2024
Merged

Implement embedded-hal 1.0 traits#431
qwandor merged 10 commits intonrf-rs:masterfrom
qwandor:embedded-hal

Conversation

@qwandor
Copy link
Copy Markdown
Member

@qwandor qwandor commented Jan 18, 2024

Now that embedded-hal 1.0 is released, this PR adds implementations of most of its relevant traits. For now I've added them alongside the existing embedded-hal 0.2 trait implementations, so that users can still use drivers based on the old traits. We can remove the old traits in a later release.

I haven't implemented the embedded-hal-nb and embedded-hal-async traits yet, and the I2C traits are also missing as the design has changed and implementing it correctly is non-trivial.

@qwandor
Copy link
Copy Markdown
Member Author

qwandor commented Jan 29, 2024

Hi, could I get a review of this soon? It's blocking me from updating some other things to embedded-hal 1.0.

@diondokter
Copy link
Copy Markdown
Contributor

@qwandor Hi, that depends a bit on: #432

In essence, this crate has no active maintainers...

Comment thread nrf-hal-common/src/delay.rs Outdated
const MAX_RVR: u32 = 0x00FF_FFFF;

let mut total_rvr = us * (HFCLK_FREQ / 1_000_000);
let mut total_rvr = ns * (HFCLK_FREQ / 1_000_000_000);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

HFCLK_FREQ is 64_000_000, so isn't this always going to come out as zero?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Whoops, good point. Fixed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That fix looks correct to me, and I can verify that the SYST-based DelayNs implementation works given the fix, when it didn't before.

But I'm not a maintainer of this repository, and I haven't read through this whole PR.

@BartMassey
Copy link
Copy Markdown
Member

I'll try these out with some of my stuff over the weekend and let you know if I see anything. I have only reviewed a couple of these so far — will get to the rest shortly.

@qwandor qwandor merged commit 36a479e into nrf-rs:master Mar 6, 2024
@qwandor qwandor deleted the embedded-hal branch March 6, 2024 12:59
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.

4 participants