golemparts/rppal

TODOs to research/implement before gpio is ready for a 0.10.0 release

golemparts opened this issue · 2 comments

golemparts commented
  • InputPins created using different instances of Gpio won't share the same EventLoop, which means they'll be added to the wrong EventLoop instance by InputPin::set_interrupt() when a user tries to poll multiple sync interrupts using Gpio::poll_interrupts(). Either share a single static EventLoop instance, or combine add/poll/remove into a single method.
  • Running InputPin::poll_interrupt() for different pins on multiple threads simultaneously will block all but one due to the required Mutex lock. Find a better solution. Update documentation.
  • 32-bit reads/writes to 32-bit aligned memory locations are atomic on ARMv6/v7/v8 (see ARM Architecture Reference Manuals). Verify GpioMem's mmap configuration (O_SYNC, MAP_SHARED) doesn't negate atomicity and doesn't cause any caching-related issues, and then remove the locking mechanism for single reads/writes. Locks are still needed when multiple operations need to be synchronized (set_mode, set_pullupdown).
  • Consider the consequences of removing the single instance limitation on Gpio, now that Pins survive a Gpio instance going out of scope.
  • Consider moving the contents of Gpio to a static variable, and have Gpio::new() either clone the static variable if it has been created already, or initialize it and return any relevant errors. (This also means GPIO_MEM_LOCKS can be moved back to a local struct field)
  • Give each Pin a cloned GpioState.
  • Use a weak reference for the shared static GpioState, so it'll automatically get dropped when all Gpio and Pin instances go out of scope.
  • Move the static PINS_TAKEN to a local GpioState struct field.
  • Consider Gpio::take() versus Gpio::get(). Option::take() and HashSet::take() similarly remove the returned value from where it was taken. However, Pin can be taken again after it goes out of scope, so that makes the expected functionality different. get() methods usually return a temporary reference instead of an owned value, but after the returned reference goes out of scope it can be retrieved again, similar to what happens when a Pin is dropped.
  • Disable pull-up/pull-down when InputPin is dropped and clear_on_drop==true.
  • Expand pin documentation in the main section of the gpio module.
  • Add a section on interrupts to the gpio module documentation.
  • Investigate if we can lock a pin through /dev/gpiochipN to prevent other processes from gaining access simultaneously, while still using /dev/gpiomem for fast register access. See comment below.
  • Using the gpio module for charlieplexing or bitbanging I2C requires switching a pin between input and output mode. This should be easier to do than dropping the derived pin and calling Gpio::get() again for each mode switch. Possible solutions:
    • Add into_ methods to InputPin and OutputPin to switch pin types.
    • Add a new pin type (IoPin?) that allows users to switch between input and output mode. The new pin type could allow users to set any mode, replacing AltPin.
    • Add a set of methods to either Pin or InputPin and OutputPin that either return a temporary reference, or take a closure that is called with a reference to a pin of the appropriate type.
  • Change Gpio::get() to return a Result rather than an Option for uniform error handling. Return an Error::PinNotAvailable when a pin is already in use, or when the pin number isn't supported on the current Pi model.
  • Consider removing Gpio::new(), and instantiate/clone GpioState from within Gpio::get(). Gpio::poll_interrupts() would need some tweaks as well. Pro: Single method that can return any applicable errors. Con: Need to lock a mutex for each call. No uniform API for all peripherals. Reconsider for 0.11.0.
golemparts commented

After doing some research/testing on using /dev/gpiochipN to lock pins for exclusive access, I've come to the conclusion the interface just isn't mature enough for this yet.

  • /dev/gpiochipN must be re-opened through a new fd each time we want to request a handle to lock a pin for exclusive access. If we continue to use an already open /dev/gpiochipN after another process takes and then releases a handle, our handle request's fd will be 0 (indicating an error).
  • Requesting a handle requires providing a pin mode (if flags is kept 0, the pin mode will still be changed to Input). Since the ioctl interface doesn't yet provide a way to set (or read) any of the ALT function modes, that means requesting a handle will always overwrite those modes unless we subsequently reset them through the registers, which ends up requiring a messy workaround at a point where we don't want to mess with the pin's mode (read the current mode through /dev/gpiomem, open /dev/gpiochipN, request a handle (which briefly changes the pin mode to Input), reset the mode through /dev/gpiomem, create a Pin instance).

I'll revisit this idea at some future point, after additional Raspberry Pi-specific features are added to /dev/gpiochipN (support for setting pull-ups/pull-downs would be nice to have as well).