-
Notifications
You must be signed in to change notification settings - Fork 768
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
Add charge_rate
to MQTT broker
#2952
Conversation
I'm switching from the Home Assistant Tesla integration to TeslaMate. I had a dashboard where I displayed the `charge_rate`. This was the only value I was missing in the mqtt topics from TeslaMate. I was wondering if there was a reason why it was not exported? It is covered by the vehicle state api call: https://github.com/adriankumpf/teslamate/blob/7a9c6b9ad1b2b03f424f46f652a53b831df73697/lib/tesla_api/vehicle/state.ex#L38 I'd love to see this added.
This is exactly what I mentioned here #2280 ! |
@adriankumpf Is there a possibility of merging this? I miss this metric since day 1 of switching from teslafi to teslamate |
@NirKli Do you think you can take a look at this? Once I get more up to date /w my understanding of the processes/elixir, maybe I can review basic stuff like this. Do you know if just adding it here is good enough or is there areas, we may need to add it to? I still have a few outstanding things I need to learn about how the whole process works :) |
❣️ |
@@ -45,7 +45,8 @@ defmodule TeslaMate.Mqtt.PubSub.VehicleSubscriber do | |||
|
|||
@always_published ~w(charge_energy_added charger_actual_current charger_phases | |||
charger_power charger_voltage scheduled_charging_start_time | |||
time_to_full_charge shift_state geofence trim_badging)a | |||
time_to_full_charge shift_state geofence trim_badging | |||
charge_rate)a |
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.
Can you add this change to the tests? https://github.com/teslamate-org/teslamate/blob/master/test/teslamate/mqtt/pubsub/vehicle_subscriber_test.exs
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.
As far as my knowledge extends, if the goal of this PR is to add a new field to MQTT that only gets updated when its value changes, then changes to the VehicleSubscriber
module might be unnecessary. The existing logic in this module already handles conditional publishing based on whether field values have changed. Therefore, just adding the new field to the Summary struct should be sufficient, provided it's not included in the @always_published list, which forces fields to be published regardless of value changes.
@@ -58,6 +58,7 @@ Vehicle data will be published to the following topics: | |||
| `teslamate/cars/$car_id/plugged_in` | true | If car is currently plugged into a charger | | |||
| `teslamate/cars/$car_id/charge_energy_added` | 5.06 | Last added energy in kWh | | |||
| `teslamate/cars/$car_id/charge_limit_soc` | 90 | Charge Limit Configured in Percentage | | |||
| `teslamate/cars/$car_id/charge_rate` | 0.0 | Charge rate in km per hour | |
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.
According to https://developer.tesla.com/docs/fleet-api?python#vehicle_data, charge_rate
could be mi/hr
or km/hr
. The field gui_settings
defines it:
Is this always km/hr and Teslamate converts it to the user's desired units, or is km/hr your experience because your GUI settings reflect that?
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.
Hi @parkr , are you sure? As for the odometer the value extracted from the API does not change based on the gui_distance_units (always in miles), I believe it only indicates what is shown on the dashboard (gui).
I did not check for tirepressure or charge-rate, but I can imagine the API delivers the basic units, and the gui is only an indication of what is shown in the car...
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.
Since this PR seemed abandoned and I want to have this feature I've redonde some of the changes in this new PR: #4130
Can you please review it there? I'm not 100% sure it's correct
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.
Hi, I'm not a user of Teslamate, so I cannot judge the PR you are proposing.
I was looking for places where charge_rate was used, and this seemed the most detailed and relevant :-)
I checked today, and with 3 phase ~13A charging (so 230x13x3 = ~8.5kW), the charge_rate indicated ~24. If assuming the car uses 0.2kWh/km (200Wh/km as indicated on the dashboard), the charge rate is 8.5 / 0.2 = 43 km/h (car indicating 40km/h, probably taking into account efficiency (~95%?). 43 km/h ~ 24.8 mi/h, which is the number indicated. This seems to confirm my previous statement that, similar to odometer, the charge_rate from the API is always provided in mi/h.
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.
Charge rate is effectively always in mph. And your discrepancy is not only efficiency, is energy not going into the battery but being used for other things. Charge rate is a very good indicator of how the energy coming from the charge port is being used:
Plugged in using 32A * 240v = 7.68kW, car and API reporting 32A 8kw. Charge rate = 0.... I'm cooling down the cabin at 40ºC to 20ºC, no energy going into the battery! Then later it reports charge rate = 5 mph, this means in my car, 1400W only going into the battery.
This metric is so important and not being used in teslamate! Teslafi uses this to calculate energy spent cooling as you can directly calculate it.
@@ -87,6 +87,7 @@ defmodule TeslaMate.Vehicles.Vehicle.Summary do | |||
charge_current_request_max: charge(vehicle, :charge_current_request_max), | |||
charge_energy_added: charge(vehicle, :charge_energy_added), | |||
charge_limit_soc: charge(vehicle, :charge_limit_soc), | |||
charge_rate: charge(vehicle, :charge_rate), |
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.
adding a test to one of the following would be good:
- test/teslamate/vehicles/vehicle_sync_test.exs
- test/teslamate/vehicles/vehicle/charging_sync_test.exs
- test/teslamate/vehicles/vehicle/charging_test.exs
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.
Fields added to the Summary
module are typically published to MQTT. Have you verified that the new field is pushed correctly in various vehicle states such as driving, charging, and idling?
@Dulanic Once tests are added, the test GitHub Actions should be run for this PR. |
@@ -45,7 +45,8 @@ defmodule TeslaMate.Mqtt.PubSub.VehicleSubscriber do | |||
|
|||
@always_published ~w(charge_energy_added charger_actual_current charger_phases | |||
charger_power charger_voltage scheduled_charging_start_time | |||
time_to_full_charge shift_state geofence trim_badging)a | |||
time_to_full_charge shift_state geofence trim_badging | |||
charge_rate)a |
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.
As far as my knowledge extends, if the goal of this PR is to add a new field to MQTT that only gets updated when its value changes, then changes to the VehicleSubscriber
module might be unnecessary. The existing logic in this module already handles conditional publishing based on whether field values have changed. Therefore, just adding the new field to the Summary struct should be sufficient, provided it's not included in the @always_published list, which forces fields to be published regardless of value changes.
Is there anything preventing this branch to be merged? |
Yes, we do not have any CI test running on this PR. Please update the branch to include latest master |
Closing, as OP did not update branch as requested on Feb 7 |
No love for this feature? |
Somebody needs to update the branch for the latest master. |
I'm switching from the Home Assistant Tesla integration to TeslaMate. I had a dashboard where I displayed the
charge_rate
. This was the only value I was missing in the mqtt topics from TeslaMate.I was wondering if there was a reason why it was not exported? It is covered by the vehicle state api call:
https://github.com/adriankumpf/teslamate/blob/7a9c6b9ad1b2b03f424f46f652a53b831df73697/lib/tesla_api/vehicle/state.ex#L38
I'd love to see this added.