Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPIO driver #367

Merged
merged 10 commits into from
Mar 11, 2022
Merged

GPIO driver #367

merged 10 commits into from
Mar 11, 2022

Conversation

alexandruradovici
Copy link
Contributor

@alexandruradovici alexandruradovici commented Jan 27, 2022

This PR implements an initial version of the GPIO driver.

Roadmap:

  • implement pin controls (input / output)
  • implement iterator
  • implement interrupts

Iterator Implementation

I am not quite sure how to implement the iterator in such a way as to prevent its multiple instantiation. The previous version of libtock was using the driver factory idea and used mutability and lifetimes to prevent this. As the current driver model is using associated functions instead of methods, this does not seem possible.

impl<S: Syscalls> Gpio<S> {
    pub fn gpios() -> Gpios<'static, S> {
        let num_gpios = S::command(DRIVER_ID, GPIO_COUNT, 0, 0)
            .get_success_u32()
            .unwrap_or_default() as usize;
        Gpios {
            num_gpios,
            current_gpio: 0,
            _syscalls: &PhantomData,
        }
    }
}

One idea is to use runtime checks and a mutable static, but this (I think) implies unsafe code. Any suggestions are very much welcome.

Usage Examples

Output

let gpios = Gpio::gpios();
let output_pin: Output<...> = gpios.nth(i)?.into();
output_pin.set();

Input

let gpios = Gpio::gpios();
let mut input_pin: Input<PullUp> = gpios.nth(i)?.into();
input_pin.read();

@jrvanwhy
Copy link
Collaborator

Do you have an intended use case for the iterator API? I'm not sure it is necessary, and if it is nontrivial to design then it's probably better to leave it out.

@alexandruradovici
Copy link
Contributor Author

The idea behind the iterator is to prevent the double usage of the same pin. Users might (mistakenly) initialize the same pin at the same time for input and output.

let output_pin: Output<...> = Gpio::pin(pin_number);
let input_pin: Input<...> = Gpio::pin(pin_number);

Using an iterator should prevent this, as borrowed pins have to go out of scope before they can be recreated. Of course, this holds true if and only if it is impossible to obtain another iterator.

@jrvanwhy
Copy link
Collaborator

The idea behind the iterator is to prevent the double usage of the same pin. Users might (mistakenly) initialize the same pin at the same time for input and output.

In the majority of cases, it should be obvious from the hardware design whether a given pin should be an input or an output. So the main way I would expect that error to occur would be for someone to assign a pin an incorrect pin number that conflicts with another pin.

I don't think there's any easy way to defend against that, and I don't think it's worth the complexity of trying to make the pin objects singletons.

@alexandruradovici
Copy link
Contributor Author

In the majority of cases, it should be obvious from the hardware design whether a given pin should be an input or an output. So the main way I would expect that error to occur would be for someone to assign a pin an incorrect pin number that conflicts with another pin.

I'm assuming here a typing mistake rather than a design mistake.

I don't think there's any easy way to defend against that, and I don't think it's worth the complexity of trying to make the pin objects singletons.

With memory allocations enabled, we could use a Vec and a runtime check. If you think this is too complicated, I'll just continue without the singleton.

@jrvanwhy
Copy link
Collaborator

With memory allocations enabled, we could use a Vec and a runtime check. If you think this is too complicated, I'll just continue without the singleton.

Definitely too complicated, in my opinion.

@alexandruradovici
Copy link
Contributor Author

I updated the API not to use an iterator.

Input pins are imuntable borrows, while output pins are mutable borrows.

let mut pin = Gpio::get_pin(0).unwrap();

let input_pin: InputPin<_, PullNone> = pin.make_input().unwrap();
let input_pin2: InputPin<_, PullNone> = pin.make_input().unwrap();
input_pin.read();
input_pin2.read();

let mut output_pin: OutputPin<_> = pin.make_output().unwrap();
output_pin.set();
output.clear();

@jrvanwhy do you think this is going in the right direction?

@jrvanwhy
Copy link
Collaborator

jrvanwhy commented Feb 1, 2022

Input pins are imuntable borrows, while output pins are mutable borrows.

let mut pin = Gpio::get_pin(0).unwrap();

let input_pin: InputPin<_, PullNone> = pin.make_input().unwrap();
let input_pin2: InputPin<_, PullNone> = pin.make_input().unwrap();
input_pin.read();
input_pin2.read();

let mut output_pin: OutputPin<_> = pin.make_output().unwrap();
output_pin.set();
output.clear();

@jrvanwhy do you think this is going in the right direction?

That seems reasonable to me, yes. I don't think the borrows on the output pins need to be mutable borrows, but that's also not a big deal to me.

@alexandruradovici
Copy link
Contributor Author

Input pins are imuntable borrows, while output pins are mutable borrows.

let mut pin = Gpio::get_pin(0).unwrap();

let input_pin: InputPin<_, PullNone> = pin.make_input().unwrap();
let input_pin2: InputPin<_, PullNone> = pin.make_input().unwrap();
input_pin.read();
input_pin2.read();

let mut output_pin: OutputPin<_> = pin.make_output().unwrap();
output_pin.set();
output.clear();

@jrvanwhy do you think this is going in the right direction?

That seems reasonable to me, yes. I don't think the borrows on the output pins need to be mutable borrows, but that's also not a big deal to me.

The idea behind the mutable borrow is to follow Rust's rules. While an input pin a a read action, an output pin is a write action. It is also true that due to how system calls work, these writes to a pin are atomic (from the app's point of view). Still, developers might take the same pin twice and write to it from several places.

If you think this is not worth the trouble, I can switch it to immutable.

@alexandruradovici
Copy link
Contributor Author

This is ready for review.

apis/gpio/src/lib.rs Outdated Show resolved Hide resolved
apis/gpio/src/tests.rs Outdated Show resolved Hide resolved
@jrvanwhy
Copy link
Collaborator

jrvanwhy commented Mar 8, 2022

@alexandruradovici reminder about this PR -- it currently has merge conflicts.

@alexandruradovici
Copy link
Contributor Author

@alexandruradovici reminder about this PR -- it currently has merge conflicts.

Sorry about this, solved.

@hudson-ayers
Copy link
Contributor

@alexandruradovici the merge commit seems to fail rustfmt

@alexandruradovici
Copy link
Contributor Author

Fixed

Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 11, 2022

Build succeeded:

@bors bors bot merged commit d575835 into tock:master Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants