spirc: Configurable volume control steps (#1498)

* spirc: Configurable volume control steps

Allow the volume control steps to be configured via the `--volume-steps`
command line parameter. The author personally found the default volume
steps of `1024` to be completely unusable, and is presently using `128`
as his configuration. Perhaps consider this as a more reasonable
default.

Additionally, reduce the delay in volume update from a wopping two
seconds to 500ms, again for usability.

Also clean up the seemingly unnecessary use of a pattern match on
whether or not `--initial-volume` was supplied.

* fixup! spirc: Configurable volume control steps

* fixup! spirc: Configurable volume control steps

* fixup! spirc: Configurable volume control steps

* fixup! spirc: Configurable volume control steps

* fixup! spirc: Configurable volume control steps

* fixup! spirc: Configurable volume control steps

---------

Co-authored-by: Scott S. McCoy <scott.s.mccoy@acm.org>

CHANGELOG.md

@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- [connect] Add command line parameter for setting volume steps.
 - [connect] Add support for `seek_to`, `repeat_track` and `autoplay` for `Spirc` loading
 - [connect] Add `pause` parameter to `Spirc::disconnect` method (breaking)
 - [connect] Add `volume_steps` to `ConnectConfig` (breaking)

connect/src/spirc.rs

@@ -133,7 +133,7 @@ enum SpircCommand {
 const CONTEXT_FETCH_THRESHOLD: usize = 2;
 
 // delay to update volume after a certain amount of time, instead on each update request
-const VOLUME_UPDATE_DELAY: Duration = Duration::from_secs(2);
+const VOLUME_UPDATE_DELAY: Duration = Duration::from_millis(500);
 // to reduce updates to remote, we group some request by waiting for a set amount of time
 const UPDATE_STATE_DELAY: Duration = Duration::from_millis(200);
 
@@ -1514,16 +1514,16 @@ impl SpircTask {
     }
 
     fn handle_volume_up(&mut self) {
-        let volume_steps = self.connect_state.device_info().capabilities.volume_steps as u16;
+        let volume = (self.connect_state.device_info().volume as u16)
+            .saturating_add(self.connect_state.volume_step_size);
 
-        let volume = (self.connect_state.device_info().volume as u16).saturating_add(volume_steps);
         self.set_volume(volume);
     }
 
     fn handle_volume_down(&mut self) {
-        let volume_steps = self.connect_state.device_info().capabilities.volume_steps as u16;
+        let volume = (self.connect_state.device_info().volume as u16)
+            .saturating_sub(self.connect_state.volume_step_size);
 
-        let volume = (self.connect_state.device_info().volume as u16).saturating_sub(volume_steps);
         self.set_volume(volume);
     }
 
@@ -1639,6 +1639,8 @@ impl SpircTask {
     }
 
     fn set_volume(&mut self, volume: u16) {
+        debug!("SpircTask::set_volume({})", volume);
+
         let old_volume = self.connect_state.device_info().volume;
         let new_volume = volume as u32;
         if old_volume != new_volume || self.mixer.volume() != volume {

connect/src/state.rs

@@ -87,7 +87,7 @@ pub struct ConnectConfig {
     pub initial_volume: u16,
     /// Disables the option to control the volume remotely (default: false)
     pub disable_volume: bool,
-    /// The steps in which the volume is incremented (default: 1024)
+    /// Number of incremental steps (default: 64)
     pub volume_steps: u16,
 }
 
@@ -99,7 +99,7 @@ impl Default for ConnectConfig {
             is_group: false,
             initial_volume: u16::MAX / 2,
             disable_volume: false,
-            volume_steps: 1024,
+            volume_steps: 64,
         }
     }
 }
@@ -127,10 +127,15 @@ pub(super) struct ConnectState {
 
     /// a context to keep track of the autoplay context
     autoplay_context: Option<StateContext>,
+
+    /// The volume adjustment per step when handling individual volume adjustments.
+    pub volume_step_size: u16,
 }
 
 impl ConnectState {
     pub fn new(cfg: ConnectConfig, session: &Session) -> Self {
+        let volume_step_size = u16::MAX.checked_div(cfg.volume_steps).unwrap_or(1024);
+
         let device_info = DeviceInfo {
             can_play: true,
             volume: cfg.initial_volume.into(),
@@ -195,6 +200,7 @@ impl ConnectState {
                 }),
                 ..Default::default()
             },
+            volume_step_size,
             ..Default::default()
         };
         state.reset();

src/main.rs

@@ -276,6 +276,7 @@ fn get_setup() -> Setup {
     const VERSION: &str = "version";
     const VOLUME_CTRL: &str = "volume-ctrl";
     const VOLUME_RANGE: &str = "volume-range";
+    const VOLUME_STEPS: &str = "volume-steps";
     const ZEROCONF_PORT: &str = "zeroconf-port";
     const ZEROCONF_INTERFACE: &str = "zeroconf-interface";
     const ZEROCONF_BACKEND: &str = "zeroconf-backend";
@@ -291,6 +292,7 @@ fn get_setup() -> Setup {
     const DEVICE_SHORT: &str = "d";
     const VOLUME_CTRL_SHORT: &str = "E";
     const VOLUME_RANGE_SHORT: &str = "e";
+    const VOLUME_STEPS_SHORT: &str = ""; // no short flag
     const DEVICE_TYPE_SHORT: &str = "F";
     const FORMAT_SHORT: &str = "f";
     const DISABLE_AUDIO_CACHE_SHORT: &str = "G";
@@ -371,6 +373,8 @@ fn get_setup() -> Setup {
     #[cfg(not(feature = "alsa-backend"))]
     const VOLUME_RANGE_DESC: &str =
         "Range of the volume control (dB) from 0.0 to 100.0. Defaults to 60.0.";
+    const VOLUME_STEPS_DESC: &str =
+        "Number of incremental steps when responding to volume control updates. Defaults to 64.";
 
     let mut opts = getopts::Options::new();
     opts.optflag(
@@ -570,6 +574,12 @@ fn get_setup() -> Setup {
         VOLUME_RANGE_DESC,
         "RANGE",
     )
+    .optopt(
+        VOLUME_STEPS_SHORT,
+        VOLUME_STEPS,
+        VOLUME_STEPS_DESC,
+        "STEPS",
+    )
     .optopt(
         NORMALISATION_METHOD_SHORT,
         NORMALISATION_METHOD,
@@ -1457,7 +1467,8 @@ fn get_setup() -> Setup {
                 } else {
                     cache.as_ref().and_then(Cache::volume)
                 }
-            });
+            })
+            .unwrap_or_default();
 
         let device_type = opt_str(DEVICE_TYPE)
             .as_deref()
@@ -1480,23 +1491,34 @@ fn get_setup() -> Setup {
             })
             .unwrap_or_default();
 
+        let volume_steps = opt_str(VOLUME_STEPS)
+            .map(|steps| match steps.parse::<u16>() {
+                Ok(value) => value,
+                _ => {
+                    let default_value = &connect_default_config.volume_steps.to_string();
+
+                    invalid_error_msg(
+                        VOLUME_STEPS,
+                        VOLUME_STEPS_SHORT,
+                        &steps,
+                        "a positive whole number <= 65535",
+                        default_value,
+                    );
+
+                    exit(1);
+                }
+            })
+            .unwrap_or_else(|| connect_default_config.volume_steps);
+
         let is_group = opt_present(DEVICE_IS_GROUP);
 
-        if let Some(initial_volume) = initial_volume {
+        ConnectConfig {
-            ConnectConfig {
+            name,
-                name,
+            device_type,
-                device_type,
+            is_group,
-                is_group,
+            initial_volume,
-                initial_volume,
+            volume_steps,
-                ..Default::default()
+            ..connect_default_config
-            }
-        } else {
-            ConnectConfig {
-                name,
-                device_type,
-                is_group,
-                ..Default::default()
-            }
         }
     };