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

fix: correctly represent online and asleep states according to API #373

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

carleeno
Copy link
Contributor

@carleeno carleeno commented Dec 4, 2022

This refactors how vehicle states are handled in Controller by tracking the state string itself, and using helper methods to reflect if car is online or asleep.

States

Importantly, this changes the behavior of the Online binary_sensor in HA to reflect if the car is available. Since the car remains available when asleep (possible to wake it), and the Online sensor in HA implies that the device is available, this change should reduce confusion about the online state (alandtse/tesla#389, alandtse/tesla#387).

Additionally, this adds a car.is_asleep property which will more accurately reflect if the car is asleep, as it will remember if it's asleep even if the car is temporarily unavailable due to a signal loss or server outage. A small update to the HA integration will be needed to start using this new property.

This also fixes a related issue that the Online sensor would flicker for any transient api request failure. It is common for me to find the Online sensor is often flapping between disconnected and connected, however the reported state from the vehicle list API shows that it was online the entire time.

(flapping of the online sensor due to transient api request failures)
image

(compared to actual api state recorded separately)
image

Now, the online sensor and state attribute will reflect that of Tesla api except when:

  • Tesla api is reporting state is online
  • and there have been 5 or more consecutive Tesla Exceptions for the vehicle
    ...in which case the state will show unavailable and Online sensor will be false until the next api request for that vehicle succeeds.

Wake if asleep

The wake_up decorator was removed as it was only used for the api() method. The wake_if_asleep logic has been refactored between the Controller.wake_up and Controller.api methods and made to be more readable and handle edge cases better:

  • If api is called for a endpoint that does not accept a vehicle_id, and wake_if_asleep=True, it will raise an exception
    • with the decorator, it would have failed as it's not possible to wake a vehicle without it'd ID, but also it's not needed to wake a vehicle for these types of commands
  • If the api is called with wake_if_asleep=True and the car has gone to sleep so recently that the Controller doesn't know it yet, the initial api request will fail, but now it will handle that gracefully, wake the vehicle, and try the command again
    • it appeared the decorator was supposed to do this, but due to some misplaced conditions, it did not.

It's also more efficient by only retrying the wake command if it failed, otherwise it waits for it to come online by polling the VEHICLE_SUMMARY endpoint at regular intervals with it's own timeout.

Tests

  • Added unit tests to ensure the state and online/asleep properties updated as expected
  • Tested in a HA dev container with my car, update interval set to 15 seconds, and ensured that the integration allowed the car to sleep once I turned off sentry mode.
  • Ensured that the sensors updated as expected to reflect my actual car state
  • Ensured that installing the integration did wake up the car even though it was asleep
  • Ensured reloading the integration did not wake up the car from sleep
  • Ensured no new warnings or errors in HA logs.
  • Ensured that sending a pointless command to the car while it's asleep doesn't wake it (such as climate off)
  • Ensured that state correctly shows offline when vehicle is actually offline

@alandtse
Copy link
Collaborator

alandtse commented Dec 4, 2022

Ok, if the online sensor is changing, can we argue that it's just a fix and not a breaking change? I can be convinced it's a better representation and thus a fix of bad behavior, particularly if that's what we had before (I can't remember).

EDIT: On the transience of the unavailable state. I'm a bit more conflicted. Under HA, if you cannot reach the UI, the correct response is Unavailable. This allows automations to then account for the bad state.

teslajsonpy/controller.py Outdated Show resolved Hide resolved
_LOGGER.debug(
"%s: %s: Wake Attempt(%s): %s. Next attempt in %s",
instance._id_to_vin(car_id)[-5:],
wrapped.__name__,
retries,
result,
wake_success,
15 + sleep_delay ** (retries + 2),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It may make sense to revisit the base wake time since we're editing this part. I believe the app is using something larger like 30. I think recent testing shows you can't ever get the car woken up by the first wake attempt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, I also saw that the first attempt usually failed, I'll collect some real world data and adjust it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh man I'm glad I dug into this more, it's not that it takes long to wake, it's that the logic was using the response from the wake command before retrying...it should update to get a new response before retying to wake.

I wrote a script to test this and found that it takes less than 10 seconds to wake up, and only 1 command needs to be sent (unless it fails).

waking car...
state after wake attempt: asleep, time: 0.2
Updating...
state after refresh: asleep, time: 0.9
[...]
Updating...
state after refresh: asleep, time: 8.7
Updating...
state after refresh: online, time: 9.5

Doing this I also realized the entire wake_up decorator can be replaced with 3 lines. I guess a decorator was useful in the old version if it was used in several places, but it's only used in Controller.api() now, so it can be massively simplified. I'll have an update later today, I want to do lots of real-world testing with this change now that it's going to affect the wakeup logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I pushed a big cleanup of the wakeup logic.
It took me most of the day to understand what the old decorator was trying to do, and after realizing that it was quite neglected and optimized, I condensed it into Controller.api(), and also fixed a few minor edge cases such as if wake_if_asleep is true but the car has went to sleep after the latest get_vehicles, or wake_if_asleep is true for an api request that does not use/have the vehicle ID (it's not possible to wake something without it'd ID).

Real-world testing so far has been really promising, wake ups are faster, the data is updated faster, and less "WAKE_UP" api requests are made.

I also cleaned up about 100 lines of redundant parameters in car.py, and ensured that the command isn't attempted if the vehicle is asleep and we don't want to wake it, along with some better wake_if_asleep determination to not wake the car if we know it's a pointless command (such as turning the climate off. if the car is asleep, we know the climate is off)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess that last change will help close alandtse/tesla#379. Do you think your changes cover all such cases or just the few you noticed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I missed that one in particular, I'll go through and add value comparisons to fix this for number entities as well

teslajsonpy/controller.py Outdated Show resolved Hide resolved
@carleeno
Copy link
Contributor Author

carleeno commented Dec 4, 2022

Ok, if the online sensor is changing, can we argue that it's just a fix and not a breaking change? I can be convinced it's a better representation and thus a fix of bad behavior, particularly if that's what we had before (I can't remember).

Why not both? I feel that it's a fix as we weren't properly representing online, online should be used to represent when the vehicle is truly offline or not available. But it also is a breaking change to anybody who uses online to detect sleep. They just need to switch over to the new-ish asleep sensor.

EDIT: On the transience of the unavailable state. I'm a bit more conflicted. Under HA, if you cannot reach the UI, the correct response is Unavailable. This allows automations to then account for the bad state.

This is true that it should show if it's unavailable. but what I have found is that the following sequence of events will commonly happen:

  1. state is online
  2. send a command (eg climate on)
  3. command succeeds, but the followup update to get the climate state fails with a 408
  4. HA reflects that the vehicle went offline right after sending the command
  5. I check my app and find the climate is actually on
  6. HA later updates to show the climate is on, and state goes back to online

Because the failures are transient, I'd argue its not actually unavailable, but just needs to be re-checked.
The follow-up PR would handle truly unavailable cases were consecutive responses are 408 by then showing it's unavailable. That state will take priority over anything Tesla shows in the vehicle list to prevent the flapping as well.

In the current PR, if the api request to turn on the climate fails, it will still show the toast in HA that it failed.

@carleeno
Copy link
Contributor Author

carleeno commented Dec 4, 2022

now that I think about it more, I could go ahead and implement the unavailable state override for consecutive failures without adding the retry logic by tracking failures across all api calls. This will ensure we don't wrongly hide if the vehicle has nothing but 408 until I finish the retry PR. I'll add that shortly.

@carleeno carleeno marked this pull request as draft December 4, 2022 15:17
@carleeno
Copy link
Contributor Author

carleeno commented Dec 4, 2022

I'm doing a bit more testing and refining, I'm not quite happy with the unavailable state logic yet.

@alandtse
Copy link
Collaborator

alandtse commented Dec 4, 2022

Thanks. This is turning into quite an involved PR. Please consider of it makes sense to split into multiple so we can revert changes on parts if there's something we didn't consider.

And thanks for figuring out the wakeup. That was some really early code when I just found out about decorators.

@carleeno
Copy link
Contributor Author

carleeno commented Dec 5, 2022

Thanks. This is turning into quite an involved PR. Please consider of it makes sense to split into multiple so we can revert changes on parts if there's something we didn't consider.

And thanks for figuring out the wakeup. That was some really early code when I just found out about decorators.

Sure I'll split out what I can, I need to ensure that the separate PRs are fully functional on their own to ensure reverts wouldn't fail for conflicts, so splitting some of it might not be possible.

@carleeno
Copy link
Contributor Author

carleeno commented Dec 8, 2022

I've decided to split this into 4 parts:

  1. don't wake for pointless commands
  2. wake_up decorator refactor/fixes
  3. TeslaException retry logic for Controller.api()
  4. state fixes (this PR, probably the most controversial change, and the only breaking change)

When I tried to add the unavailable state logic without retries it was really messy and felt like a hack...so I'm going to add the api retry logic first, so that we can simply set unavailable if a command fails all retries.

There will likely be merge conflicts if we try to revert a lower number while keeping a higher number, especially as 4 depends on 2 and 3...but at least we can step back if needed, and/or slowly roll these changes in stages. But because of the close proximity of edits across these 4 prs, I'll wait to publish/update each subsequent PR until the previous one is approved and merged in, otherwise it'll be tough for me to build them completely independently and then merge them all together.

@alandtse
Copy link
Collaborator

alandtse commented Dec 9, 2022

Since it's a series of PRs, I'm not immediately pushing a release (unless someone else asks for it for a different PR). Let me know if you want intermediary releases at any point you're doing this.

@carleeno
Copy link
Contributor Author

carleeno commented Dec 9, 2022

Since it's a series of PRs, I'm not immediately pushing a release (unless someone else asks for it for a different PR). Let me know if you want intermediary releases at any point you're doing this.

I imagine I should be able to finish this series by the end of weekend...but feel free to cut a release at any point as I'm keeping everything fully functional from 1 PR to the next. And I wouldn't want to hold up the nav destination feature because that's a pretty awesome update to have.

@alandtse
Copy link
Collaborator

alandtse commented Dec 9, 2022

We'll see if @InTheDaylight14 has time to complete it before you, but if you're both likely to get this done this weekend I can cut the release at the same time. Let me know your timing but don't forget you have to wait till teslajsonpy before you can get tesla PRs in.

@carleeno
Copy link
Contributor Author

carleeno commented Dec 9, 2022

We'll see if @InTheDaylight14 has time to complete it before you, but if you're both likely to get this done this weekend I can cut the release at the same time. Let me know your timing but don't forget you have to wait till teslajsonpy before you can get tesla PRs in.

Thanks, yea I don't need a new teslajsonpy release for a tesla pr until after the last of this series lands.

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