zabuldon / teslajsonpy

Apache License 2.0
56 stars 62 forks source link

fix: Replace charge_amps with charge_current_request in set_charging_amps #392

Closed Bre77 closed 1 year ago

Bre77 commented 1 year ago

When calling set_charging_amps it looks for a parameter charge_amps and uses this to save the updated value, however this parameter isnt used anywhere else. I believe this is actually meant to be the charge_current_request parameter.

Fixes https://github.com/alandtse/tesla/issues/479

Bre77 commented 1 year ago

To add this this, there doesn't appear to be any way to access the charge_amps parameter, so it could probably be completely removed, including the testing fixtures.

alandtse commented 1 year ago

Thanks. Can you please confirm the API call actually works as expected? You can test with the custom api in HA. Please also add a test for this so future changes don't revert this.

Lastly, mainly optional, we may want to submit a change to the unofficial API if Tesla has indeed changed the parameter. https://tesla-api.timdorr.com/vehicle/commands/charging#post-api-1-vehicles-id-command-set_charging_amps

EDIT: Clarified that the test change is required. The optional part would be the API documentation update upstream.

alandtse commented 1 year ago

Test failure is unrelated to your change and can be ignored. But please note the request to update the test.

Bre77 commented 1 year ago

~@alandtse if the correct parameter is charging_amps then would the fix instead be adding a public method to access it in the library, and for you to update the HACS integration to use that instead of charge_current_request? Right now the value that is set and the value that are read are completely different. (charging_amps/charge_current_request)~

EDIT: I think I was right the first time, the state value is charge_current_request as per https://tesla-api.timdorr.com/vehicle/state/chargestate

alandtse commented 1 year ago

Please do verify. There are state getters and commands to set stuff. You've linked the state command but I also provided the last known command. It's totally possible it's changed too.

Bre77 commented 1 year ago

Please do verify. There are state getters and commands to set stuff. You've linked the state command but I also provided the last known command. It's totally possible it's changed too.

Setting this value has not changed/broken in the HACS integration, I am using your integration succesfully to change this value already. It's only the state not being updated in Home Assistant that is broken.

Bre77 commented 1 year ago

I have confirmed that the "CHARGING_AMPS" command updates both "charge_amps" and "charge_current_request", so this PR is still valid.

"charge_state": {
       ...
        "charge_amps": 1,
        "charge_current_request": 1,
        "charge_current_request_max": 32,
        ...
Bre77 commented 1 year ago

For potential backwards compatability, and to match how the Tesla API behaves, I have changed set_charging_amps to set both charge_amps and charge_current_request in _vehicle_data

alandtse commented 1 year ago

Thanks!