rubberduck203 commented 4 years ago

Motivation

Most boards have one or more LEDs on them for application use. Because operating these lights is simple IO, the temptation is to simply use an OutputPin to control them. However, depending on how the LEDs are wired into the circuit, they may be active high or low. If a programmer uses a raw OutputPin pin to control them, the onus is on them to remember whether set_high() (and it's inverse set_low() turns the light on or off at every use in the source code.

A proper digital::Led trait could remove much of that cognitive overhead, reducing the number of times that needs to be thought about to just the initial creation of the peripheral.

There's an additional benefit of further separation between the application logic and the hardware. If a Led is passed along to the application module, a change to the active high/low behavior of the light at a hardware level could be managed in a single location by changing out which implementation is used.

Requirements

Must abstract away the actual logic level of the GPIO the light is attached to
Must allow for use of GPIO expanders as an underlying hardware implementation.
- i.e. a digital::v2 style Result API.
Should allow for ToggleableLed implementations whenever ToggleableOutputPin is available.

Proposal

I have actually implemented this already in the STM32F3DISCOVERY board support crate I've been working on.

pub trait Led {
    type Error;

    fn on(&mut self) -> Result<(), Self::Error>;
    fn off(&mut self) -> Result<(), Self::Error>;
}

pub trait ToggleableLed {
    type Error;

    fn toggle(&mut self) -> Result<(), Self::Error>;
}

https://github.com/rubberduck203/stm32f3-discovery/blob/236c069c88038efd9aaee2bb6fb971d86e393215/src/leds/hal.rs#L3-L14

One thing of interest here is that in addition to the traits themselves, it's actually possible to implement both ActiveHighLed and ActiveLowLed only in terms of OutputPin.

https://github.com/rubberduck203/stm32f3-discovery/blob/236c069c88038efd9aaee2bb6fb971d86e393215/src/leds/hal.rs#L16-L47

The implementation need not be re-implemented for each different MCU, only instantiated with a concrete implementation of an OutputPin.

let led = ActiveHighLed::new(
                  gpioe
                  .pe9
                  .into_push_pull_output(&mut gpioe.moder, &mut gpioe.otyper)
              );

I believe including a software implementation of a trait would be a first for embedded-hal, but should be considered in this case.

dzarda commented 4 years ago

Fair proposal. Have you considered generalizing the concept beyond LEDs? For example turning on a P-FET (active-low) vs N-FET (active-high)

rubberduck203 commented 4 years ago

That thought had not occurred to me @dzarda. My use of transistors has rarely been "on or off", but usually a PWM input. Which, now that I say it out loud, I've done the same thing with LEDs in the past to control their brightness/fade them on and off. That could be added to the API later though, IMO.

Hmmm... is there a common name for this type of "on/off/toggle" property that LEDs and FETs share?

dzarda commented 4 years ago

Yeah I can't figure out good naming for it either - "active state" or "active level" is best I can come up with.

rubberduck203 commented 4 years ago

Thinking about this more deeply, I also have a proposal brewing in my brain for a Switch trait. Perhaps this is an OutputSwitch.

hannobraun commented 4 years ago

I don't have a strong opinion on this (sounds like a good idea in general), but I'd like to point out that such a trait could not be implemented in HAL crates (which concern themselves with MCUs, not boards). This makes it different from the other traits in this crate.

I remember someone posting about a board-level equivalent to embedded-hal recently. Maybe the trait would be a better fit there. I can't find it again though. Does anyone know what I'm talking about?

rubberduck203 commented 4 years ago

such a trait could not be implemented in HAL crates which concern themselves with MCUs, not boards)

They actually could. In fact, it can be implemented solely in terms of OutputPin with no knowledge of a particular MCU at all. It is indeed different, but because the implementation would actually be included alongside the trait, not because it would need to be implemented by a BSP.

hannobraun commented 4 years ago

Yes, it can be implemented without knowledge of the MCU. But it can't be implemented in the HAL crate, because that doesn't have knowledge of a particular board.

How would you implement that trait without knowing how the LEDs are wired on the board?

rubberduck203 commented 4 years ago

@hannobraun I linked to an implementation in the OP. https://github.com/rubberduck203/stm32f3-discovery/blob/236c069c88038efd9aaee2bb6fb971d86e393215/src/leds/hal.rs#L16-L47

hannobraun commented 4 years ago

And that is a board support crate, which is exactly my point.

rubberduck203 commented 4 years ago

Sure. That's where I originally implemented it, because that's where I needed it, which prompted me to open this issue. If you look at the code I linked, you'll notice that the Led trait implementations do not depend on anything except OutputPin and ToggleableOutputPin. No hardware specific anything involved here.

hannobraun commented 4 years ago

Again, yes, but how would you know whether an LED is active-high or active-low without knowing the specifics of a particular board?

rubberduck203 commented 4 years ago

You don't, but you do when you initialize it. Just like I don't know what pins are attached to SPI when I write a driver, I don't know whether it's active high or low, but the person initializing it does.

This is a bit different from the other traits, because it is both a trait and a driver.

hannobraun commented 4 years ago

Okay, I realize we're not really talking about the same thing. This is my fault, as my comments were misleading. And because I didn't realize that until now, I misunderstood your replies. Let me try again.

All traits in this crate basically have two "target audiences": HAL crates (which implement the traits) and portable driver code (which use the traits). This trait is different (as you noted) because its implementations would live in embedded-hal.

There are still two "target audiences" though. One is the same, portable driver code on one side (using the trait). But the other would be board support crates, which could provide instances of those traits (see example here, of a crate that could do this: [1], [2]). Therefore, one might argue that this trait belongs in the BSP HAL (that I mentioned before, but can no longer find), not in embedded-hal.

I'd like to reiterate that I don't have a strong opinion. I just wanted to point this out.

rubberduck203 commented 4 years ago

Okay. I'm following you now. It's a fair point, but I don't think any BSP would be forced into providing these. It's simple enough for an application developer to instantiate it themselves, and I don't really foresee drivers consuming these. This is really an abstraction for the application developer to use. Which, honestly, also makes this trait a bit different from the status quo.

That's why I wanted to talk about it, because it's not the same two target audiences. The target audiences here are

Application Developers (who can either use ones provided by a BSP or initialize themselves)
Board Support Crates (who or may not leverage them)

dzarda commented 4 years ago

IMO drivers might want to use this as well:

Driver of some networking stack needs to blink link LEDs (duh)
A matrix display driver needs to activate columns and rows, each of which may have inverted logic
Some UART-over-RS-485 IO needs to drive the transceiver output enable line if the UART periph. / driver doesn't support HW OE

david-sawatzke commented 4 years ago

This would be very convenient for me, for this: https://github.com/david-sawatzke/embedded-morse

Maybe also stuff like lcd brightness?

rubberduck203 commented 4 years ago

Oh wow. That’s really interesting. I didn’t think any drivers would want to consume this.

This is an interesting case right here.

                    if self.invert {
                        self.pin.set_low()?;
                    } else {
                        self.pin.set_high()?;
                    }

https://github.com/david-sawatzke/embedded-morse/blob/master/src/lib.rs#L198-L202

This can be accomplished with zero cost using the abstraction I created.

dzarda commented 4 years ago

Inspiration: Linux DeviceTree assigns this attribute to all GPIOs: devicetree/bindings/gpio/gpio.txt

EDIT: I don't think we should merge these two concepts in the same way.

rubberduck203 commented 4 years ago

Maybe also stuff like lcd brightness?

The thought did briefly cross my mind @david-sawatzke.

I created a crate to flush these ideas out. https://crates.io/crates/switch-hal

I'm not sure whether or not the traits belong here, but I do think they're a good idea. Having an external place to work out the kinks seems like a good idea.

rubberduck203 commented 4 years ago

I'm sure there are changes to come, but I feel like the currently published version of switch-hal is stable enough to start playing with if anyone's interested in doing so.

rubberduck203 commented 4 years ago

@david-sawatzke thank you so much for bringing your example to my attention. It's exactly the kind of thing I wanted to create this for.

https://github.com/david-sawatzke/embedded-morse/pull/1/files

The trait removes the code overhead of checking at runtime to see if the pin is active high or low and the mental overhead of remembering which way is which. (It took me a good minute to figure out which piece of code was turning it on and which was turning it off. Making this a great example!)

rust-embedded / embedded-hal

digital::Led trait #185

Motivation

Requirements

Proposal