search

Rahix / avr-device

Register access crate for AVR microcontrollers
Apache License 2.0
170 stars 66 forks source link

`critical-section` support #88

Closed torrancew closed 2 years ago

torrancew commented 2 years ago

In attempting to integrate the avr-hal ecosystem with embedded-nal, I ran into a series of hurdles involving heapless,atomic-polyfill and critical-section.

Ultimately, if avr-device were to implement the API provided by embassy-rs/critical-section, integrating with embedded-nal and its dependencies would be greatly simplified.

Would you be open to a PR that makes the required (minor) changes to avr_device::interrupt to accomodate this? I can see two main ways to go about it:

Option 1: Add support for AVR targets to critical-section by having it depend upon avr_device when building for the AVR platform

The main missing piece here would be a public API that critical-section can use to read the GIE field of SREG. Since this register is not exposed by avr_device, we would need to surface it in some way or another (either a public method that invokes the underlying LLVM assembler, or exposing it in a way more consistent with the SVD-backed registers. This would open the possibility of migrating the avr_device::interrupt::free API to the critical_section::with API, which is equivalent in signature, but not in name.

Side note: I'm unsure on the rationale for patching SREG out of SVDs. Is it simply a matter of not wanting the exposure to be a safe API?

Option 2: Implement critical-section via its custom-impl feature, entirely in avr_device

I believe this would be less ergonomic for users who inadvertently pull in both, but could be mistaken here, as I haven't had a lot of exposure to the nuances of Cargo features & how they propagate in complex dependency graphs. The only change to the avr-device API in this case would be the net-new implementation of the trait provided by critical-section (which closely mirrors the existing interrupt API).

In either case I'm more than willing to write the patches, both to avr_device and to critical-section, but wanted to see if there was a preferred direction on your end.

Rahix commented 2 years ago

Hi,

yes, I think adding the necessary API to support critical-section is a good idea and "Option 1" is certainly the better way forward. Adding a helper to check the status of the interrupt bit is not a problem and might be beneficial beyond this usecase.

Side note: I'm unsure on the rationale for patching SREG out of SVDs. Is it simply a matter of not wanting the exposure to be a safe API?

In the form generated from svd2rust, it would allow circumventing safety relevant invariants from safe code. Other architecture crates solve this by just reimplementing the accessors for such registers and fields manually with safety in mind (making critical parts unsafe). In avr-device we haven't gotten around to adding that yet, but in principle, this is of course something we should also provide at some point.