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

Button driver #369

Merged
merged 12 commits into from
Feb 26, 2022
Merged

Button driver #369

merged 12 commits into from
Feb 26, 2022

Conversation

alexandruradovici
Copy link
Contributor

The PR adds the button driver. Reading the number of buttons and their states work.

@jrvanwhy I am not sure if I used the subscribe correctly. The idea is to provide two subscribe related functions:

  1. wait_for_pressed and wait_for_released using futures that await the press or release of a button. I could not figure out how to use futures here, as share::scope is not async;
  2. register_for_events that takes two closures, one called when ever there is button press or release and another one that allows apps to run code while being subscribed. I am not sure if this is the exact intended use of the subscribe system call.

@jrvanwhy
Copy link
Collaborator

jrvanwhy commented Feb 1, 2022

The intended design was more like:

use libtock_platform::{ErrorCode, share, subscribe, Subscribe, Upcall};

pub enum ButtonEvent {
    Pressed,
    Released,
}

pub struct ButtonListener<F: Fn(idx: u32, event: ButtonEvent)>(fcn: F);

impl<F: Fn(idx: u32, event: ButtonEvent)> Upcall<subscribe::OneId<3, 0>> for ButtonListener<F> {
    fn upcall(&self, idx: u32, pressed: u32, _arg2: u32) {
        // I'm omitting error handling for the case where pressed is not 0 or 1 here to keep code size down.
        // I suspect that coercing non-1 values into 1 results in the smallest code on most architectures.
        let event = match pressed {
            0 => ButtonEvent::Released,
            _ => ButtonEvent::Pressed,
        };
        self.fcn(idx, event);
    }
}

pub struct Button<S: Syscalls>(S);

impl<S: Syscalls> Button<S> {
    fn register_listener<'share, F: Fn(idx: u32, event: ButtonEvent)>(
        &self, listener: &'share ButtonListener<F>, subscribe: share::Handle<Subscribe<'share, S, 3, 0>>)
        -> Result<(), ErrorCode>
    {
        S::subscribe(subscribe, listener)
    }
}

The share::scope call belongs in the app's code, not in libtock_buttons.

@alexandruradovici
Copy link
Contributor Author

This makes sense, thank you for the feedback, I'll redesign it.

I do have a question related to the async functions. My understanding is that it should receive the handle from the application's scope.

pub async fn wait_switch_pressed<'share>(
        button: u32,
        subscribe: Handle<'_, Subscribe<'share, S, DRIVER_ID, 0>>,
    ) -> Result<(), ErrorCode> {
        let called = Cell::<Option<(u32, u32)>>::new(None);
        Self::enable_interrupts(button)?;
        S::subscribe::<_, _, DefaultConfig, DRIVER_ID, 0>(subscribe, &called)?;
        futures::wait_until(|| {
            if let Some((pressed_button, 1)) = called.get() {
                button == pressed_button
            } else {
                false
            }
        })
        .await;
        S::unsubscribe(DRIVER_ID, 0);
        let _ = Self::disable_interrupts(button);
        Ok(())
    }

The problem that I am facing is that called does not live long enough. Any suggestions would be very helpful. The envisioned usage of this function is:

share::scope(|subscribe| {
  futures::block_on(async {
    Button::wait_for_pressed(0, subscribe).await;
  });
});

As the main function is not async anymore, my example might not be the right one.

@jrvanwhy
Copy link
Collaborator

jrvanwhy commented Feb 2, 2022

I wasn't planning to add any Futures-based APIs to libtock-rs, and haven't really thought about what that integration would look like.

@alexandruradovici
Copy link
Contributor Author

I redesigned the API and deleted the usage of async. This should be ready for review.

@alexandruradovici alexandruradovici marked this pull request as ready for review February 2, 2022 23:56
apis/buttons/src/lib.rs Show resolved Hide resolved
apis/buttons/src/lib.rs Show resolved Hide resolved
unittest/src/fake/buttons/mod.rs Show resolved Hide resolved
unittest/src/fake/buttons/mod.rs Outdated Show resolved Hide resolved
unittest/src/fake/buttons/mod.rs Outdated Show resolved Hide resolved
unittest/src/fake/buttons/tests.rs Outdated Show resolved Hide resolved
apis/buttons/src/lib.rs Outdated Show resolved Hide resolved
unittest/src/fake/buttons/mod.rs Outdated Show resolved Hide resolved
unittest/src/fake/buttons/mod.rs Outdated Show resolved Hide resolved
apis/buttons/src/tests.rs Outdated Show resolved Hide resolved
unittest/src/fake/buttons/mod.rs Outdated Show resolved Hide resolved
@jrvanwhy
Copy link
Collaborator

bors d+

IIRC, this PR is not waiting on anything.

@bors
Copy link
Contributor

bors bot commented Feb 26, 2022

✌️ alexandruradovici can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@alexandruradovici
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 26, 2022

Build succeeded:

@bors bors bot merged commit b6d744f into tock:master Feb 26, 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