Skip to content

Add enable_wake_on_start option to Tesla#33035

Merged
bdraco merged 1 commit into
home-assistant:devfrom
alandtse:wake_on_start
Mar 23, 2020
Merged

Add enable_wake_on_start option to Tesla#33035
bdraco merged 1 commit into
home-assistant:devfrom
alandtse:wake_on_start

Conversation

@alandtse
Copy link
Copy Markdown
Contributor

@alandtse alandtse commented Mar 20, 2020

Proposed change

Add an option for users to decide whether the Tesla component should wake up all cars on Home Assistant startup. This allows a user to choose whether cars should continue to sleep (and not update information) or to wake up the cars potentially interrupting long term hibernation and increasing vampire drain. Users who need to restart Home Assistant while their cars are parked away from charging (e.g., airports) would benefit with this option.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link
Copy Markdown

Hey there @zabuldon, mind taking a look at this pull request as its been labeled with a integration (tesla) you are listed as a codeowner for? Thanks!

@alandtse
Copy link
Copy Markdown
Contributor Author

Wait, do options need to be documented?

@MartinHjelmare
Copy link
Copy Markdown
Member

Should we extend the update listener for this new option? Do we need to reload the whole integration on option change, since we pass the option to the library instance connect method?

@alandtse
Copy link
Copy Markdown
Contributor Author

No I don't thinks so, this option only impacts the first time the integration is loaded. Once the integration is loaded, it doesn't do anything further so changing it after running doesn't do anything until the next time HA restarts.

@MartinHjelmare
Copy link
Copy Markdown
Member

That's why we should reload the config entry. That would apply it without need for restart.

@alandtse
Copy link
Copy Markdown
Contributor Author

I don't think you're understanding what this option does. It doesn't matter if the effect of the option applies only on the next restart because it only impacts behavior on that next restart.

It configures whether the integration will wake up every vehicle on HA restart. Given how often HA is updated and needs to be restarted, users may not want to always wake up their vehicles as a Tesla takes over 24 hours of non-activity to go into lower power mode. For example, if they are parked at an airport, they may want to restart HA or update HA without potentially draining the battery of their car they need to drive home. There is a real money cost to users to continuously wake up their vehicles when they are sleeping. We should provide the option to minimize that cost.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Mar 21, 2020

Why is it limited to waking up (or not) on home assistant restart? What is it that causes the potential wake up in the code?

@alandtse
Copy link
Copy Markdown
Contributor Author

It is limited to HA startup because it results in the entities starting as Unavailable. Some users may not want to see that. Once a vehicle wakes up naturally because of someone driving the car or opening the door, the information is automatically refreshed.

I don't understand your second question. There's a forced wake up command in the API. On HA restart, it's not sent if the option is enabled. If the option is disabled, it is sent.

@MartinHjelmare
Copy link
Copy Markdown
Member

What code is responsible for the wake up command get sent? On controller.connect call? Then this doesn't only happen on restart but when the config entry is set up. That also happens if the config entry is reloaded eg when the user edits the entity registry for entities part of the config entry.

So should we rename the option?

@alandtse
Copy link
Copy Markdown
Contributor Author

Yes, the connect call.

The option isn't available when a user initially sets up the integration because the options flow aren't shown on initial setup. The default behavior in that case is to not wake any vehicles which is what the integration currently does without this option at all.

What do you mean "edit the entity registry"? That forces a reload of the entity? I don't think that's documented anywhere that users would know that's happening.

I'm not opposed to renaming the option, I just don't think users would understand the "edit the registry" point., "Force cars awake on startup" seems to be a natural description of what happens if you enable the option.

@MartinHjelmare
Copy link
Copy Markdown
Member

There's a reload of the corresponding config entry if a user disables an entity.

@alandtse
Copy link
Copy Markdown
Contributor Author

For the specific entity type or for the entire integration?

Again, if you have a name you'd prefer I can put it in. If you think it's better to not provide this option to users, I can also close it. I was just trying to respond to a user request but frankly I don't care about this option.

@MartinHjelmare
Copy link
Copy Markdown
Member

The config entry is reloaded, if a user disables an entity, and the config entry supports unloading.

I don't have any good suggestions at the moment. Names are hard.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

The code looks good!

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 23, 2020

The default is to not wake on start right?

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 23, 2020

The default is to not wake on start right?

Never-mind, answered my own question.

@bdraco bdraco self-requested a review March 23, 2020 03:28
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 23, 2020

Testing this now

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 23, 2020

Confirmed it did not wake the car when not enabled.
Confirmed it woke the car when enabled.

@bdraco bdraco merged commit 087b672 into home-assistant:dev Mar 23, 2020
@lock lock Bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants