search

imxrt-rs / imxrt-hal

Rust for NXP i.MX RT
Apache License 2.0
122 stars 29 forks source link

GPIO: embedded-hal 1.0 compatibility #148

Closed Finomnis closed 5 months ago

Finomnis commented 6 months ago

Could you share a minimum example that's fixed by 64c3af9? Or, could you help me understand why we need this fix?

64c3af9 adds critical sections inside routines that (indirectly) require a &mut Port<...> exclusive reference. By requiring the exclusive reference, we've made it the user's responsibility to avoid this race condition. If they're sharing their Port object across execution contexts, they'll need to wrap it in something like critical_section::Mutex.

You might be right, this might have been an oversight on my part. I might have carried over some thoughts from LPSPI, where it will be necessary that an interrupt disables itself upon getting fired, making this critical section necessary there.

I will revert that change.

Finomnis commented 6 months ago

@mciantyre Ready for more reviews. Any leftover criticism/comments?

Otherwise I'd like to merge this as a base for the other eh1 implementations. Especially as it's only a non-breaking minor change.

Finomnis commented 5 months ago

@mciantyre Ping :) Or do you want to wait for the eh-1 release?

Finomnis commented 5 months ago

@mciantyre Ready for review again. Now update to eh-1.0 release.

Finomnis commented 5 months ago

I rebased onto commits that fix clippy warnings in other code, simplified the history, and added a CHANGELOG entry.

I dropped the MSRV recommendation. It's not clear why we should target 1.60, or why it's necessary to adopt an MSRV for this PR. And without a CI job, I'm afraid it would be difficult to maintain the MSRV ourselves. If you have a need for an MSRV, we can discuss in a separate issue.

A agree with all of your points.