Add more logging to mark-scan

[kshen0]

Overview

#3983

• Adds logging to mark-scan frontend, backend, and daemons
• Migrates PAT input daemon from C to Rust to converge on Rust logging library

Demo Video or Screenshot

N/A

Testing Plan

• Add automated tests

Checklist

• I have added logging where appropriate to any new user actions, system updates such as file reads or storage writes, or errors introduced.

[kshen0]

Most behavior requiring logs is common to VxMark or covered in the backend

[CLAassistant]

All committers have signed the CLA.

[eventualbuddha]

Needs CircleCI config updates.

[eventualbuddha]

Feels like we need a useLogger hook.

[kshen0]

In line with discussion in #4592 (comment), 579f510 moves logging to the backend

[kshen0]

Needs CircleCI config updates.

Do you mind if I do this in a follow up PR? I didn't realize circleci.yml is autogenerated. From a quick look, the generation script only handles pnpm packages atm and needs to be extended to handle Rust crates.

Rust tests pass here but validate monorepo failed.

[kshen0]

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.

[eventualbuddha]

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

[eventualbuddha]

This isn't needed, right?

[eventualbuddha]

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)

[eventualbuddha]

Rebase weirdness?

[kshen0]

Somewhere along the way this code path was reported as uncovered so I added this test.

[eventualbuddha]

A couple minor things, but LGTM!

[eventualbuddha]

When you're not using the value it seems a little weird to use unwrap_or_else. Maybe do this?

Suggested change

	set_up_pins().unwrap_or_else(|error| {
	log!(
	event_id: EventId::ConnectToPatInputComplete,
	disposition: Disposition::Failure,
	message: format!("An error occurred during GPIO pin connection: {error}")
	);
	});
	if let Err(error) = set_up_pins() {
	log!(
	event_id: EventId::ConnectToPatInputComplete,
	disposition: Disposition::Failure,
	message: format!("An error occurred during GPIO pin connection: {error}")
	);
	}

Also, do we want to continue after this?

[kshen0]

5171668
good catch, thank you

[eventualbuddha]

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.

[kshen0]

a75cf84

doc comments added to both daemons and Pin struct

[kshen0]

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.

[kshen0]

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.