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

Initial KE100 support. #121

Merged
merged 17 commits into from
Nov 25, 2023
Merged

Conversation

pwoerndle
Copy link
Contributor

Reading data works. Setting temperature and frost protection returns empty result from the device, which creates an EmptyResult runtime error.

@pwoerndle
Copy link
Contributor Author

I did some more digging and the EmptyResult error is thrown in control_child method in tapo/src/api_client.rs on the last statement.

    pub(crate) async fn control_child<R>(
        &self,
        device_id: String,
        child_request: TapoRequest,
    ) -> Result<R, Error>
    where
        R: fmt::Debug + DeserializeOwned + TapoResponseExt,
    {
        debug!("Control child...");
        let params = MultipleRequestParams::new(vec![child_request]);
        let request = TapoRequest::MultipleRequest(Box::new(TapoParams::new(params)));

        let params = ControlChildParams::new(device_id, request);
        let request = TapoRequest::ControlChild(Box::new(TapoParams::new(params)));

        let responses = self
            .protocol
            .execute_request::<ControlChildResult<TapoMultipleResponse<R>>>(request, true)
            .await?
            .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult))?
            .response_data
            .result
            .responses;

        let response = responses
            .into_iter()
            .next()
            .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult))?;

        validate_response(&response)?;

        response
           .result
           .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult))
    }

Since the control_child method is used for read and write operations, I guess we need to check the result to be empty at some point, but would it make sense to do the check, when we interpret the result?

@mihai-dinculescu
Copy link
Owner

Looking at the tapo app source code, I see a few fields that might be useful:

  • temp_unit string (it should probably be converted to an enum)
  • min_control_temp integer
  • max_control_temp integer
  • min_temp integer (it ends up getting used by frost protection)

You're correct. ApiClient::control_child should either be updated to handle empty responses or a copy of the function that can should be created.

@mihai-dinculescu
Copy link
Owner

Can you please run the example with RUST_LOG=debug in front and paste the inner response you get?

For example, get_trigger_logs returns something like

Device inner response decrypted: {"result":{"responseData":{"result":{"responses":[{"method":"get_trigger_logs","result":{"start_id":0,"logs":[],"sum":0},"error_code":0}]}}},"error_code":0}
Device inner response: TapoResponse { error_code: 0, result: Some(ControlChildResult { response_data: TapoMultipleResponse { result: TapoMultipleResult { responses: [TapoResponse { error_code: 0, result: Some(TriggerLogsResult { start_id: 0, sum: 0, logs: [] }) }] } } }) }

@pwoerndle
Copy link
Contributor Author

This what I get when executing device.set_temperature(19).await?;:

 2023-10-20T06:07:48.307Z INFO  tapo_ke100                                > EXECUTING COMMAND WITH EMPTY RESULT
 2023-10-20T06:07:48.307Z DEBUG tapo::api::api_client                     > Control child...
 2023-10-20T06:07:48.307Z DEBUG tapo::api::protocol::passthrough_protocol > Request to passthrough: {"method":"control_child","params":{"device_id":"REDACTED","requestData":{"method":"multipleRequest","params":{"requests":[{"method":"set_device_info","params":{"device_on":true,"target_temp":19}}]}}}}
 2023-10-20T06:07:50.506Z DEBUG tapo::api::protocol::passthrough_protocol > Device responded with: TapoResponse { error_code: 0, result: Some(TapoResult { response: "kSb5Ej1g6e5y/FqhXbR8zs2pbesSXEoFnpur25O4SvYOAZIpvJ6bXrBACLOoaxeYHShR60TbXG1bad+r/jBY1vjKaAvEN3TYXqkxTTHp051EIBT88l1z1rV538LWPgGcLy+hD1rUdDX0wMO/p7HvDwMPIv6wxhnG/NwGxaTxt8g=" }) }
 2023-10-20T06:07:50.506Z DEBUG tapo::api::protocol::passthrough_protocol > Device inner response decrypted: {"result":{"responseData":{"result":{"responses":[{"method":"set_device_info","error_code":0}]}}},"error_code":0}
 2023-10-20T06:07:50.507Z DEBUG tapo::api::protocol::passthrough_protocol > Device inner response: TapoResponse { error_code: 0, result: Some(ControlChildResult { response_data: TapoMultipleResponse { result: TapoMultipleResult { responses: [TapoResponse { error_code: 0, result: None }] } } }) }
 2023-10-20T06:07:50.507Z DEBUG tapo::api::protocol::passthrough_protocol > validate_response passed
 2023-10-20T06:07:50.507Z DEBUG tapo::api::protocol::passthrough_protocol > assign result passed

What do you think about splitting ApiClient::control_child into ApiClient::read_child and ApiClient::write_child to handle the None response for the write operation? I'm struggling a bit to make ApiClient::control_child handle both cases.

@mihai-dinculescu
Copy link
Owner

This should get the job done.

api_client

    pub(crate) async fn control_child<R>(
        &self,
        device_id: String,
        child_request: TapoRequest,
    ) -> Result<Option<R>, Error>
    where
        R: fmt::Debug + DeserializeOwned + TapoResponseExt,
    {
        debug!("Control child...");
        let params = MultipleRequestParams::new(vec![child_request]);
        let request = TapoRequest::MultipleRequest(Box::new(TapoParams::new(params)));

        let params = ControlChildParams::new(device_id, request);
        let request = TapoRequest::ControlChild(Box::new(TapoParams::new(params)));

        let responses = self
            .protocol
            .execute_request::<ControlChildResult<TapoMultipleResponse<R>>>(request, true)
            .await?
            .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult))?
            .response_data
            .result
            .responses;

        let response = responses
            .into_iter()
            .next()
            .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult))?;

        validate_response(&response)?;

        Ok(response.result)
    }

hub_handler

    pub(crate) async fn control_child<R>(
        &self,
        device_id: String,
        request_data: TapoRequest,
    ) -> Result<Option<R>, Error>
    where
        R: fmt::Debug + DeserializeOwned + TapoResponseExt,
    {
        self.client.control_child(device_id, request_data).await
    }

s200b_handler

    pub async fn get_device_info(&self) -> Result<S200BResult, Error> {
        let request = TapoRequest::GetDeviceInfo(TapoParams::new(EmptyParams));

        self.hub_handler
            .control_child(self.device_id.clone(), request)
            .await?
            .ok_or_else(|| Error::Tapo(TapoResponseError::EmptyResult))
    }

@pwoerndle
Copy link
Contributor Author

Thanks for the code snippets. I managed to resolve the issue with the None result in 8716347. Need to work a bit more on this to support temperatures in float and add the parameters you suggested above.

@pwoerndle
Copy link
Contributor Author

Meanwhile I added support for min/max_control temp, child_protection and temp_offset.
I added a min_temp parameter, but although the API doesn't respond with an error when setting a value the value on the device doesn't change (checked in the app). What is strange, is that neither device_info nor component_list seem to expose the min_temp parameter.
I'm thinking of adding a function to query the current temp directly for convenience as this will most likely be a common use case, apart from setting the temperature.

Let me know if you have any other thoughts on what could be added.

@mihai-dinculescu
Copy link
Owner

You're correct.
There's a separate endpoint called set_frost_protection that requires min_temp and temp_unit as parameters. It's up to you if you want to include it in this pull request. Alternatively, we could save it for a future update to keep this one more manageable.
By the way, there's also a get_frost_protection endpoint that I suspect will return the min_temp.

However, It would be useful to include temp_unit.

Regarding your question, there's a separate endpoint called get_temperature_records that should fulfill the task. You can implement it in a similar way to get_trigger_logs.

@pwoerndle
Copy link
Contributor Author

I've tried the changes you proposed but seem to be getting a compilation error in tapo/src/api/child_devices/ke100_handler.rs now:

error[E0698]: type inside `async fn` body must be known in this context
  --> tapo/src/api/child_devices/ke100_handler.rs:63:14
   |
63 |             .control_child(self.device_id.clone(), request)
   |              ^^^^^^^^^^^^^ cannot infer type for type parameter `R` declared on the method `control_child`
   |
note: the type is part of the `async fn` body because of this `await`

I suggest we try to get this one fixed and then close this PR. I'll keep working on the other stuff and will create another PR once ready.

Thanks for the help with getting the Rust code right. As you may have recognized I'm learning by doing here.

@pwoerndle
Copy link
Contributor Author

Never mind, I fixed it. If I understood the Rust documentation right, I needed to bind control_child: .control_child::<KE100Result>.

    pub async fn set_temperature(&self, target_temperature: u8) -> Result<(), Error> {

        let control_range = self.get_control_range().await?;

        if target_temperature < control_range[0] || target_temperature > control_range[1] {
            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]),
            });
        }

        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::<KE100Result>(self.device_id.clone(), request)
            .await?;

        Ok(())
    }

I assume I can use the same pattern for all the functions which don't return a result?

@mihai-dinculescu
Copy link
Owner

I assume I can use the same pattern for all the functions which don't return a result?

Exactly, yeah. However, it would be better to use .control_child::<serde_json::Value> instead of .control_child::<KE100Result>.

Please give me a shout when you're done with everything you want to do, and I'll do a review.
If you don't have the time to address all the little things, you can permit me to contribute to the PR.

@pwoerndle
Copy link
Contributor Author

Just pushed the last updates. I think the PR is ready to go for review.

@pwoerndle pwoerndle marked this pull request as ready for review October 29, 2023 17:28
tapo/src/api/child_devices/t31x_handler.rs Outdated Show resolved Hide resolved
tapo/src/responses/child_device_list_result.rs Outdated Show resolved Hide resolved
tapo/examples/tapo_ke100.rs Outdated Show resolved Hide resolved
tapo/examples/tapo_ke100.rs Outdated Show resolved Hide resolved
tapo/src/api/child_devices/ke100_handler.rs Outdated Show resolved Hide resolved
tapo/src/requests/set_device_info/trv.rs Show resolved Hide resolved
tapo/src/api/child_devices/ke100_handler.rs Show resolved Hide resolved
tapo/src/requests/set_device_info/trv.rs Show resolved Hide resolved
Copy link
Owner

@mihai-dinculescu mihai-dinculescu left a comment

Choose a reason for hiding this comment

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

Very well done. You have addressed all the important aspects, which is impressive considering you are new to Rust.

I am aware that I left a few comments, but they are all minor and mostly related to naming and unnecessary elements that can be removed.

@mihai-dinculescu mihai-dinculescu force-pushed the main branch 2 times, most recently from 4498527 to 79a4a91 Compare November 5, 2023 10:08
pwoerndle and others added 11 commits November 5, 2023 11:32
…ost protection returns empty result, which creates and EmptyResult error.
and handling None result in set_frost_protection_on response
Fixing documenation for target_temperature to be only u8.
Fixing type defintions for min/max_control_temp.
target_temperature is validated against current min/max_control_temp
Added function to set min_temp but currently does not affect device
As suggested by mihai-dinculescu

Co-authored-by: Mihai Dinculescu <[email protected]>
- 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 `?`
hub_handler returns Result<Option<R>, Error> for control_child().
@pwoerndle
Copy link
Contributor Author

I found one more thing I may need your help with. It's about decoding the base64 encoded nickname. The following fragment in example/tapo_ke100.rs returns the base64 encoded nickname:

let hub = ApiClient::new(tapo_username, tapo_password)?
    .h100(ip_address)
    .await?;

// Get a handler for the child device
let device = hub.ke100(device_id);

// Get the device info of the child device
let device_info = device.get_device_info().await?;
info!("Device info: {device_info:?}");

But when I use the detour via get_child_device_list() as in the following fragment, I get the decoded nickname in device.nickname

// Adapted code from examples/tapo_h100.rs

let hub = ApiClient::new(tapo_username, tapo_password)?
    .h100(ip_address)
    .await?;
let child_device_list = hub.get_child_device_list().await?;

for child in child_device_list {
    match child {
        ChildDeviceResult::KE100(device) => {
            info!(
                "Found KE100 child device with nickname: {}, id: {}, current temperature: {} {:?} and target temperature: {} {:?}.",
                device.nickname,
                device.device_id,
                device.current_temperature,
                device.temperature_unit,
                device.target_temperature,
                device.temperature_unit,
            );
        }

Do you see a similar behavior, when you test other child devices such as s200b or t300? Is this intended behavior or should we do something about this?

@mihai-dinculescu
Copy link
Owner

Ah, that is a defect. Nice find!
I've fixed it in fa73e40.

- Enforce single value TemperatureUnit enum for KE100 with Celsius
- Revert previous change to global TemperatureUnit enum
- Fix for nickname decoding
@pwoerndle
Copy link
Contributor Author

Merged your fixes for the nickname decoding into this PR. The decoding seems to work also with the feature I added to supported empty results for the control_child function.

@mihai-dinculescu
Copy link
Owner

I've committed some minor tweaks for language and lints.
I think this is in a good shape to ship 🚀

@mihai-dinculescu mihai-dinculescu merged commit 95f7098 into mihai-dinculescu:main Nov 25, 2023
13 checks passed
@mihai-dinculescu
Copy link
Owner

Released in v0.7.6.
Thank you for this amazing effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants