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

Add more logging to mark-scan #4569

Merged
merged 19 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions apps/mark-scan/accessible-controller/src/controllerd.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
//! Daemon whose purpose is to expose signal from VSAP's accessible controller
//! to the mark-scan application.
//!
//! Signal from the accessible controller is available in userspace over
//! the serialport protocol. The daemon connects to the controller and polls
//! for change in signal value. When a button press is detected, it sends
//! a keypress event for consumption by the mark-scan application.

use serialport::{self, SerialPort};
use std::{
io,
Expand Down
10 changes: 10 additions & 0 deletions apps/mark-scan/pat-device-input/src/patinputd.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
//! Daemon whose purpose is to expose signal from VSAP's input for personal
//! assistive technology (PAT) devices to the mark-scan application.
//!
//! PAT input status is made accessible in userspace with GPIO pins. The daemon
//! connects to the pins then polls their values. When a change in value is read,
//! the daemon sends a keypress event for consumption by the application.
//!
//! Notably, running this daemon is required for the mark-scan app to read PAT
//! device connection status.

use pin::Pin;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Write a //! module-level doc comment? I know a fair amount of the existing Rust code doesn't do this, but I'm trying to encourage better habits here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a75cf84

doc comments added to both daemons and Pin struct

use std::{
io,
Expand Down
24 changes: 24 additions & 0 deletions apps/mark-scan/pat-device-input/src/pin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,33 +10,54 @@ use vx_logging::{log, EventId};
const EXPORT_PIN_FILEPATH: &str = "/sys/class/gpio/export";
const UNEXPORT_PIN_FILEPATH: &str = "/sys/class/gpio/unexport";

/// Pin provides an interface to read GPIO pins that support the sysfs protocol.
///
/// # Example
///
/// ```
/// const PIN_NUMBER = 478;
/// const MY_PIN: Pin = Pin::new(PIN_NUMBER);
/// MY_PIN.set_up()?;
/// let is_active = MY_PIN.is_active();
/// do_something_with_value(is_active);
/// ```
pub struct Pin {
eventualbuddha marked this conversation as resolved.
Show resolved Hide resolved
address: u16,
}

impl Pin {
/// Creates a new instance of Pin at the given address (pin number).
pub const fn new(address: u16) -> Self {
Self { address }
}

// Exports the pin to be globally accessible from userspace.
fn export(&self) -> io::Result<()> {
fs::write(EXPORT_PIN_FILEPATH, self.to_string())
}

// Unexports the pin, removing access from userspace.
fn unexport(&self) -> io::Result<()> {
fs::write(UNEXPORT_PIN_FILEPATH, self.to_string())
}

// Sets the pin direction to "in" so its value is readable. Must be called after
// `export`
fn set_direction_in(&self) -> io::Result<()> {
let filepath = format!("/sys/class/gpio/gpio{self}/direction");
fs::write(filepath, b"in")
}

/// Removes access to the pin from userspace.
pub fn tear_down(&self) {
self.unexport()
.expect("Unexpected failure to tear down pin");
}

/// Makes the pin accessible from userspace. If the pin is already set up due to
/// a previous `set_up` call, or from other callers of the sysfs interface, `set_up`
/// will tear down the pin and attempt one more time to set up. This may cause
/// interruption to other processes attempting to read the pin value.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This brings up the question of whether we should just continue when we see the pin is already exported. IMO since we expect this daemon to be the only exporter it should never rely on the unexpected state of the pin already being exported. ie. it should always be exercising the path to export the pin.

pub fn set_up(&self) -> io::Result<()> {
if let Err(err) = self.export() {
log!(
Expand All @@ -51,6 +72,9 @@ impl Pin {
self.set_direction_in()
}

/// Reads the pin value and returns the boolean inverse of the pin's raw value.
/// # Example
/// When the pin's value is b'0', `is_active` returns true.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this convention is specific to VSAP's implementation, so it could arguably live in the daemon code. But since we have no planned use for this module outside the PAT integration it feels cleaner to leave this translation in the pin module.

pub fn is_active(&self) -> bool {
let filepath = format!("/sys/class/gpio/gpio{self}/value");
let Ok(mut file) = File::open(filepath) else {
Expand Down