teslamate-org / teslamate

A self-hosted data logger for your Tesla 🚘
https://docs.teslamate.org
MIT License
5.97k stars 743 forks source link

feat: bundle MQTT data in one json blob #3660

Closed JakobLichterfeld closed 7 months ago

JakobLichterfeld commented 9 months ago

I am kind of uneasy having these sent as separate MQTT topics. For example, think of a program receiving MQTT events, it will receive destination, and then latitude, and then longitude. Although order is unpredictable. There will be a brief time when it has a mismatch of old values and new values. Outputting all of this in one json blob might be better.

Yes, I think this is already a problem with the latitude/longitude output.

(see https://github.com/teslamate-org/teslamate/pull/3657#issuecomment-1933195471)

WesSec commented 9 months ago

Continued discussion from #3657, focussed on active drive navigation data:

I fully agree, for "same time define data" such as latlong and initial navigation state. For the other travel ETA's not so much, these update throughout the journey, which will require to update the complete blob. It is likely that the latlong will be set (data will be pushed/updated) and the route will still be calculating, resulting in two messages anyway, not solving the issue. This can be tested

Also adding more stuff to a big json requires the processing client (often HA) to parse this json, where normally an mqtt topic can be read easily.

I think both ways can be done and advocated for, zigbee2mqtt is more json based too, but i'd suggest to not wait for a possible complete revamp of the mqtt structure for this awesome change already.

vincep5 commented 9 months ago

Also in regards to the other discussion https://github.com/teslamate-org/teslamate/pull/3657 It would be nice to have the distance and minutes to destination into MQTT so that I could automate things for my Home Assistant instance.

JakobLichterfeld commented 9 months ago

Also in regards to the other discussion https://github.com/teslamate-org/teslamate/pull/3657 It would be nice to have the distance and minutes to destination into MQTT so that I could automate things for my Home Assistant instance.

That's already merged and will be included in the next release.

LelandSindt commented 8 months ago

I would like to make sure I understand the intent here. The idea is to group similar topics into a single topic with a json payload.

for example

/teslamate/cars/1/latitude = 1234

and

/teslamate/cars/1/longitude = 1234

would be made available as

/teslamate/cars/1/location = {"latitude":123,"longitude":1234}

but not to group all topics into a single json payload...

/teslamate/cars/1 = { <all of the keys and values....> }

While I can see the value in grouping lat and long into location as they will be consumed together... grouping too many topics together negates the flexibility and scalability that MQTT provides by design.

JakobLichterfeld commented 8 months ago

Correct, the idea is to bundle the data which belongs together, like position data in one Json blob, time and distance to destination.

LelandSindt commented 8 months ago

Tagging @tobiasehlert and @brchri as their projects consume Teslamate MQTT payloads.

longzheng commented 8 months ago

Also in regards to the other discussion #3657 It would be nice to have the distance and minutes to destination into MQTT so that I could automate things for my Home Assistant instance.

That's already merged and will be included in the next release.

Just confirming that my PR did not include distance and minutes in the MQTT output, I only added it to the state parser

JakobLichterfeld commented 8 months ago

Just confirming that my PR did not include distance and minutes in the MQTT output, I only added it to the state parser

Thanks for clarification, absolutely right.

brianmay commented 8 months ago

I want to fix this now, but not sure what format I should use, examples include:

  1. latitude,longitude
  2. longitude,latitude
  3. {"latitude":123,"longitude":1234}
  4. above with elevation attribute also.

1 and 2 are smaller, but could be confusing, easy to get latitude and longitude confused. Not sure about 3rd party support[1] 3 Is bigger but not as ambiguous.
4 Includes height. Should elevation be considered part of the position?

I am not particularly fussed either way, but is important we try to get this right from day 0.

Notes:

[1] I think it might be possible in home-assistant, see https://community.home-assistant.io/t/split-sensor-data-into-two-using-comma-as-delimiter/567217. For node-red guessing https://flowfuse.com/node-red/core-nodes/split/ might work.

brchri commented 8 months ago

I want to fix this now, but not sure what format I should use, examples include:

  1. latitude,longitude
  2. longitude,latitude
  3. {"latitude":123,"longitude":1234}
  4. above with elevation attribute also.

1 and 2 are smaller, but could be confusing, easy to get latitude and longitude confused. Not sure about 3rd party support[1] 3 Is bigger but not as ambiguous. 4 Includes height. Should elevation be considered part of the position?

I am not particularly fussed either way, but is important we try to get this right from day 0.

Notes:

[1] I think it might be possible in home-assistant, see https://community.home-assistant.io/t/split-sensor-data-into-two-using-comma-as-delimiter/567217. For node-red guessing https://flowfuse.com/node-red/core-nodes/split/ might work.

I vote 3 or 4, for what it's worth. json is a standard for this type of usage, and for third parties, it seems like making an opinionated decision on whether lat or lng should be first could be contentious and compatibility may vary by consumer; json takes out the guesswork for consumers.

I'm not particularly opinionated on elevation being included, but as food for thought, KML spec does optionally bundle altitude with lat and lng when defining geofences, but I'm really not sure it matters as much.

JakobLichterfeld commented 8 months ago

I vote for option 4. Imo all position data belongs to each other, so lat, long and elevation.