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

feat: bundle MQTT data in one json blob #3660

Closed
JakobLichterfeld opened this issue Feb 8, 2024 · 11 comments · Fixed by #3729
Closed

feat: bundle MQTT data in one json blob #3660

JakobLichterfeld opened this issue Feb 8, 2024 · 11 comments · Fixed by #3729
Labels
area:teslamate Related to TeslaMate core enhancement New feature or request kind:idea Idea for new feature or some form of enhancement

Comments

@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Feb 8, 2024

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 #3657 (comment))

@JakobLichterfeld JakobLichterfeld added enhancement New feature or request kind:idea Idea for new feature or some form of enhancement area:teslamate Related to TeslaMate core labels Feb 8, 2024
@WesSec
Copy link

WesSec commented Feb 10, 2024

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
Copy link

vincep5 commented Feb 13, 2024

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.

@JakobLichterfeld
Copy link
Collaborator Author

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.

@LelandSindt
Copy link
Contributor

LelandSindt commented Feb 15, 2024

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
Copy link
Collaborator Author

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

@LelandSindt
Copy link
Contributor

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

@longzheng
Copy link
Contributor

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
Copy link
Collaborator Author

JakobLichterfeld commented Feb 19, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

brchri commented Mar 10, 2024

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
Copy link
Collaborator Author

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

brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit to brianmay/teslamate that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 11, 2024
brianmay added a commit that referenced this issue Mar 12, 2024
brianmay added a commit that referenced this issue Mar 12, 2024
brianmay added a commit that referenced this issue Mar 12, 2024
brianmay added a commit that referenced this issue Mar 17, 2024
JakobLichterfeld added a commit that referenced this issue Mar 18, 2024
* Add location topic

Fixes #3660.

* doc: update mqtt topics with new location topic

---------

Co-authored-by: Jakob Lichterfeld <jakob-lichterfeld@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:teslamate Related to TeslaMate core enhancement New feature or request kind:idea Idea for new feature or some form of enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants