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

Track open/close states for individual doors #3962

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

SaswatPadhi
Copy link
Contributor

This PR supersedes #3208.

In this PR, we introduce code changes to track the 4 individual door open/close states from Tesla's vehicle_state API, that are currently aggregated into the doors_open state in Teslamate.

This PR also includes changes required to publish these 4 states on MQTT.

I have tested the changes with my own car and HA's MQTT integration.

Copy link

netlify bot commented Jun 16, 2024

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit 3dd3b4e
🔍 Latest deploy log https://app.netlify.com/sites/teslamate/deploys/66725f93a702ae0009156451
😎 Deploy Preview https://deploy-preview-3962--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@brianmay
Copy link
Collaborator

Just wondering, do we want to have individual topics for each door?

We could have one topic that publishes everything as a json.

One one hand, doing them individually is in line with what we already have.

On the other hand, I think if you are more likely to want to access all the doors, and only requiring one subscription makes it easier.

Not too fussed either way. Maybe both is an option also.

@SaswatPadhi
Copy link
Contributor Author

Thanks for the initial comments, @brianmay. You raised a bunch of different interesting points; I'm inlining my response to each below.

We could have one topic that publishes everything as a json.

At least for Home Assistant this would slightly complicate the corresponding MQTT-based sensors, which is my main use case. Doable, but a bit cumbersome, to extract the relevant parts out of the JSON payload and set up the sensors.

One one hand, doing them individually is in line with what we already have.

Yes, for example the 4x TPMS warning topics, and the 4x TPMS pressure value topics.
And I like those. Makes it super easy to consume these values downstream without additional unpacking.

On the other hand, I think if you are more likely to want to access all the doors, and only requiring one subscription makes it easier.

Good point, but the existing doors_open topic kinda captures "all the doors" case -- it tracks if all the doors are closed.

I'm personally more interested in keeping individual topics, but as you said, having both is an option as well.

@JakobLichterfeld
Copy link
Collaborator

Echoing Brian on this. For the location topic we opted for a single topic with both, latitude and longitude in the same topic as they always belong to each other. Door state does not correlate, but having 7 door states (4 doors, trunk, frunk) and an all would be confusing.

CHANGELOG.md Outdated Show resolved Hide resolved
@JakobLichterfeld JakobLichterfeld added the enhancement New feature or request label Jun 18, 2024
@SaswatPadhi
Copy link
Contributor Author

Thanks @JakobLichterfeld for chiming in.

For the location topic we opted for a single topic with both, latitude and longitude in the same topic as they always belong to each other.

Exactly, for location it makes sense to have them together -- nobody will likely create a latitude sensor and a separate longitude sensor.

Door state does not correlate, but having 7 door states (4 doors, trunk, frunk) and an all would be confusing.

Hmm, Tesla APIs, the Tesla integration from alandtse, and Teslemetry all provide 6 different sensors (4 doors + 2 trunks).
Now that we'll will be exposing the fine-grained values, we could reconsider whether we still need the "all" option (that none of the other tools I named I above expose), but that's a separate discussion.

All that said though, I'd love to be able to use these (either as individual topics, or extracted out of JSON payload) as soon as possible, so I'd be okay with merging the JSON-style topic if you and @brianmay feel strongly about it.

@brianmay
Copy link
Collaborator

The difference here and other places is that having 7 seperate topics is never going to give us an invalid state. Even if all 7 doors are closed at the same time, if we get them as 7 seperate events there is never going to be any invalid state in the transition.

This is unlike latitude/longitude where receiving a latitude without the longitude will give an invalid location until we receive both values.

Or - the other case I have discovered recently, receiving a "Charging" message followed by a charge limit means my software will make announcements "The car is charging from 70 percent to 50 percent" shortly before saying "The car is charging from 70 percent to 90 percent". Something I have been meaning to try to address.

But it does mean that software - including Home Assistant - is going to have to subscribe to 7 different topics to get all events. And it it highly likely that you are going to want to have all events. I think there is going to be some sort of overhead, even in Home Assistant to subscribing to more topics then required.

I don't see the issue with having individual topics and an all topic. I propose we merge this as is, and look at doing an all topic (and maybe some sort of combined charging topic) at a later stage.

@SaswatPadhi
Copy link
Contributor Author

But it does mean that software - including Home Assistant - is going to have to subscribe to 7 different topics to get all events. And it it highly likely that you are going to want to have all events. I think there is going to be some sort of overhead, even in Home Assistant to subscribing to more topics then required.

MQTT is a pub/sub (publish/subscribe) protocol though. It does not periodically poll for updates. Subscribing to 6 different topics may likely have less overhead compared to subscribing to 1 topic that is published to 6x more frequently with a 6x larger JSON payload. Furthermore, on any downstream application (e.g. HA sensors), unpacking the JSON payload itself would incur additional overhead.
Taking this reasoning to the extreme -- having a single teslamate topic with the various states exposed withing a huge JSON payload will actually incur massive overhead, since new messages (with huge JSON payloads) on this topic will be published on any state change. Instead having separate topics allows for smaller payloads and each topic to be updated async at its own rate.

I don't see the issue with having individual topics and an all topic. I propose we merge this as is, and look at doing an all topic (and maybe some sort of combined charging topic) at a later stage.

Thank you, please take a look at the changes. I have fixed the linter warnings and addressed @JakobLichterfeld's comment above.

@brianmay
Copy link
Collaborator

I rerun the tests, this time they passed. Looks like there still are some intermittent faults here.

@JakobLichterfeld JakobLichterfeld merged commit 195433c into teslamate-org:master Jun 19, 2024
12 checks passed
@jlestel
Copy link
Contributor

jlestel commented Jun 19, 2024

For information, the choice of Tesla Telemetry team is to keep only one sensor: sensor.*_doorstate with sample value {"DoorState": "DriverFront|PassengerFront"}

@SaswatPadhi SaswatPadhi deleted the door_states branch June 19, 2024 17:16
@SaswatPadhi
Copy link
Contributor Author

SaswatPadhi commented Jun 19, 2024

Hi @jlestel, thanks for the link to the sheet. It documents two different things: (a) what data Tesla's Fleet API exposes, and (b) what HA sensors Teslemetry exposes. The sensor.*_doorstate currently does not exist. I'm not sure if it is outdated, or planned for the future. Also note that there is no mention of frunk or trunk in that sheet.

I recently switched from Teslemetry to Teslamate though, and Teslemetry exposes 4 door sensors named binary_sensor.vehicle_state_{df,dr,pf,pr}:
https://github.com/Teslemetry/hass-teslemetry/blob/main/custom_components/teslemetry/binary_sensor.py#L171-L194
Their HomeAssistant integration page also describes the 4 sensors: https://www.home-assistant.io/integrations/teslemetry/

Regarding the Tesla Fleet API data, I checked the vehicle_data end point documented at https://developer.tesla.com/docs/fleet-api#vehicle_data. If you click on the right side, where it says "Click to view successful response", you will still see the df, dr, pf, pr fields under vehicle_state in the response. So I am confused as to what this "DoorState" in the sheet refers to. I am also confused as to what the sample value of DriverFront|PassengerFront means.

@jlestel
Copy link
Contributor

jlestel commented Jun 20, 2024

It doesn't seem to be clear but what I sent you are the sensors planned by the Tesla Telemetry team. Basically, Telemetry is the streamed version of the Fleet API.

I'm not talking about custom implementations of third-party services.

I just wanted to tell you that official Tesla Telemetry will only send this field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants