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

feat(adb): add client flavors and autolaunch #2515

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions alvr/adb/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ license.workspace = true
alvr_common.workspace = true
alvr_filesystem.workspace = true
alvr_server_io.workspace = true
alvr_session.workspace = true

anyhow = "1"
ureq = "2.10"
Expand Down
42 changes: 33 additions & 9 deletions alvr/adb/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ mod parse;

use alvr_common::anyhow::Result;
use alvr_common::{dbg_connection, error};
use alvr_session::{ClientFlavor, Settings};
use std::collections::HashSet;

pub enum WiredConnectionStatus {
Expand All @@ -24,7 +25,7 @@ impl WiredConnection {
Ok(Self { adb_path })
}

pub fn setup(&self, control_port: u16, stream_port: u16) -> Result<WiredConnectionStatus> {
pub fn setup(&self, control_port: u16, settings: &Settings) -> Result<WiredConnectionStatus> {
Copy link
Member

@zmerp zmerp Nov 17, 2024

Choose a reason for hiding this comment

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

I would pass the minimum subset of settings as parameter. This would be ConnectionConfig

let Some(device_serial) = commands::list_devices(&self.adb_path)?
.into_iter()
.filter_map(|d| d.serial)
Expand All @@ -35,7 +36,7 @@ impl WiredConnection {
));
};

let ports = HashSet::from([control_port, stream_port]);
let ports = HashSet::from([control_port, settings.connection.stream_port]);
let forwarded_ports: HashSet<u16> =
commands::list_forwarded_ports(&self.adb_path, &device_serial)?
.into_iter()
Expand All @@ -49,16 +50,39 @@ impl WiredConnection {
);
}

#[cfg(debug_assertions)]
let process_name = "alvr.client.dev";
#[cfg(not(debug_assertions))]
let process_name = "alvr.client.stable";
let process_name = match &settings.connection.client_flavor {
ClientFlavor::Store => if alvr_common::is_stable() {
"alvr.client"
} else {
"alvr.client.dev"
}
.to_owned(),
ClientFlavor::Github => if alvr_common::is_stable() {
"alvr.client.stable"
} else {
"alvr.client.dev"
}
.to_owned(),
ClientFlavor::Custom(name) => name.clone(),
};
Comment on lines +53 to +67
Copy link
Member

Choose a reason for hiding this comment

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

This is missing the fallbacks.
Store: store -> gh
GitHub: gh -> store
Custom: custom -> store -> gh

Skip store if not stable.

You can implement this by storing a slice to process_names. At this point it would be better to extract the package names to string constants.


if commands::get_process_id(&self.adb_path, &device_serial, process_name)?.is_none() {
if !commands::is_package_installed(&self.adb_path, &device_serial, &process_name)? {
Ok(WiredConnectionStatus::NotReady(
"ALVR client is not running".to_owned(),
"ALVR client is not installed".to_owned(),
))
} else if !commands::is_activity_resumed(&self.adb_path, &device_serial, process_name)? {
} else if commands::get_process_id(&self.adb_path, &device_serial, &process_name)?.is_none()
{
if settings.connection.client_autolaunch {
commands::start_application(&self.adb_path, &device_serial, &process_name)?;
Ok(WiredConnectionStatus::NotReady(
"Starting ALVR client".to_owned(),
))
} else {
Ok(WiredConnectionStatus::NotReady(
"ALVR client is not running".to_owned(),
))
}
} else if !commands::is_activity_resumed(&self.adb_path, &device_serial, &process_name)? {
Ok(WiredConnectionStatus::NotReady(
"ALVR client is paused".to_owned(),
))
Expand Down
20 changes: 9 additions & 11 deletions alvr/server_core/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,17 +288,15 @@ pub fn handshake_loop(ctx: Arc<ConnectionContext>, lifecycle_state: Arc<RwLock<L
wired_connection.as_ref().unwrap()
};

let status = match wired_connection.setup(
CONTROL_PORT,
SESSION_MANAGER.read().settings().connection.stream_port,
) {
Ok(status) => status,
Err(e) => {
error!("{e:?}");
thread::sleep(RETRY_CONNECT_MIN_INTERVAL);
continue;
}
};
let status =
match wired_connection.setup(CONTROL_PORT, SESSION_MANAGER.read().settings()) {
Ok(status) => status,
Err(e) => {
error!("{e:?}");
thread::sleep(RETRY_CONNECT_MIN_INTERVAL);
continue;
}
};

if let WiredConnectionStatus::NotReady(m) = status {
dbg_connection!("handshake_loop: Wired connection not ready: {m}");
Expand Down
26 changes: 26 additions & 0 deletions alvr/session/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1104,6 +1104,13 @@ pub enum SocketBufferSize {
Custom(#[schema(suffix = "B")] u32),
}

#[derive(SettingsSchema, Serialize, Deserialize, Clone)]
pub enum ClientFlavor {
Store,
Github,
Custom(String),
}

#[derive(SettingsSchema, Serialize, Deserialize, Clone)]
pub struct ConnectionConfig {
#[schema(strings(
Expand All @@ -1114,6 +1121,16 @@ TCP: Slower than UDP, but more stable. Pick this if you experience video or audi

pub client_discovery: Switch<DiscoveryConfig>,

#[schema(strings(
help = r#"Wether ALVR should try to automatically launch the client when establishing a wired connection."#
))]
pub client_autolaunch: bool,

#[schema(strings(
help = r#"Which type of client should ALVR look for when establishing a wired connection."#
))]
pub client_flavor: ClientFlavor,
Comment on lines +1124 to +1132
Copy link
Member

Choose a reason for hiding this comment

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

I would rename these to wired_client_autolaunch and wired_client_version_check. I would also switch the order of these two settings. The reason is wired_client_autolaunch requires wired_client_version_check

Copy link
Collaborator

Choose a reason for hiding this comment

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

version_check makes no sense tho, this isn't about a the v version, which is probably going to confuse people

Copy link
Member

Choose a reason for hiding this comment

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

Right. it's that "flavor" may sound a bit funny. I don't have better ideas so let's use wired_client_flavor_check

Copy link
Collaborator

@The-personified-devil The-personified-devil Nov 17, 2024

Choose a reason for hiding this comment

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

wired_client_type and maybe adjust the help string to "Which type of release of the client ..."


#[schema(strings(
help = "This script will be ran when the headset connects. Env var ACTION will be set to `connect`."
))]
Expand Down Expand Up @@ -1752,6 +1769,15 @@ pub fn session_settings_default() -> SettingsDefault {
auto_trust_clients: cfg!(debug_assertions),
},
},
client_autolaunch: true,
client_flavor: ClientFlavorDefault {
Custom: "alvr.client".to_owned(),
variant: if alvr_common::is_stable() {
ClientFlavorDefaultVariant::Store
} else {
ClientFlavorDefaultVariant::Github
},
},
web_server_port: 8082,
stream_port: 9944,
osc_local_port: 9942,
Expand Down
Loading