Fix review comments

- Rename temp to temperature in variable names and add renaming in serde - Add module for TemperatureUnit enum and remove declaration from results - Remove set_min_temperature() function - Replace get_control_range() with min/max from get_device_info() - Replace `unwrap()` with `?`
mihai-dinculescu · Nov 5, 2023 · d1352d8 · d1352d8
1 parent 7022521
commit d1352d8
Show file tree

Hide file tree

Showing 9 changed files with 66 additions and 98 deletions.
diff --git a/tapo/examples/tapo_ke100.rs b/tapo/examples/tapo_ke100.rs
@@ -18,9 +18,9 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
     let tapo_username = env::var("TAPO_USERNAME")?;
     let tapo_password = env::var("TAPO_PASSWORD")?;
     let ip_address = env::var("IP_ADDRESS")?;
-    // ID of the KE100 device. Can be obtained from executing `get_child_device_component_list_json()`` on the hub device.
+    // ID of the KE100 device. Can be obtained from executing `get_child_device_component_list()`` on the hub device.
     let device_id = env::var("DEVICE_ID")?;
-    let target_temp: u8 = env::var("TEMPERATURE")?.parse().unwrap();
+    let target_temperature: u8 = env::var("TARGET_TEMPERATURE")?.parse()?;
 
     let hub = ApiClient::new(tapo_username, tapo_password)?
         .h100(ip_address)
@@ -34,7 +34,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
     info!("Device info: {device_info:?}");
 
     // Set temperature on target device
-    device.set_temperature(target_temp).await?;
+    device.set_target_temperature(target_temperature).await?;
 
     // Get the device info of the child device
     let device_info = device.get_device_info().await?;

diff --git a/tapo/src/api/child_devices/ke100_handler.rs b/tapo/src/api/child_devices/ke100_handler.rs
@@ -45,34 +45,19 @@ impl<'h> KE100Handler<'h> {
     /// # Arguments
     ///
     /// * `target_temperature` - between min_control_temp and max_control_temp
-    pub async fn set_temperature(&self, target_temperature: u8) -> Result<(), Error> {
+    pub async fn set_target_temperature(&self, target_temperature: u8) -> Result<(), Error> {
 
-        let control_range = self.get_control_range().await?;
+        //let control_range = self.get_control_range().await?;
+        let device_info = self.get_device_info().await?;
 
-        if target_temperature < control_range[0] || target_temperature > control_range[1] {
+        if target_temperature < device_info.min_control_temperature || target_temperature > device_info.max_control_temperature {
             return Err(Error::Validation {
                 field: "target_temperature".to_string(),
-                message: format!("Target temperature must be between {} (min_control_temp) and {} (max_control_temp)", control_range[0], control_range[1]),
+                message: format!("Target temperature must be between {} (min_control_temp) and {} (max_control_temp)", device_info.min_control_temperature, device_info.max_control_temperature),
             });
         }
 
-        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().target_temp(target_temperature)?)?;
-        let request = TapoRequest::SetDeviceInfo(Box::new(TapoParams::new(json)));
-
-        self.hub_handler
-            .control_child::<serde_json::Value>(self.device_id.clone(), request)
-            .await?;
-
-        Ok(())
-    }
-
-    /// Sets the *min temperature*, which is applied in frost protection mode.
-    ///
-    /// # Arguments
-    ///
-    /// * `min_temperature` - between 5 and 15
-    pub async fn set_min_temperature(&self, min_temperature: u8) -> Result<(), Error> {
-        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().min_temp(min_temperature)?)?;
+        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().target_temperature(target_temperature)?)?;
         let request = TapoRequest::SetDeviceInfo(Box::new(TapoParams::new(json)));
 
         self.hub_handler
@@ -88,7 +73,7 @@ impl<'h> KE100Handler<'h> {
     ///
     /// * `min_control_temperature`
     pub async fn set_min_control_temperature(&self, min_control_temperature: u8) -> Result<(), Error> {
-        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().min_control_temp(min_control_temperature)?)?;
+        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().min_control_temperature(min_control_temperature)?)?;
         let request = TapoRequest::SetDeviceInfo(Box::new(TapoParams::new(json)));
 
         self.hub_handler
@@ -104,7 +89,7 @@ impl<'h> KE100Handler<'h> {
     ///
     /// * `max_control_temperature`
     pub async fn set_max_control_temperature(&self, max_control_temperature: u8) -> Result<(), Error> {
-        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().max_control_temp(max_control_temperature)?)?;
+        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().max_control_temperature(max_control_temperature)?)?;
         let request = TapoRequest::SetDeviceInfo(Box::new(TapoParams::new(json)));
 
         self.hub_handler
@@ -150,9 +135,9 @@ impl<'h> KE100Handler<'h> {
     ///
     /// # Arguments
     ///
-    /// * `temp_offset` - between 5 and 30
-    pub async fn set_temp_offset(&self, temp_offset: i8) -> Result<(), Error> {
-        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().temp_offset(temp_offset)?)?;
+    /// * `temperature_offset` - between 5 and 30
+    pub async fn set_temperature_offset(&self, temperature_offset: i8) -> Result<(), Error> {
+        let json = serde_json::to_value(TrvSetDeviceInfoParams::new().temperature_offset(temperature_offset)?)?;
         let request = TapoRequest::SetDeviceInfo(Box::new(TapoParams::new(json)));
 
         self.hub_handler
@@ -161,16 +146,4 @@ impl<'h> KE100Handler<'h> {
 
         Ok(())
     }
-
-    /// Returns *min_control_temp* and *max_control_temp* as Vec<u8>.
-    async fn get_control_range(&self) -> Result<Vec<u8>, Error> {
-            let request = TapoRequest::GetDeviceInfo(TapoParams::new(EmptyParams));
-
-            self.hub_handler
-                .control_child::<KE100Result>(self.device_id.clone(), request)
-                .await?
-                .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult))
-                .map(|result| vec![result.min_control_temp,result.max_control_temp])
-        }
-
 }
diff --git a/tapo/src/api/child_devices/t31x_handler.rs b/tapo/src/api/child_devices/t31x_handler.rs
@@ -41,6 +41,6 @@ impl<'h> T31XHandler<'h> {
             .await?
             .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult));
 
-        Ok(result.unwrap().try_into()?)
+        Ok(result?.try_into()?)
     }
 }
diff --git a/tapo/src/requests/set_device_info/trv.rs b/tapo/src/requests/set_device_info/trv.rs
@@ -4,25 +4,25 @@ use crate::error::Error;
 
 #[derive(Debug, Default, Serialize)]
 pub(crate) struct TrvSetDeviceInfoParams {
+    #[serde(skip_serializing_if = "Option::is_none", rename = "target_temp")]
+    target_temperature: Option<u8>,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub target_temp: Option<u8>,
+    frost_protection_on: Option<bool>,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub frost_protection_on: Option<bool>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub child_protection: Option<bool>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub temp_offset: Option<i8>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub min_temp: Option<u8>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub min_control_temp: Option<u8>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub max_control_temp: Option<u8>,
+    child_protection: Option<bool>,
+    #[serde(skip_serializing_if = "Option::is_none", rename = "temp_offset")]
+    temperature_offset: Option<i8>,
+    #[serde(skip_serializing_if = "Option::is_none", rename = "min_temp")]
+    min_temperature: Option<u8>,
+    #[serde(skip_serializing_if = "Option::is_none", rename = "min_control_temp")]
+    min_control_temperature: Option<u8>,
+    #[serde(skip_serializing_if = "Option::is_none", rename = "max_control_temp")]
+    max_control_temperature: Option<u8>,
 }
 
 impl TrvSetDeviceInfoParams {
-    pub fn target_temp(mut self, value: u8) -> Result<Self, Error> {
-        self.target_temp = Some(value);
+    pub fn target_temperature(mut self, value: u8) -> Result<Self, Error> {
+        self.target_temperature = Some(value);
         self.validate()
     }
     pub fn frost_protection_on(mut self, value: bool) -> Result<Self, Error> {
@@ -33,39 +33,35 @@ impl TrvSetDeviceInfoParams {
         self.child_protection = Some(value);
         self.validate()
     }
-    pub fn temp_offset(mut self, value: i8) -> Result<Self, Error> {
-        self.temp_offset = Some(value);
-        self.validate()
-    }
-    pub fn min_temp(mut self, value: u8) -> Result<Self, Error> {
-        self.min_temp = Some(value);
+    pub fn temperature_offset(mut self, value: i8) -> Result<Self, Error> {
+        self.temperature_offset = Some(value);
         self.validate()
     }
-    pub fn min_control_temp(mut self, value: u8) -> Result<Self, Error> {
-        self.min_control_temp = Some(value);
+    pub fn min_control_temperature(mut self, value: u8) -> Result<Self, Error> {
+        self.min_control_temperature = Some(value);
         self.validate()
     }
-    pub fn max_control_temp(mut self, value: u8) -> Result<Self, Error> {
-        self.max_control_temp = Some(value);
+    pub fn max_control_temperature(mut self, value: u8) -> Result<Self, Error> {
+        self.max_control_temperature = Some(value);
         self.validate()
     }
 }
 
 impl TrvSetDeviceInfoParams {
     pub(crate) fn new() -> Self {
         Self {
-            target_temp: None,
+            target_temperature: None,
             frost_protection_on: None,
             child_protection: None,
-            temp_offset: None,
-            min_temp: None,
-            min_control_temp: None,
-            max_control_temp: None,
+            temperature_offset: None,
+            min_temperature: None,
+            min_control_temperature: None,
+            max_control_temperature: None,
         }
     }
 
     pub fn validate(self) -> Result<Self, Error> {
-        if let Some(temp_offset) = self.temp_offset {
+        if let Some(temp_offset) = self.temperature_offset {
             if temp_offset < -10 || temp_offset> 10 {
                 return Err(Error::Validation {
                     field: "temp_offset".to_string(),

diff --git a/tapo/src/responses.rs b/tapo/src/responses.rs
@@ -14,6 +14,7 @@ mod tapo_response;
 mod tapo_result;
 mod token_result;
 mod trigger_logs_result;
+mod temperature_unit;
 
 pub use child_device_list_result::*;
 pub use current_power_result::*;
@@ -23,6 +24,7 @@ pub use device_usage_result::*;
 pub use energy_data_result::*;
 pub use energy_usage_result::*;
 pub use trigger_logs_result::*;
+pub use temperature_unit::*;
 
 pub(crate) use control_child_result::*;
 pub(crate) use decodable_result_ext::*;

diff --git a/tapo/src/responses/child_device_list_result.rs b/tapo/src/responses/child_device_list_result.rs
@@ -64,7 +64,7 @@ pub enum ChildDeviceResult {
     T310(Box<T31XResult>),
     /// T315 temperature & humidity sensor.
     T315(Box<T31XResult>),
-    /// KE100 TRV.
+    /// KE100 thermostatic radiator valve (TRV).
     KE100(Box<KE100Result>),
     /// Catch-all for currently unsupported devices.
     /// Please open an issue if you need support for a new device.

diff --git a/tapo/src/responses/child_device_list_result/ke100_result.rs b/tapo/src/responses/child_device_list_result/ke100_result.rs
@@ -1,16 +1,7 @@
 use serde::{Deserialize, Serialize};
 
 use crate::error::Error;
-use crate::responses::{decode_value, DecodableResultExt, Status, TapoResponseExt};
-
-/// Temperature unit.
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
-#[serde(rename_all = "camelCase")]
-#[allow(missing_docs)]
-pub enum TemperatureUnit {
-    Celsius,
-    Fahrenheit,
-}
+use crate::responses::{decode_value, DecodableResultExt, Status, TapoResponseExt, TemperatureUnit};
 
 
 /// KE100 TRV.
@@ -41,13 +32,18 @@ pub struct KE100Result {
     pub r#type: String,
     #[serde(rename = "temp_unit")]
     pub temperature_unit: TemperatureUnit,
-    pub current_temp: f32,
-    pub target_temp: f32,
-    pub min_control_temp: u8,
-    pub max_control_temp: u8,
+    #[serde(rename = "current_temp")]
+    pub current_temperature: f32,
+    #[serde(rename = "target_temp")]
+    pub target_temperature: f32,
+    #[serde(rename = "min_control_temp")]
+    pub min_control_temperature: u8,
+    #[serde(rename = "max_control_temp")]
+    pub max_control_temperature: u8,
     pub frost_protection_on: bool,
     pub location: String,
-    pub temp_offset: i8,
+    #[serde(rename = "temp_offset")]
+    pub temperature_offset: i8,
     pub child_protection: bool,
 }
 

diff --git a/tapo/src/responses/child_device_list_result/t31x_result.rs b/tapo/src/responses/child_device_list_result/t31x_result.rs
@@ -3,16 +3,7 @@ use itertools::izip;
 use serde::{Deserialize, Serialize};
 
 use crate::error::Error;
-use crate::responses::{decode_value, DecodableResultExt, Status, TapoResponseExt};
-
-/// Temperature unit.
-#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
-#[serde(rename_all = "camelCase")]
-#[allow(missing_docs)]
-pub enum TemperatureUnit {
-    Celsius,
-    Fahrenheit,
-}
+use crate::responses::{decode_value, DecodableResultExt, Status, TapoResponseExt, TemperatureUnit};
 
 /// T310/T315 temperature & humidity sensor.
 ///

diff --git a/tapo/src/responses/temperature_unit.rs b/tapo/src/responses/temperature_unit.rs
@@ -0,0 +1,10 @@
+use serde::{Deserialize,Serialize};
+
+/// Temperature unit.
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+#[allow(missing_docs)]
+pub enum TemperatureUnit {
+    Celsius,
+    Fahrenheit,
+}