-
Notifications
You must be signed in to change notification settings - Fork 57
Add FleetTelemetryMQTT vehicle module #617
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
Conversation
5c51133
to
07af2ed
Compare
This looks great, @RichieB2B, It's great to have fleet telemetry support for TWCManager. Only (exceedingly minor) feedback I have is that in the doco it refers to a single VIN however some owners will have multiple vehicles and would want to list multiple VINs in |
Multiple VINs is of course supported. I'll change the documentation to reflect this. |
b76dd29
to
e944282
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I'm excited by this direction. Two things I'm less excited about, however:
- This gives us a second MQTT client implementation using nearly-identical logic. Is there a way to dedupe this with the Teslamate integration so we don't have to maintain both?
- This mimics the implementation in
main
rather than the implementation in #571, which may be counterproductive. However, some of the abstraction created by #571 might simplify the deduplication as well.
@MikeBishop Whoops, I thought #571 was merged already (I didn't pay much attention to the TeslaMate client at the time I tested it). With that code we can create a generic Telemetry class and base TeslaMateVehicle and FleetTelemetryMQTT off of it. Do you want to merge #571 first or shall I attempt merge this MR and #571 into a new MR? |
Merged #571. |
4b413dd
to
ff18982
Compare
@MikeBishop I introduced the |
7815340
to
c49fce9
Compare
c49fce9
to
1ff6242
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only change I needed to make to get it not crashing; I'll watch for a bit and make sure that it's actually pulling data, but it looks good. Thanks for the work you've put in on this!
Co-authored-by: Mike Bishop <[email protected]>
It's run for a couple hours through some charging, a departure and return, and stopping the plugged-up car after returning. I don't see any Tesla API requests for data going out through the proxy after initial start-up, so I think it's safe to assume the Teslamate side is working. Ship it! |
Since 6278961 the Fleet Telemetry is all working great as well. Ready to merge! One remark: I copied over all the events/signals from the TeslaMate implementation but TWCManager does not seem to use these:
so I did not include them in the example Fleet Telemetry config file. The more signals you request, the more you will be charged. For TeslaMate it might be different: you keep these values for trending and reporting anyway so delevering them to TWCManager is free since TeslaMate has already colllected them. |
That's fine, and your analysis is correct that requesting more from Teslamate doesn't hurt anything much. For history, #571 was the beginnings of an attempt to separate out and merge pieces of the major overhaul that I had begun in #483. The goal of #483 was to move away from a reliance on serial connection to Gen2 TWCs and lean more heavily on the Tesla API, opening the door to controlling vehicles on UMCs, Gen3 TWCs, third-party EVSEs, etc. As a result, it needed to collect the power data we currently get from the TWC directly from the car. However, with the shift to Fleet API happening in the main branch, #483 has a lot of bit-rot and will probably need to be re-done if we still want it. For now, we should minimize what we get from the Fleet Telemetry to keep costs minimal. Long-term, we may need those additional values if we make another attempt at getting away from a single EVSE. |
Thanks for painting the bigger picture, makes a lot of sense. Can you merge this PR? |
This module allows to receive vehicle data from a Tesla Fleet Telemetry server instead of polling the
vehicle_data
endpoint.