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 15 commits
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
34 changes: 34 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ members = [
"libs/logging",
"libs/types-rs",
"apps/mark-scan/accessible-controller",
"apps/mark-scan/pat-device-input",
]

[workspace.dependencies]
base64 = "0.21.4"
bitter = "0.6.1"
clap = { version = "4.0.29", features = ["cargo"] }
crc16 = "0.4.0"
ctrlc = "3.4.2"
hex = "0.4.3"
image = "0.24.5"
imageproc = "0.23.0"
Expand Down
4 changes: 4 additions & 0 deletions apps/mark-scan/accessible-controller/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ path = "src/controllerd.rs"

[dependencies]
crc16 = { workspace = true }
ctrlc = { workspace = true }
num_enum = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
serialport = { workspace = true }
thiserror = { workspace = true }
vx-logging = { workspace = true }
uinput = { workspace = true }
9 changes: 9 additions & 0 deletions apps/mark-scan/accessible-controller/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# a phony dependency that can be used as a dependency to force builds
FORCE:

build: FORCE
mkdir -p target && cargo build --release --target-dir target/.

run:
./target/release/controllerd

6 changes: 3 additions & 3 deletions apps/mark-scan/accessible-controller/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,14 @@ To build:

```
cd apps/mark-scan/accessible-controller
cargo build
make build
```

To run:

```
// From vxsuite root
sudo ./target/debug/controllerd
// From apps/mark-scan/accessible-controller
sudo ./target/release/controllerd
```

To test:
Expand Down
154 changes: 122 additions & 32 deletions apps/mark-scan/accessible-controller/src/controllerd.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
use serialport::{self, SerialPort};
use std::{
io, thread,
io,
process::exit,
sync::{
atomic::{AtomicBool, Ordering},
Arc,
},
thread,
time::{Duration, Instant},
};
use uinput::{
event::{keyboard, Keyboard},
Device,
};
use vx_logging::{log, set_app_name, Disposition, EventId, EventType};

const POLL_INTERVAL: Duration = Duration::from_millis(10);
const MAX_ECHO_RESPONSE_WAIT: Duration = Duration::from_secs(1);
const APP_NAME: &str = "vx-mark-scan-controller-daemon";
const POLL_INTERVAL: Duration = Duration::from_millis(50);
const MAX_ECHO_RESPONSE_WAIT: Duration = Duration::from_secs(5);
const UINPUT_PATH: &str = "/dev/uinput";
const DEVICE_PATH: &str = "/dev/ttyACM1";
const DEVICE_BAUD_RATE: u32 = 9600;
Expand Down Expand Up @@ -200,11 +208,11 @@ enum Action {

fn send_key(device: &mut Device, key: keyboard::Key) -> Result<(), CommandError> {
device.click(&key)?;
device.synchronize().unwrap();
device.synchronize()?;
Ok(())
}

fn handle_command(device: &mut Device, data: &[u8]) -> Result<(), CommandError> {
fn handle_command(data: &[u8]) -> Result<Option<keyboard::Key>, CommandError> {
let ButtonStatusCommand { button, action } = data.try_into()?;

let key: keyboard::Key;
Expand Down Expand Up @@ -244,18 +252,17 @@ fn handle_command(device: &mut Device, data: &[u8]) -> Result<(), CommandError>
},
Action::Released => {
// Button release is a no-op since we already sent the keypress event
return Ok(());
return Ok(None);
}
}

send_key(device, key)
Ok(Some(key))
}

fn validate_connection(port: &mut Box<dyn SerialPort>) -> Result<(), io::Error> {
let echo_command = EchoCommand::new(vec![0x01, 0x02, 0x03, 0x04, 0x05]);
let echo_command: Vec<u8> = echo_command.into();
match port.write(&echo_command) {
Ok(_) => println!("Echo command sent"),
Ok(_) => log!(EventId::ControllerHandshakeInit; EventType::SystemAction),
Err(error) => eprintln!("{error:?}"),
}

Expand All @@ -271,11 +278,22 @@ fn validate_connection(port: &mut Box<dyn SerialPort>) -> Result<(), io::Error>
"Received different response from echo command: {echo_response:x?}"
);

println!("Received valid echo command response");
log!(
event_id: EventId::ControllerHandshakeComplete,
event_type: EventType::SystemAction,
disposition: Disposition::Success
);
return Ok(());
}
Err(ref e) if e.kind() == io::ErrorKind::TimedOut => (),
Err(e) => eprintln!("Error reading echo response: {e:?}"),
Err(e) => {
log!(
event_id: EventId::ControllerHandshakeComplete,
message: format!("Error reading echo response: {e:?}"),
event_type: EventType::SystemAction,
disposition: Disposition::Failure
);
}
}

if start_time.elapsed() >= MAX_ECHO_RESPONSE_WAIT {
Expand All @@ -285,6 +303,12 @@ fn validate_connection(port: &mut Box<dyn SerialPort>) -> Result<(), io::Error>
thread::sleep(POLL_INTERVAL);
}

log!(
event_id: EventId::ControllerHandshakeComplete,
message: "No echo response received".to_string(),
event_type: EventType::SystemAction,
disposition: Disposition::Failure
);
Err(io::Error::new(
io::ErrorKind::TimedOut,
"No echo response received",
Expand All @@ -303,42 +327,109 @@ fn create_virtual_device() -> Device {
}

fn main() {
// Open the serial port
let port = serialport::new(DEVICE_PATH, DEVICE_BAUD_RATE)
.timeout(POLL_INTERVAL)
.open();

println!("Opened controller serial port at {DEVICE_PATH}");
set_app_name(APP_NAME);
log!(
EventId::ProcessStarted;
EventType::SystemAction
);

let running = Arc::new(AtomicBool::new(true));

if let Err(e) = ctrlc::set_handler({
let running = running.clone();
move || {
running.store(false, Ordering::SeqCst);
}
}) {
log!(
event_id: EventId::ErrorSettingSigintHandler,
message: e.to_string(),
event_type: EventType::SystemStatus
);
}

// Create virtual device for keypress events
log!(
EventId::CreateVirtualUinputDeviceInit;
EventType::SystemAction
);
let mut device = create_virtual_device();
println!("Created virtual device");

// Wait for virtual device to register
thread::sleep(Duration::from_secs(1));
log!(
event_id: EventId::CreateVirtualUinputDeviceComplete,
disposition: Disposition::Success,
event_type: EventType::SystemAction
);

log!(
EventId::ControllerConnectionInit;
EventType::SystemAction
);

// Open the serial port
let port = serialport::new(DEVICE_PATH, DEVICE_BAUD_RATE)
.timeout(POLL_INTERVAL)
.open();

match port {
Ok(mut port) => {
validate_connection(&mut port).unwrap();

println!("Receiving data on {DEVICE_PATH} at {DEVICE_BAUD_RATE} baud");
log!(
event_id: EventId::ControllerConnectionComplete,
message: format!("Receiving data on {DEVICE_PATH} at {DEVICE_BAUD_RATE} baud"),
event_type: EventType::SystemAction,
disposition: Disposition::Success
);

let mut serial_buf: Vec<u8> = vec![0; 1000];
loop {
if !running.load(Ordering::SeqCst) {
log!(
EventId::ProcessTerminated;
EventType::SystemAction
);
exit(0);
}
match port.read(serial_buf.as_mut_slice()) {
Ok(size) => {
if let Err(e) = handle_command(&mut device, &serial_buf[..size]) {
eprintln!("Unexpected error handling command: {e}");
Ok(size) => match handle_command(&serial_buf[..size]) {
Ok(key_option) => {
if key_option.is_some() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored handle_command to avoid passing the device, which is cumbersome to create in CI because it requires special permissions. This seemed easier than configuring udev rules for CI.

The downside is I introduced this nested conditioning to handle when the command should no-op ie. when the controller indicates one of its buttons was released. We only want to send keyboard events when the button is depressed.

To support this, handle_command returns Some(keyboard::Key) on controller button press, and None(keyboard::Key) on button release.

Open to feedback on how to handle these checks more cleanly/inline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be improved by being more specific about the types you're looking for in your match arms:

                        Ok(Some(key)) => {
                            if let Err(err) = send_key(&mut device, key) {
                                log!(EventId::UnknownError, "Error sending key: {err}");
                            }
                        }
                        Ok(None) => {}

In general, you don't want to use is_some() or is_ok() et al when you want the value being wrapped. Use match/if let/let/else destructuring

let key = key_option.unwrap();
if let Err(err) = send_key(&mut device, key) {
log!(EventId::UnknownError, "Error sending key: {err}");
}
}
}
}
Err(err) => log!(
event_id: EventId::UnknownError,
message: format!(
"Unexpected error when handling controller command: {err}"
),
event_type: EventType::SystemStatus
),
},
// Timeout error just means no event was sent in the current polling period
Err(ref e) if e.kind() == io::ErrorKind::TimedOut => (),
Err(e) => eprintln!("{e:?}"),
Err(e) => {
log!(
event_id: EventId::UnknownError,
message: format!("Unexpected error when opening serial port: {e}"),
event_type: EventType::SystemStatus
);
}
}
}
}
Err(e) => {
panic!(r#"Failed to open "{DEVICE_PATH}". Error: {e}"#);
log!(
event_id: EventId::ControllerConnectionComplete,
message: format!("Failed to open {DEVICE_PATH}. Error: {e}"),
event_type: EventType::SystemAction,
disposition: Disposition::Failure
);
exit(1);
}
}
}
Expand All @@ -351,9 +442,8 @@ mod tests {

#[test]
fn test_handle_command_packet_length_error() {
let mut device = create_virtual_device();
let bad_data = [0x01];
match handle_command(&mut device, &bad_data) {
match handle_command(&bad_data) {
Err(CommandError::UnexpectedPacketSize(size)) => assert_eq!(size, 1),
result => panic!("Unexpected result: {result:?}"),
}
Expand All @@ -362,9 +452,8 @@ mod tests {
#[test]
fn test_handle_command_data_length() {
let bad_data_length: u8 = 0x03;
let mut device = create_virtual_device();
let bad_data = [0x30, 0x00, bad_data_length, 0x00, 0x00, 0x00, 0x00];
match handle_command(&mut device, &bad_data) {
match handle_command(&bad_data) {
Err(CommandError::UnexpectedDataLength(length)) => {
assert_eq!(length, bad_data_length as u16)
}
Expand All @@ -374,7 +463,6 @@ mod tests {

#[test]
fn test_handle_command_success() {
let mut device = create_virtual_device();
// In prod we wait 1s for the device to register.
// We can afford to be riskier to speed up tests.
thread::sleep(DEVICE_WAIT_DURATION);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't needed, right?

Expand All @@ -388,6 +476,8 @@ mod tests {
0xc8,
0x37,
];
handle_command(&mut device, &data).unwrap();
let key_option = handle_command(&data).unwrap();
assert!(key_option.is_some());
assert_eq!(key_option.unwrap(), keyboard::Key::R);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend this:

Suggested change
let key_option = handle_command(&data).unwrap();
assert!(key_option.is_some());
assert_eq!(key_option.unwrap(), keyboard::Key::R);
assert_eq!(handle_command(&data).unwrap().unwrap(), keyboard::Key::R);

In tests, unwrapping Option and Result generally provides more useful error messages than assert!(key_option.is_some()) or assert!(result.is_ok()). For example:

called `Result::unwrap()` on an `Err` value: UnexpectedDataLength(258)

}
}
1 change: 0 additions & 1 deletion apps/mark-scan/backend/build-patinputd.sh

This file was deleted.

2 changes: 1 addition & 1 deletion apps/mark-scan/backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"build"
],
"scripts": {
"build": "tsc --build tsconfig.build.json && copyfiles -u 3 src/custom-paper-handler/fixtures/*.jpg build/custom-paper-handler/fixtures && ./build-patinputd.sh",
"build": "tsc --build tsconfig.build.json && copyfiles -u 3 src/custom-paper-handler/fixtures/*.jpg build/custom-paper-handler/fixtures",
"clean": "rm -rf build tsconfig.tsbuildinfo tsconfig.build.tsbuildinfo",
"format": "prettier '**/*.+(css|graphql|json|less|md|mdx|sass|scss|yaml|yml)' --write",
"lint": "pnpm type-check && eslint .",
Expand Down
Loading