fix: correctly represent online and asleep states according to API

[carleeno]

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)

(compared to actual api state recorded separately)

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]

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.

[alandtse]

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.

[carleeno]

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

[carleeno]

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

[carleeno]

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)

[alandtse]

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?

[carleeno]

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

[carleeno]

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]

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]

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

[alandtse]

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]

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]

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]

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]

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]

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]

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.