search

rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
2.01k stars 202 forks source link

async: add digital::Wait. #346

Closed Dirbaio closed 2 years ago

Dirbaio commented 2 years ago

Add digital::Wait trait.

This is the previously proposed WaitForX traits, unified in a single one supporting both edge-triggered and level-triggered waits.

It is possible to software-emulate edge-triggered out of level-triggered hardware and vice-versa, so requiring support for both shouldn't make it unimplementable for any MCU. It is a good thing to require both: for example, stm32 EXTI is edge-triggered, but drivers usually want level-triggered. It'd be bad if a HAL decided "the hardware only supports edge-triggered, so I'm only going to impl edge-triggered!".

Impl for nRF's GPIOTE here: https://github.com/embassy-rs/embassy/blob/master/embassy-nrf/src/gpiote.rs Impl for STM32's EXTI here: https://github.com/embassy-rs/embassy/blob/master/embassy-stm32/src/exti.rs

(impls still not unified, but unifying shouldn't be an issue).

ryankurte commented 2 years ago

bikeshedding but, do we have any preference for wait_for_high() and wait_for_low() vs. wait_for_state(_: PinState)? not sure whether it's more convenient to have less or separate future types.

Liamolucko commented 2 years ago

Would this trait only be meant for implementing on pins themselves, or could it also be used elsewhere?

The case I'm thinking of is on the nrf51, which can't implement it on plain pins (in the same way, at least) because it lacks a latch register; would it be implemented on something like a GPIOTE channel instead?

Dirbaio commented 2 years ago

@ryankurte IMO the proposed methods are more consistent with InputPin, OutputPin, which have is_high/is_low, set_high/set_low instead of get_state, set_state. I do kinda think using "state" is slightly nicer, but IMO consistency with InputPin/OutputPin is more important. (Unless we change InputPin/OutputPin....? :see_no_evil: )

@Liamolucko they could be implemented on pins or on something else, depending on the hardware. On embassy-nrf they're implemented on individual pins. On stm32 using EXTI needs to reseve an "EXTI channel", so in embassy-stm32 they're implemented in a separate struct ExtiInput (which also impls InputPin, so it's like an "input pin with extra capabilities"). I don't know about nrf51, I guess it could impl it for GPIOTE channels if it doesn't have the GPIOTE PORT event.

ryankurte commented 2 years ago

@ryankurte IMO the proposed methods are more consistent with InputPin, OutputPin, which have is_high/is_low, set_high/set_low instead of get_state, set_state. [...] Unless we change InputPin/OutputPin....? see_no_evil )

we do actually have a default OutputPin::set_state these days, though curiously no InputPin::get_state.

in this instance it would seem to me to be simpler to provide default impls for wait_for_high and wait_for_low based on wait_for_state(s: PinState) (as well as the equivalent for edges) than the opposite case, though this is the opposite to OutputPin. either way i suggest we provide both options using defaults.

Dirbaio commented 2 years ago

Oh, I didn't know about set_state. hmm... Doesn't "state" refer to the "output pin state", like in StatefulOutputPin though? Are we OK with "state" both referring to the "output state" and the "input state" in different traits? The Wait trait is about input.

Also, there's no way to provide default method impls with GAT-based async traits. Given this, I think it's better to not include these "helper methods" for now, because they'll force every implementor to include boilerplate. There hopefully will with the future async-fn-in-traits, so we can revisit later when we switch to it.

Dirbaio commented 2 years ago

friendly ping @ryankurte @eldruin could we unblock this?

I believe wait_for_high, wait_for_low is the most consistent with InputPin's is_high, is_low.

Either way, all GPIO traits should be consistent (either all using "state" or all using "high/low"), so if someone feels the "state" based API is better please open an issue and we can discuss and then change all traits (not just Wait), but let's not let that block this (especially since for a while the cost of breaking embedded-hal-async will stay low)

bors[bot] commented 2 years ago

Build succeeded: