nfarina / homebridge-tesla

Tesla plugin for homebridge: https://github.com/nfarina/homebridge
154 stars 38 forks source link

Add battery service #16

Closed jimhe closed 5 years ago

jimhe commented 5 years ago

This commit allows the user to specify chargeState in their config. It exposes a battery service that shows battery percentage, charging state, and alerts on battery low if battery percentage is less than or equal to sub-property alertLowBattery (default 20).

Resolves #14

nfarina commented 5 years ago

This looks great! What kinds of voice commands will trigger this? And does it (theoretically) support multiple vehicles?

jimhe commented 5 years ago

I will test voice commands when I get a chance later this week -- I will update the README with them.

AFAIK, each vehicle adds a new TeslaAccessory with all the services bundled inside of it, so it should support multiple vehicles just like the existing services inside that class.

jimhe commented 5 years ago

@nfarina -- I updated the README with the new voice commands possible.

nfarina commented 5 years ago

Looks awesome. I'll merge and give it a try, then deploy. Thanks so much!

nfarina commented 5 years ago

Oh one observation though - I don't think the low battery alert can be reliable can it? Because the server would need to poll (and wake up) the car periodically to check its status. It would only give you the alert if you just asked the car (via HomeKit) to do something?

jimhe commented 5 years ago

You're correct. It's not exactly an "alert" in the classical sense, but in the confines of HomeKit itself. In this case, if the car charge is below alertLowBattery, it will show a small exclamation point in the car's accessories to notify the user.

nfarina commented 5 years ago

Ah I see, it's an officially supported characteristic! That's cool. OK I might tweak the README language a bit to reflect that.

nfarina commented 5 years ago

I'm having some trouble getting this all working with my car. I can ask "What's the battery level of the Model 3" and I get "The Model 3 battery is at 87%" which is great. But I can't seem to make anything else work. Any idea how to tell it to start charging for instance?

jimhe commented 5 years ago

Hmm, my understanding is that BatteryService is more of a read-only service. I don't think the UI supports the setting of any of the characteristics. If it did, ideally there would be a new characteristic for BatteryService that allows the user to specific a TargetBatteryLevel. We could then add a callback on setting this characteristic to the Tesla API to alter the vehicle's percentage setpoint.

The only Siri commands I've verified are the ones I placed in the README.

nfarina commented 5 years ago

OK! I did a bunch of refactoring and changed the way some of this stuff works. Let me run these changes by you and see what you think:

1) Added your battery service (thanks!) 2) Changed climate control to be a specific named switch - "Turn on the Model 3 Climate Control" or "Turn on the Climate" or whatever you like. 3) Added a new named switch for connection state - "Turn on the Model 3 Connection". This will wake up the car so you can ask it about battery status and other stuff that won't wake up the car automatically. 4) Added a new named "Charger" switch so you can initiate charging (if you have scheduled charging at night for instance and want to charge during the day) - so "Turn on the Model 3 Charger" or whatever you like.

The only breaking change is the climate one - previously you could say "Turn on the Model 3" and it would turn on the climate. Now you have to make an explicitly named switch for that.

Thoughts?

CCing others I've seen active in the repo: @cbrandlehner @aveach @dlt- @stevesreed

stevesreed commented 5 years ago

These sounds like great changes. I’ll try them out this weekend.

For item 4, does turning off the switch stop charging? That’s something useful as well, so we can stop charging when the tou rates changing.

-Steve

On Jun 4, 2019, at 9:32 AM, Nick Farina notifications@github.com wrote:

OK! I did a bunch of refactoring and changed the way some of this stuff works. Let me run these changes by you and see what you think:

Added your battery service (thanks!) Changed climate control to be a specific named switch - "Turn on the Model 3 Climate Control" or "Turn on the Climate" or whatever you like. Added a new named switch for connection state - "Turn on the Model 3 Connection". This will wake up the car so you can ask it about battery status and other stuff that won't wake up the car automatically. Added a new named "Charger" switch so you can initiate charging (if you have scheduled charging at night for instance and want to charge during the day) - so "Turn on the Model 3 Charger" or whatever you like. The only breaking change is the climate one - previously you could say "Turn on the Model 3" and it would turn on the climate. Now you have to make an explicitly named switch for that.

Thoughts?

CCing others I've seen active in the repo: @cbrandlehner @aveach @dlt- @stevesreed

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

nfarina commented 5 years ago

Yes, turning the "Charger" switch off will stop charging.

jimhe commented 5 years ago

@nfarina, your changes sound fine to me!

nfarina commented 5 years ago

Great, it's deployed!

One new issue I'm seeing personally though - asking Siri to unlock the car also pops the front trunk. I think this started happening when the battery service was added? Wondering if anyone else is seeing this.