dio: Make digital PWM RAII

[auscompgeek]

No description provided.

[Lytigas]

With the handle, isn't it always enabled?

[auscompgeek]

Good point. I probably forgot to remove this when moving things around.

[auscompgeek]

Actually re-reading this, this doesn't take a handle, so this comment is correct.

[Lytigas]

Isn't this method only usable on a DigitalPwm, which will alwys have PWM enabled?

[auscompgeek]

This is an associated function, not a method.

[Lytigas]

I like the idea, but I think the borrowing creates a unwieldy interaction with borrowck.

struct PWMMechanism<'a> {
    dio: DigitalOutput,
    pwm: DigitalPwm<'a>,
}

impl<'a> PWMMechanism<'a> {
    fn new(port: i32) -> Self {
        let mut d = DigitalOutput::new(port).unwrap();
        let p = d.enable_pwm(0.5).unwrap();
        Self { dio: d, pwm: p }
    }
}

error[E0515]: cannot return value referencing local variable `d`
  --> wpilib-examples/./digital_pwm.rs:20:9
   |
19 |         let p = d.enable_pwm(0.5).unwrap();
   |                 - `d` is borrowed here
20 |         Self { dio: d, pwm: p }
   |         ^^^^^^^^^^^^^^^^^^^^^^^ returns a value referencing data owned by the current function

error[E0505]: cannot move out of `d` because it is borrowed
  --> wpilib-examples/./digital_pwm.rs:20:21
   |
16 | impl<'a> PWMMechanism<'a> {
   |      -- lifetime `'a` defined here
...
19 |         let p = d.enable_pwm(0.5).unwrap();
   |                 - borrow of `d` occurs here
20 |         Self { dio: d, pwm: p }
   |         ------------^----------
   |         |           |
   |         |           move out of `d` occurs here
   |         returning this value requires that `d` is borrowed for `'a`

error: aborting due to 2 previous errors

I think a better solution would be to have enable_pwm take ownership, and return a Result<DigitalPWM, (Err, DigitalOut)>. The DigitalPWM (name tbd) struct has the functionality in that state, and a downgrade method back to DigitalOut.

[auscompgeek]

Note to self: add some sort of test to ensure using this actually compiles.

[Lytigas]

With this API, in the case of a failure, the DIO is lost. I'm not sure if its possible to recover here, or if it's worth trying, but if it is recoverable we could return Result<DigitalPWM, (HalError,Self)> or something similar.

[auscompgeek]

Such a Result would make it difficult to Try.

[Lytigas]

I agree, but in a realistic usecase where you want to recover from a failed upgrade, this API requires you to recreate the DigitalOut. Using ? requires just .map_err(|(e, _p)| e), which we can put in an example.

[auscompgeek]

I'm struggling to find a good reason to recover from an error in this scenario though. What would you do in such a scenario? The Nidec Brushless requires a DIO pin in PWM mode, for example.

[Lytigas]

In the general case of a hardware output failure, you basically have two options:

1. Retry Later
2. Abandon and restart
3. Ignore it and carry on running your robot

The failure mode in which 1. could be useful is possible resource exhaustion *status = NO_AVAILABLE_RESOURCES; in HAL_AllocateDigitalPWM.

It's also more conducive to dependency injection. Say I get a DigitalOutput injected; I don't know it's port. How do I recover from this error? Maybe I just wanted to try using PWM if it's available. If I only want to use PWM sometimes, I'd like to keep my DigitalOutput around for other use.

These use cases / failure modes are unlikely enough that I'm willing to merge it either way, but that's my perspective.

[Lytigas]

Isn't this method only usable on a DigitalPwm, which will alwys have PWM enabled?