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

battery_level vs. usable_battery_level #321 #338

Merged
merged 11 commits into from
Jan 14, 2020
Merged

battery_level vs. usable_battery_level #321 #338

merged 11 commits into from
Jan 14, 2020

Conversation

ctraber
Copy link
Contributor

@ctraber ctraber commented Jan 3, 2020

As a first step I added usable_battery_level to positions table and summary page.

Sorry, but I'm not familiar with the Elixir yet and I'm sure there are still things missing (translation, test,?)...

Bildschirmfoto 2020-01-03 um 12 35 51

@ctraber
Copy link
Contributor Author

ctraber commented Jan 3, 2020

Tests passing on my dev env now.

Here I get:
12:49:44.528 [error] Postgrex.Protocol (#PID<0.1069.0>) disconnected: ** (DBConnection.ConnectionError) owner #PID<0.3853.0> exited

%> 100%)</td>
</tr>
<% end %>
<%= unless is_nil(@summary.usable_battery_level) do %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to hide the 'Usable State of Charge' row if usable_battery_level == battery_level? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure makes sense. There is no additional information when both are the same.
I'll also try to make a snowflake when difference is higher than 2% I think thats what the TeslaApp does.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll also try to make a snowflake when difference is higher than 2% I think thats what the TeslaApp does.

Yeah, that would be great!

@adriankumpf
Copy link
Collaborator

Looks really good! After rerunning the tests everything passes :)

@ctraber
Copy link
Contributor Author

ctraber commented Jan 3, 2020

How can you restart the test without commit? Just in case i happens again.

@adriankumpf
Copy link
Collaborator

There is a "restart build" button on the Travis page. But I'm not sure if it allows you to do that. Ideally, it shouldn't be required to rerun that often. Some tests are sensitive to timing though and therefore can be flaky.

@adriankumpf
Copy link
Collaborator

Excellent. Will merge soon!

usable_battery_level and battery_level), milage and outdoor temperature.
@ctraber
Copy link
Contributor Author

ctraber commented Jan 8, 2020

Finally it was cold enough for a snowflake. By using usable_battery_level, the projected range is not changing that much with temperature. I think it gives a better degradation overview.

Bildschirmfoto 2020-01-08 um 11 51 33

Bildschirmfoto 2020-01-08 um 12 02 38

@adriankumpf
Copy link
Collaborator

It took a little longer, but I could finally take a closer look at the PR. I made a just few adjustments:

  • Replaced the existing degradation dashboard with your 'projected range' dashboard
  • Moved the "(xxx km 100%)" text into a tooltip
  • Added a test for the snowflake icon

Thanks again! :)

@adriankumpf adriankumpf merged commit 0d46ebf into teslamate-org:master Jan 14, 2020
@adriankumpf
Copy link
Collaborator

Resolves #321

@ctraber
Copy link
Contributor Author

ctraber commented Jan 14, 2020

Will I see the tooltip in iOS Safari?
Most of the time I view it on my phone.

@adriankumpf
Copy link
Collaborator

Should work with 3904a18

@ctraber
Copy link
Contributor Author

ctraber commented Jan 15, 2020

Thanks a lot!
Just tried. Works perfectly.

@ctraber
Copy link
Contributor Author

ctraber commented Jan 18, 2020

I tested your new projected range dashboard.
In the second panel the coalesce for usable_battery_level is missing. I would add it but for some reason I do not get the dashboard, I changed in filesystem, after building a new grafana image.

Do you have an idea why it is not showing the changed dashboard after docker build and restart of container?

@adriankumpf
Copy link
Collaborator

You are referring to the following query (line 3), right?

SELECT
	$__timeGroup(date, '6h') AS time,
	convert_km(sum([[preferred_range]]_battery_range_km) / sum(usable_battery_level) * 100, '$length_unit') AS "Projected Range (using usable_battery_level) [$length_unit]",
	convert_km(sum([[preferred_range]]_battery_range_km) / sum(battery_level) * 100, '$length_unit') AS "Projected Range (using battery_level) [$length_unit]"
FROM
	positions
WHERE
	$__timeFilter(date) and
	car_id = $car_id
GROUP BY
	1
ORDER BY
	1,2  DESC

I don't see why sum(coalesce(usable_battery_level,battery_level)) should be necessary. Could you explain that?

Do you have an idea why it is not showing the changed dashboard after docker build and restart of container?

Does it show up if you add the dashboard under a new name (and with a different uid)?

@ctraber
Copy link
Contributor Author

ctraber commented Jan 18, 2020

The null values mess up the graph at the point where database starts having values for usable_battera_level. Maybe sum() with some null values, not sure.

see the spike with app. 1700 km.
Bildschirmfoto 2020-01-18 um 14 33 55

I can add new dashboards.
But, I copied yours with "save as" changed it and copied the json to the existing file, changed names... But if I re-build the container its again your previous version..
=> changing in file, build... and still the version before file change.

@adriankumpf
Copy link
Collaborator

Hmm, seems to work fine for me. Anyway, if coalesce helps, let's add it back in.

But, I copied yours with "save as" changed it and copied the json to the existing file, changed names... But if I re-build the container its again your previous version..
=> changing in file, build... and still the version before file change.

Very strange. Sounds like it takes the previous file when creating the image? Maybe try doing the following:

  1. Stop the container
  2. delete the grafana volume
  3. build the image
  4. run the new image

@ctraber
Copy link
Contributor Author

ctraber commented Jan 18, 2020

Deleting the volume did the trick.
Thanks!

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