Gracefully handle out-of-range error codes

Fixes #22.

Author: RReverser

src/api/camera/image_array/client.rs

@@ -192,9 +192,9 @@ impl Response for ASCOMResult<ImageArray> {
             );
             Ok(ndarray::Array::from_shape_vec(shape, data)?.into())
         } else {
-            Err(ASCOMError::new(
-                ASCOMErrorCode::try_from(u16::try_from(metadata.error_number)?)?,
-                std::str::from_utf8(raw_data)?.to_owned(),
+            Err(ASCOMError::new_with_unbounded_code(
+                metadata.error_number,
+                &String::from_utf8_lossy(raw_data),
             ))
         };
         Ok(ResponseWithTransaction {
@@ -207,6 +207,7 @@ impl Response for ASCOMResult<ImageArray> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::assert_matches;
 
     /// Build an `application/imagebytes` payload at the given byte
     /// offset within a `Vec<u8>`. The `Vec`'s base allocation is
@@ -285,4 +286,28 @@ mod tests {
                 .unwrap_or_else(|e| panic!("leading_pad={leading_pad}: {e:?}"));
         }
     }
+
+    /// Regression test for issue #22: an `imagebytes` body carrying an HRESULT-style
+    /// `error_number` outside Alpaca's valid range must parse into an `UNSPECIFIED`
+    /// error (preserving the message) rather than failing the whole response.
+    #[test]
+    fn from_reqwest_maps_out_of_range_error_number_to_unspecified() -> eyre::Result<()> {
+        let metadata = ImageBytesMetadata {
+            metadata_version: 1_i32,
+            error_number: -2_147_024_882_i32,
+            data_start: i32::try_from(size_of::<ImageBytesMetadata>())?,
+            ..bytemuck::Zeroable::zeroed()
+        };
+        let mut buf = bytemuck::bytes_of(&metadata).to_vec();
+        buf.extend_from_slice(b"Not enough storage");
+
+        let mime: Mime = IMAGE_BYTES_TYPE.parse()?;
+        let parsed = <ASCOMResult<ImageArray> as Response>::from_reqwest(mime, &buf)?;
+        assert_matches!(parsed.response, Err(ASCOMError {
+            code: ASCOMErrorCode::UNSPECIFIED,
+            message,
+        }) if message.contains("Not enough storage"));
+
+        Ok(())
+    }
 }

src/errors.rs

@@ -1,6 +1,6 @@
 use serde::{Deserialize, Serialize};
 use std::borrow::Cow;
-use std::fmt::{self, Debug, Display, Formatter};
+use std::fmt::{self, Debug, Display, Formatter, UpperHex};
 use std::ops::RangeInclusive;
 use thiserror::Error;
 
@@ -11,24 +11,44 @@ const DRIVER_BASE: u16 = 0x500;
 /// The maximum value for error numbers.
 const MAX: u16 = 0xFFF;
 
+/// The valid range of error numbers.
+const RANGE: RangeInclusive<u16> = BASE..=MAX;
+
+fn invalid_error_code(raw: impl UpperHex) -> eyre::Error {
+    eyre::eyre!("Error code {raw:#X} is out of valid range ({RANGE:#X?})")
+}
+
 /// Alpaca representation of an ASCOM error code.
 #[derive(Clone, Copy, PartialEq, Eq, Serialize, Deserialize, derive_more::Display)]
 #[display("{self:?}")]
 #[serde(transparent)]
 pub struct ASCOMErrorCode(u16);
 
+impl TryFrom<i32> for ASCOMErrorCode {
+    type Error = eyre::Error;
+
+    /// Convert a raw error code into an `ASCOMErrorCode` if it's in the valid range.
+    fn try_from(raw: i32) -> eyre::Result<Self> {
+        if let Ok(raw) = u16::try_from(raw)
+            && RANGE.contains(&raw)
+        {
+            Ok(Self(raw))
+        } else {
+            Err(invalid_error_code(raw))
+        }
+    }
+}
+
 impl TryFrom<u16> for ASCOMErrorCode {
     type Error = eyre::Error;
 
     /// Convert a raw error code into an `ASCOMErrorCode` if it's in the valid range.
     fn try_from(raw: u16) -> eyre::Result<Self> {
-        const RANGE: RangeInclusive<u16> = BASE..=MAX;
-
-        eyre::ensure!(
-            RANGE.contains(&raw),
-            "Error code {raw:#X} is out of valid range ({RANGE:#X?})"
-        );
-        Ok(Self(raw))
+        if RANGE.contains(&raw) {
+            Ok(Self(raw))
+        } else {
+            Err(invalid_error_code(raw))
+        }
     }
 }
 
@@ -95,7 +115,7 @@ impl ASCOMErrorCode {
 }
 
 /// ASCOM error.
-#[derive(Debug, Clone, Serialize, Deserialize, Error)]
+#[derive(Debug, Clone, Serialize, Error)]
 #[error("ASCOM error {code}: {message}")]
 pub struct ASCOMError {
     /// Error number.
@@ -106,6 +126,33 @@ pub struct ASCOMError {
     pub message: Cow<'static, str>,
 }
 
+impl<'de> Deserialize<'de> for ASCOMError {
+    fn deserialize<D: serde::Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
+        #[derive(Deserialize)]
+        #[serde(rename_all = "PascalCase")]
+        struct Repr {
+            error_number: i32,
+            error_message: Cow<'static, str>,
+        }
+
+        let Repr {
+            error_number,
+            error_message,
+        } = Repr::deserialize(deserializer)?;
+
+        // A zero error number is success; anything else goes through the shared out-of-range
+        // handling so the JSON and `imagebytes` paths agree (issue #22).
+        Ok(if error_number == 0 {
+            Self {
+                code: ASCOMErrorCode::OK,
+                message: error_message,
+            }
+        } else {
+            Self::new_with_unbounded_code(error_number, &error_message)
+        })
+    }
+}
+
 impl ASCOMError {
     /// Create a new `ASCOMError` from given error code and a message.
     pub fn new(code: ASCOMErrorCode, message: impl Display) -> Self {
@@ -114,6 +161,23 @@ impl ASCOMError {
             message: format!("{message:#}").into(),
         }
     }
+
+    /// Create an `ASCOMError` from a raw, non-zero error number that may fall outside Alpaca's
+    /// valid range.
+    ///
+    /// Drivers occasionally report HRESULT-style codes (e.g. `-2147024882`) that don't fit.
+    ///
+    /// Instead of failing hard, which would lose the original error message (see issue #22),
+    /// we surface those as `UNSPECIFIED` with the range error prepended to the main one.
+    pub(crate) fn new_with_unbounded_code(raw_code: i32, message: &str) -> Self {
+        match ASCOMErrorCode::try_from(raw_code) {
+            Ok(code) => Self::new(code, message),
+            Err(err) => Self::new(
+                ASCOMErrorCode::UNSPECIFIED,
+                format_args!("{err}: {message}"),
+            ),
+        }
+    }
 }
 
 /// Result type for ASCOM methods.
@@ -186,7 +250,7 @@ ascom_error_codes! {
     // Extra codes for internal use only.
 
     /// Reserved 'catch-all' error code (0x4FF) used when nothing else was specified.
-    UNSPECIFIED = 0x4FF,
+    pub(crate) UNSPECIFIED = 0x4FF,
 }
 
 impl ASCOMError {
@@ -206,3 +270,47 @@ impl ASCOMError {
         Self::new(ASCOMErrorCode::UNSPECIFIED, message)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{ASCOMError, ASCOMErrorCode};
+    use std::assert_matches;
+
+    // Devices sometimes return HRESULT-style error numbers that don't fit `u16`. These must
+    // deserialize as `UNSPECIFIED` (keeping the message) rather than failing the response (#22).
+    #[test]
+    fn deserializes_out_of_range_error_number_as_unspecified() -> eyre::Result<()> {
+        assert_matches!(
+            serde_json::from_str(r#"{"ErrorNumber": -2147024882, "ErrorMessage": "Not enough storage"}"#)?,
+            ASCOMError {
+                code: ASCOMErrorCode::UNSPECIFIED,
+                message,
+            } if message.contains("out of valid range") && message.contains("Not enough storage")
+        );
+
+        Ok(())
+    }
+
+    // In-range codes (including success `0`, which is outside the `0x400..=0xFFF` range but is a
+    // valid `u16`) must round-trip unchanged so the client can still detect `OK` responses.
+    #[test]
+    fn deserializes_in_range_error_numbers_unchanged() -> eyre::Result<()> {
+        assert_matches!(
+            serde_json::from_str(r#"{"ErrorNumber": 0, "ErrorMessage": ""}"#)?,
+            ASCOMError {
+                code: ASCOMErrorCode::OK,
+                ..
+            }
+        );
+
+        assert_matches!(
+            serde_json::from_str(r#"{"ErrorNumber": 1025, "ErrorMessage": "bad"}"#)?,
+            ASCOMError {
+                code: ASCOMErrorCode::INVALID_VALUE,
+                ..
+            }
+        );
+
+        Ok(())
+    }
+}