`Gpio` API about embedded-hal HOT 11 CLOSED

What methods should it provide?

I'd say just set_low and set_high for an Output trait and is_low and is_high for an Input trait. Configuration should not be a concern of this HAL, IMO.

Output could also have a toggle method. My concern there is that some microcontrollers may not have a method to do this atomically unless one has exclusive access to the GPIO peripheral / port output register.

We probably should have another trait to work at the "port" level: one byte or a half-word at a time.

from embedded-hal.

While separating the concern of use and configuration should be fine for most sub-systems in theory, GPIOs are one specific case where it is quite common to change the configuration during run-time, i.e. to change from output with a specific state (LOW/HIGH) to input to make the pin floating.

from embedded-hal.

A lot of devices have their normal communication interface (SPI etc) and separately some interrupt lines to let you know data is ready or whatever. A lot of device drivers using embedded-hal will want some way to be told that the interrupt has occurred (though often they also need a way to be told "the interrupt is not wired up, please poll/do something else instead").

One simple way is to just have the driver expose a method that the user arranges to be called when the interrupt happens, but maybe there's scope for passing the driver a Gpio-type object which can be awaited for a pin change interrupt.

from embedded-hal.

@therealprof makes sense

@adamgreig

A lot of device drivers using embedded-hal will want some way to be told that the interrupt has occurred (though often they also need a way to be told "the interrupt is not wired up, please poll/do something else instead").

That sounds a bit like overstepping into the application logic. It's up to the application to decide whether to use the driver in interrupt context (in response to a pin change) or to continuously poll one of the device's pin. I think the driver API should support both modes but not dictate which one to use.

I may be misunderstanding what you are saying though. In my mind the driver API should allow these two modes:

fn callback_on_pin_changed() {
    driver.do_something();
}

while data_is_ready_pin.is_low() {}

driver.do_something();

Whereas an API like this:

driver.do_something_if(&data_is_ready_pin)

feel less useful regardless of whether it's blocking or async. I mean it's fine if that's a convenience method / function but it should not be the only / main API.

from embedded-hal.

Okay, that's fair. I want to try porting a radio modem driver from C to Rust some day reasonably soon, which might yield further thoughts on this. A lot of the wait states are in internal operations that are annoying to have to suspend and then ask the user to resume based on some event, but perhaps that will be mitigated by async support anyway.

from embedded-hal.

As I see it in order to get GPIO right one must think carefully about which operations need to be atomic. As soon as we think about ISRs changing pin direction the problem becomes apparent. If an ISR interrupts the main thread in the middle of a modification to the direction of one pin and the ISR changes the direction of another pin on the same port the change the ISR made will be lost because the main thread has a local copy of the direction register which is outdated.

We have the same problem with modifying output state but there is usually hardware support for making these operations trivially atomic. We theoretically have the same problem with every read-modify-write operation on a bitfield in a SFR which contains other bitfields which are modified in other interrupt contexts. This theoretical problem often becomes a practical one with initialization type registers like power or clock management as well as buffer status registers if TX and RX share a reg and are intended to run in different priorities (send from main thread but receive in ISR to avoid overflow for example). Some CAN peripherals in particular are tricky in this respect.

from embedded-hal.

@odinthenerd All these data races / race conditions you mention are rather easy to spot in Rust. At least with current "you must have a reference (&-) to a peripheral to modify it" model. Basically all read modify write operations that may occur in preemptible context will require a critical section:

extern crate stm32f103xx;

use stm32f103xx::GPIOA;

fn main() {
    // critical section: the RMW operation is atomic
    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        let old = gpioa.odr.read().bits();
        let new = old ^ 1;
        gpioa.odr.write(|w| w.bits(new));
    });
}

fn isr() {
    // critical section: the RMW operation is atomic
    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        let old = gpioa.odr.read().bits();
        let new = old ^ 1;
        gpioa.odr.write(|w| w.bits(new));
    });
}

Of course nothing stops you from creating a race condition in safe Rust (data races are banned in safe Rust, but race conditions are safe (or rather they don't require unsafe)) by doing the RWM operation in a non-atomic fashion:

extern crate stm32f103xx;

use stm32f103xx::GPIOA;

fn main() {
    // uses critical sections, but the RMW operation is NOT atomic
    let old = interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        gpioa.odr.read().bits()
    });

    let new = old ^ 1; // can be preempted

    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        gpioa.odr.write(|w| w.bits(new));
    });
}

fn isr() {
    // uses critical sections, but the RMW operation is NOT atomic
    let old = interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        gpioa.odr.read().bits()
    });

    let new = old ^ 1; // can be preempted

    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        gpioa.odr.write(|w| w.bits(new));
    });
}

But I think it should be rather obvious from the code that you are doing something wrong. In C the compiler would probably be happy to compile this for you:

#include <stm32f103xx.h>

int main() {
  int old = GPIOA->ODR;
  int new = old ^ 1;
  GPIOA->ODR = new;
}

void isr() {
  int old = GPIOA->ODR;
  int new = old ^ 1;
  GPIOA->ODR = new;
}

Unsynchronized global mutation. This has data races and race conditions, but no hint that something bad may be happening -- specially if the definitions of main and isr are not right next to each other.

from embedded-hal.

if you achieve atomicity by turning off interrupts it makes it really hard to reason about the theoretical maximum amount of time that interrupts could be off for.

from embedded-hal.

if you achieve atomicity by turning off interrupts

That's one way but not the only way. In Real Time for The Masses (RTFM) v2 you can achieve atomicity by disabling sharing: basically you assign ownership of a peripheral to main or to an interrupt then you can directly access the peripheral from that context without any critical section. When you do have sharing of resources between interrupts that run at different priorities then only the lower priority interrupt needs a critical section whereas the higher priority task can directly access the resource. OTOH, when interrupts than run at the same priority share a resource no critical section is needed as no preemption is possible. All this greatly reduces the number of critical sections. Also in RTFM critical sections don't disable all the interrupts, just the ones that may also access a shared resource -- this greatly reduces blocking time.

This approach falls within the "you must have a reference (&-) to a peripheral to modify it" model. You could probably also implement a CAS loop approach on top of the reference model but I haven't explored that area myself.

it makes it really hard to reason about the theoretical maximum amount of time that interrupts could be off for.

Hard how? By only looking at the code? Sure, more critical sections are harder to analyze by hand. But if you really care about the real time properties of your system then you are probably using proper computer assisted WCET and scheduling analysis.

Also, this has gone very off topic. Let's please return to the topic of the GPIO API.

from embedded-hal.

We have shipped a digital::OutputPin trait in v0.1.0 of this crate. We are still missing traits for input pins, output ports, pins that operate in both input and output mode, etc. I'm going to close this issue in favor of several more targeted issues.

from embedded-hal.

in favor of several more targeted issues.

See #29, #30 and #31

from embedded-hal.