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

Add battery temperature from "Smart Battery Sense" to Victron MPPT live view #1292

Merged
merged 1 commit into from
Oct 13, 2024

Conversation

SW-Niko
Copy link

@SW-Niko SW-Niko commented Sep 28, 2024

I have integrated the battery temperature provided by the “Smart Battery Sense” into the web UI. The battery temperature is only displayed if a “Smart Battery Sense” or “BVM” is present and valid data is available.
Like on the Victron app the value will be displaced in °C without decimal places
Bugfix: Converting temperature from [K] to [°C]

Test cases:

  • Display on the web UI when the "Smart Battery Sense" is present (temperature displayed)
  • Display on the web UI when the "Smart Battery Sense" is not present (temperature not displayed)
  • Display on the web UI when the "Smart Battery Sense" fails during operation. (temperature disappears after 10 – 60 seconds)

grafik

@SW-Niko SW-Niko changed the title Add battery temperature from "Smart Battery Sense" to live view Add battery temperature from "Smart Battery Sense" to Victron MPPT live view Oct 3, 2024
@schlimmchen schlimmchen self-assigned this Oct 8, 2024
@schlimmchen
Copy link
Member

Looks good, but I have two comments:

  1. Please rename "Battery temperature": This label is missing a hint that this value is coming from an attached smart battery sense and the "Battery" is redundant here, as the whole card is about "Output (Battery)". How about "Smart Battery Sense Temperature"?
  2. This change is missing publishing the value to MQTT and HomeAssistent auto-discovery. Please either implement both and add it to this changeset, or create an issue now to track that this is missing, so we can take care of it eventually.

@SW-Niko
Copy link
Author

SW-Niko commented Oct 10, 2024

I can do that.

  1. Please rename "Battery temperature":

"Smart Battery Sense Temperature" needs 2 rows on a smart phone display. How about „Temperature (SBS)“ or „SBS temperature“? I just want to avoid using more space than the currently longest text "Efficiency (calculated)"

  1. This change is missing publishing the value to MQTT and HomeAssistent auto-discovery

It seems the value to MQTT and HomeAssistent auto-discovery will be published already. 🤔

See "MqttHandleVedirect.cpp" Line 141
PUBLISH_OPT(SmartBatterySenseTemperatureMilliCelsius, "SmartBatterySenseTemperature", currentData.SmartBatterySenseTemperatureMilliCelsius.second / 1000.0);

and "MqttHandleVedirectHASS.cpp" Line 98
if (optMpptData->SmartBatterySenseTemperatureMilliCelsius.first != 0) { publishSensor("Smart Battery Sense temperature", "mdi:temperature-celsius", "SmartBatterySenseTemperature", "temperature", "measurement", "°C", *optMpptData);

@schlimmchen
Copy link
Member

It seems the value to MQTT and HomeAssistent auto-discovery will be published already. 🤔

You are absolutely right about that. I apologize for the confusion.

"Smart Battery Sense Temperature" needs 2 rows on a smart phone display.

Good point. Let's go with "SBS temperature", as you suggested. It matches "Charge controller temperature" in the "Device Info" card to the left/top.

@SW-Niko
Copy link
Author

SW-Niko commented Oct 13, 2024

Ok, done. 👍

a battery temperature value measured by a Victron smart battery sense
and communicated to a connected Victron MPPT charge controller will now
appear in the live view card.
@schlimmchen schlimmchen merged commit 844d920 into hoylabs:development Oct 13, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants