chore: refactor camera trait, optimize setter

Signed-off-by: Nikolaos Karaolidis <nick@karaolidis.com>

src/camera/devices/mod.rs

@@ -1,4 +1,3 @@
-use anyhow::bail;
 use rusb::GlobalContext;
 
 use super::CameraImpl;
@@ -13,26 +12,6 @@ pub struct SupportedCamera<P: rusb::UsbContext> {
     pub impl_factory: ImplFactory<P>,
 }
 
-impl<P: rusb::UsbContext> SupportedCamera<P> {
-    pub fn new_camera(&self, device: &rusb::Device<P>) -> anyhow::Result<Box<dyn CameraImpl<P>>> {
-        let descriptor = device.device_descriptor()?;
-
-        let matches =
-            descriptor.vendor_id() == self.vendor && descriptor.product_id() == self.product;
-
-        if !matches {
-            bail!(
-                "Device with vendor {:04x} and product {:04x} does not match {}",
-                descriptor.vendor_id(),
-                descriptor.product_id(),
-                self.name
-            );
-        }
-
-        Ok((self.impl_factory)())
-    }
-}
-
 macro_rules! default_camera_impl {
     (
         $const_name:ident,

src/camera/mod.rs

@@ -21,6 +21,8 @@ use ptp::{
 use ptp_cursor::{PtpDeserialize, PtpSerialize};
 use rusb::{GlobalContext, constants::LIBUSB_CLASS_IMAGE};
 
+use crate::usb::find_endpoint;
+
 const SESSION: u32 = 1;
 
 pub struct Camera {
@@ -29,75 +31,6 @@ pub struct Camera {
 }
 
 impl Camera {
-    pub fn from_device(device: &rusb::Device<GlobalContext>) -> anyhow::Result<Self> {
-        for supported_camera in devices::SUPPORTED {
-            if let Ok(r#impl) = supported_camera.new_camera(device) {
-                let bus = device.bus_number();
-                let address = device.address();
-
-                let config_descriptor = device.active_config_descriptor()?;
-
-                let interface_descriptor = config_descriptor
-                    .interfaces()
-                    .flat_map(|i| i.descriptors())
-                    .find(|x| x.class_code() == LIBUSB_CLASS_IMAGE)
-                    .ok_or(rusb::Error::NotFound)?;
-
-                let interface = interface_descriptor.interface_number();
-                debug!("Found interface {interface}");
-
-                let handle = device.open()?;
-                handle.claim_interface(interface)?;
-
-                let bulk_in = Self::find_endpoint(
-                    &interface_descriptor,
-                    rusb::Direction::In,
-                    rusb::TransferType::Bulk,
-                )?;
-                let bulk_out = Self::find_endpoint(
-                    &interface_descriptor,
-                    rusb::Direction::Out,
-                    rusb::TransferType::Bulk,
-                )?;
-
-                let transaction_id = 0;
-
-                let chunk_size = r#impl.chunk_size();
-
-                let mut ptp = Ptp {
-                    bus,
-                    address,
-                    interface,
-                    bulk_in,
-                    bulk_out,
-                    handle,
-                    transaction_id,
-                    chunk_size,
-                };
-
-                debug!("Opening session");
-                let () = r#impl.open_session(&mut ptp, SESSION)?;
-                debug!("Session opened");
-
-                return Ok(Self { r#impl, ptp });
-            }
-        }
-
-        bail!("Device not supported");
-    }
-
-    fn find_endpoint(
-        interface_descriptor: &rusb::InterfaceDescriptor<'_>,
-        direction: rusb::Direction,
-        transfer_type: rusb::TransferType,
-    ) -> Result<u8, rusb::Error> {
-        interface_descriptor
-            .endpoint_descriptors()
-            .find(|ep| ep.direction() == direction && ep.transfer_type() == transfer_type)
-            .map(|x| x.address())
-            .ok_or(rusb::Error::NotFound)
-    }
-
     pub fn name(&self) -> &'static str {
         self.r#impl.supported_camera().name
     }
@@ -336,6 +269,76 @@ impl Drop for Camera {
     }
 }
 
+impl TryFrom<&rusb::Device<GlobalContext>> for Camera {
+    type Error = anyhow::Error;
+
+    fn try_from(device: &rusb::Device<GlobalContext>) -> anyhow::Result<Self> {
+        let descriptor = device.device_descriptor()?;
+
+        let vendor = descriptor.vendor_id();
+        let product = descriptor.product_id();
+
+        for supported_camera in devices::SUPPORTED {
+            if vendor != supported_camera.vendor || product != supported_camera.product {
+                continue;
+            }
+
+            let r#impl = (supported_camera.impl_factory)();
+
+            let bus = device.bus_number();
+            let address = device.address();
+
+            let config_descriptor = device.active_config_descriptor()?;
+
+            let interface_descriptor = config_descriptor
+                .interfaces()
+                .flat_map(|i| i.descriptors())
+                .find(|x| x.class_code() == LIBUSB_CLASS_IMAGE)
+                .ok_or(rusb::Error::NotFound)?;
+
+            let interface = interface_descriptor.interface_number();
+            debug!("Found interface {interface}");
+
+            let handle = device.open()?;
+            handle.claim_interface(interface)?;
+
+            let bulk_in = find_endpoint(
+                &interface_descriptor,
+                rusb::Direction::In,
+                rusb::TransferType::Bulk,
+            )?;
+            let bulk_out = find_endpoint(
+                &interface_descriptor,
+                rusb::Direction::Out,
+                rusb::TransferType::Bulk,
+            )?;
+
+            let transaction_id = 0;
+
+            let chunk_size = r#impl.chunk_size();
+
+            let mut ptp = Ptp {
+                bus,
+                address,
+                interface,
+                bulk_in,
+                bulk_out,
+                handle,
+                transaction_id,
+                chunk_size,
+            };
+
+            debug!("Opening session");
+            let () = r#impl.open_session(&mut ptp, SESSION)?;
+            debug!("Session opened");
+
+            return Ok(Self { r#impl, ptp });
+        }
+
+        bail!("Device not supported");
+    }
+}
+
 pub trait CameraImpl<P: rusb::UsbContext> {
     fn supported_camera(&self) -> &'static SupportedCamera<P>;
 

src/camera/ptp/hex.rs

@@ -279,7 +279,8 @@ impl DerefMut for FujiCustomSettingName {
 
 impl TryFrom<String> for FujiCustomSettingName {
     type Error = anyhow::Error;
-    fn try_from(value: String) -> Result<Self, anyhow::Error> {
+
+    fn try_from(value: String) -> anyhow::Result<Self> {
         if value.len() > Self::MAX_LEN {
             bail!("Value '{}' exceeds max length of {}", value, Self::MAX_LEN);
         }
@@ -289,7 +290,8 @@ impl TryFrom<String> for FujiCustomSettingName {
 
 impl FromStr for FujiCustomSettingName {
     type Err = anyhow::Error;
-    fn from_str(s: &str) -> Result<Self, anyhow::Error> {
+
+    fn from_str(s: &str) -> anyhow::Result<Self> {
         if s.len() > Self::MAX_LEN {
             bail!("Value '{}' exceeds max length of {}", s, Self::MAX_LEN);
         }

src/cli/common/file.rs

@@ -8,6 +8,7 @@ pub enum Input {
 
 impl FromStr for Input {
     type Err = anyhow::Error;
+
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         if s == "-" {
             Ok(Self::Stdin)
@@ -34,6 +35,7 @@ pub enum Output {
 
 impl FromStr for Output {
     type Err = anyhow::Error;
+
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         if s == "-" {
             Ok(Self::Stdout)

src/cli/simulation/mod.rs

@@ -97,7 +97,6 @@ fn handle_list(json: bool, device_id: Option<&str>) -> anyhow::Result<()> {
 #[derive(Serialize)]
 #[serde(rename_all = "camelCase")]
 pub struct FilmSimulationRepr {
-    pub slot: FujiCustomSetting,
     pub name: FujiCustomSettingName,
     pub simulation: FujiFilmSimulation,
     pub size: FujiImageSize,
@@ -121,7 +120,6 @@ pub struct FilmSimulationRepr {
 
 impl fmt::Display for FilmSimulationRepr {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        writeln!(f, "Slot: {}", self.slot)?;
         writeln!(f, "Name: {}", self.name)?;
         writeln!(f, "Simulation: {}", self.simulation)?;
         writeln!(f, "Size: {}", self.size)?;
@@ -156,7 +154,6 @@ fn handle_get(json: bool, device_id: Option<&str>, slot: FujiCustomSetting) -> a
     camera.set_active_custom_setting(slot)?;
 
     let repr = FilmSimulationRepr {
-        slot,
         name: camera.get_custom_setting_name()?,
         simulation: camera.get_film_simulation()?,
         size: camera.get_image_size()?,
@@ -242,15 +239,17 @@ fn handle_set(
     }
 
     // White Balance
-    let white_balance = match &options.white_balance {
+    if let Some(white_balance) = &options.white_balance {
-        Some(white_balance) => {
+        camera.set_white_balance(*white_balance)?;
-            camera.set_white_balance(*white_balance)?;
+    }
-            white_balance
-        }
-        None => &camera.get_white_balance()?,
-    };
 
     if let Some(temperature) = &options.white_balance_temperature {
+        let white_balance = if let Some(white_balance) = &options.white_balance {
+            white_balance
+        } else {
+            &camera.get_white_balance()?
+        };
+
         if *white_balance != FujiWhiteBalance::Temperature {
             warn!("White Balance mode is not set to 'Temperature', refusing to set temperature")
         } else {
@@ -267,35 +266,40 @@ fn handle_set(
     }
 
     // Exposure
-    let dynamic_range_priority = match &options.dynamic_range_priority {
+    if let Some(dynamic_range_priority) = &options.dynamic_range_priority {
-        Some(dynamic_range_priority) => {
+        camera.set_dynamic_range_priority(*dynamic_range_priority)?;
-            camera.set_dynamic_range_priority(*dynamic_range_priority)?;
-            dynamic_range_priority
-        }
-        None => &camera.get_dynamic_range_priority()?,
-    };
-
-    if let Some(dynamic_range) = &options.dynamic_range {
-        if *dynamic_range_priority != FujiDynamicRangePriority::Off {
-            warn!("Dynamic Range Priority is enabled, refusing to set dynamic range")
-        } else {
-            camera.set_dynamic_range(*dynamic_range)?;
-        }
     }
 
-    if let Some(highlights) = &options.highlight {
+    if options.dynamic_range.is_some() || options.highlight.is_some() || options.shadow.is_some() {
-        if *dynamic_range_priority != FujiDynamicRangePriority::Off {
+        let dynamic_range_priority =
-            warn!("Dynamic Range Priority is enabled, refusing to set highlight tone")
+            if let Some(dynamic_range_priority) = &options.dynamic_range_priority {
-        } else {
+                dynamic_range_priority
-            camera.set_highlight_tone(*highlights)?;
+            } else {
-        }
+                &camera.get_dynamic_range_priority()?
-    }
+            };
 
-    if let Some(shadows) = &options.shadow {
+        if let Some(dynamic_range) = &options.dynamic_range {
-        if *dynamic_range_priority != FujiDynamicRangePriority::Off {
+            if *dynamic_range_priority != FujiDynamicRangePriority::Off {
-            warn!("Dynamic Range Priority is enabled, refusing to set shadow tone")
+                warn!("Dynamic Range Priority is enabled, refusing to set dynamic range")
-        } else {
+            } else {
-            camera.set_shadow_tone(*shadows)?;
+                camera.set_dynamic_range(*dynamic_range)?;
+            }
+        }
+
+        if let Some(highlights) = &options.highlight {
+            if *dynamic_range_priority != FujiDynamicRangePriority::Off {
+                warn!("Dynamic Range Priority is enabled, refusing to set highlight tone")
+            } else {
+                camera.set_highlight_tone(*highlights)?;
+            }
+        }
+
+        if let Some(shadows) = &options.shadow {
+            if *dynamic_range_priority != FujiDynamicRangePriority::Off {
+                warn!("Dynamic Range Priority is enabled, refusing to set shadow tone")
+            } else {
+                camera.set_shadow_tone(*shadows)?;
+            }
         }
     }
 

src/usb/mod.rs

@@ -2,11 +2,23 @@ use anyhow::{anyhow, bail};
 
 use crate::camera::Camera;
 
+pub fn find_endpoint(
+    interface_descriptor: &rusb::InterfaceDescriptor<'_>,
+    direction: rusb::Direction,
+    transfer_type: rusb::TransferType,
+) -> Result<u8, rusb::Error> {
+    interface_descriptor
+        .endpoint_descriptors()
+        .find(|ep| ep.direction() == direction && ep.transfer_type() == transfer_type)
+        .map(|x| x.address())
+        .ok_or(rusb::Error::NotFound)
+}
+
 pub fn get_connected_cameras() -> anyhow::Result<Vec<Camera>> {
     let mut connected_cameras = Vec::new();
 
     for device in rusb::devices()?.iter() {
-        if let Ok(camera) = Camera::from_device(&device) {
+        if let Ok(camera) = Camera::try_from(&device) {
             connected_cameras.push(camera);
         }
     }
@@ -25,7 +37,7 @@ pub fn get_connected_camera_by_id(id: &str) -> anyhow::Result<Camera> {
 
     for device in rusb::devices()?.iter() {
         if device.bus_number() == bus && device.address() == address {
-            return Camera::from_device(&device);
+            return Camera::try_from(&device);
         }
     }