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

Merge 20191117042320_add_cost_field_to_charges.exs (Charge Cost field) #258

Merged
merged 4 commits into from
Nov 24, 2019

Conversation

ngardiner
Copy link
Contributor

@ngardiner ngardiner commented Nov 16, 2019

Hi @adriankumpf,

Off the back of the feature request for adding costs to charging details, I wanted to see if I could propose a new database column for this.

I'm new to making a schema change this way so appreciate any feedback. Once I work out how it works, I'll update the development doco to explain the process.

I have modeled this on other schema changes I can see in the distribution, however I couldn't find an example of a datatype with arguments in the way that I have defined this one, and I am not sure how to make these migrations apply to my existing distribution, I have tried restarting teslamate but that doesn't seem to initiate it.

The reasoning for the datatype proposed is:

  • floating point datatypes are imprecise
  • whilst integer could be used for representing cents, it would require a conversion routine for this which might differ per currency based on localisation - not sure if that is just adding unnecessary complexity?
  • decimal(6,2) gives 6 digits in total with a 2 digit exact decimal precision. This would support a number up to 9999.99 or 999999. It is somewhat of an assumption that this will be sufficient for all currencies however it doesn't seem unreasonable to me in that currencies with high values will generally not involve decimal precision. Do you think it is sufficient, or would a larger field size (say 12,2) be a safer bet?

Once I know how best to test this, I'll test it on my distribution. Let me know if you would like me to approach this in a different way.

@ngardiner
Copy link
Contributor Author

Okay, reading the ecto documentation I see that mix datatypes cannot be defined this way. I will need to do some more reading.


def change do

alter table(:charges) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

The cost field should be added to the :charging_processes table. The naming could be better, I know :-)

use Ecto.Migration

def change do

Copy link
Collaborator

Choose a reason for hiding this comment

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

To make the formatter happy, you can run MIX_ENV=test mix format

@adriankumpf
Copy link
Collaborator

A datatype like decimal(6,2) is a good choice in my opinion. We shouldn't overthink it for now.

@adriankumpf
Copy link
Collaborator

To apply the database migration there is mix ecto.migrate

@marcogabriel
Copy link
Contributor

marcogabriel commented Nov 21, 2019

How will cost data get into that column?

I charge at different chargers. They apply different costs, some per minute, some per kwh, some are free and some use a fixed fee per charge.

As a first step, I built a custom dashboard for me that just displays the charges at home (location: home) and applies my kwh price on the gross kwh consumption of the charge (fixed multiplication in the table, not even in the database). That gives me an overview how much my car consumes at home and what fraction of my power bill is caused by my car.

My approach is simple, but just covers cost at home.

Is there any other way than entering manually the charging type (minutes, kwh, fix, free) and the location as a tuple?

Here is an example of my added dashboard with the calculated column:
image

@ngardiner
Copy link
Contributor Author

ngardiner commented Nov 22, 2019 via email

@toerb
Copy link

toerb commented Nov 22, 2019

For me just a database column with a simple cost input per charging session would be great.
Since charging costs are calculated by various ways (per minute, per kWh, per kWh and per minute in the same time, per session) I think it is the easiest to just start with a simple manual input. I would wait for the monthly bill of my providers and input the charging costs based on the data in the bill.

By simple input I mean a simple to implement input (like just a field in the database) and not a user friendly input field in the web frontend.

@ngardiner: Thanks for taking your time to contribute to this project and to this issue in particular!

@toerb
Copy link

toerb commented Nov 22, 2019

BTW, this is related to issue #185.

@ngardiner
Copy link
Contributor Author

ngardiner commented Nov 23, 2019

Huge thanks yet again @adriankumpf for the context around working with ecto/mix.

I have fixed the formatting issues and properly defined the column datatype, and have tested it in my environment:

teslamate=# \d charging_processes
 [...]
 cost                 | numeric(6,2)                |

@adriankumpf, I think the next steps will be to have a think about the data entry mechanism for this. I don't think I would be very useful at all UI wise (at this point, anyway) but I have an idea that I want to run past you, before I work on a separate PR. Here's my thoughts, what do you think?

  • This PR I think satisfies the schema update first approach, where the field then becomes available for population manually via psql
  • There would also need to be grafana dashboard updates which haven't happened yet, but I'd propose pushing that into the next PR
  • Alongside this, what would you think about an API endpoint that takes either a PUT or POST value to update cost for a given charge id? This doesn't satisfy the UI interface request in the original issue Feature request: Add costs to chargings session #185, but it does provide a programmatic interface so that external systems could update the value if they needed to (and avoids the need to run psql within the DB container to update values until a UI interface exists)
  • It might provide some reuse capability for the UI interface, as well.

@ngardiner
Copy link
Contributor Author

ngardiner commented Nov 23, 2019

In addition to the mix migration test in v1.13.0-dev, I have also just run a quick test with a charger session. The logs show:

18:34:46.765 car_id=1 [info] Charging / SOC: 73%
18:37:28.880 car_id=1 [info] Derived efficiency factor: 136.1 Wh/km (8x confirmed)
18:37:28.880 car_id=1 [info] Charging / Disconnected / 0.1 kWh – 3 min

And the charging_processes table is updated without issue, with cost being empty:

 id |         start_date         |          end_date          | charge_energy_added | start_ideal_range_km | end_ideal_range_km | start_battery_level | end_battery_level | duration_min | outside_temp_avg | car_id | position_id | address_id | start_rated_range_km | end_rated_range_km | geofence_id | charge_energy_used | cost
----+----------------------------+----------------------------+---------------------+----------------------+--------------------+---------------------+-------------------+--------------+------------------+--------+-------------+------------+----------------------+--------------------+-------------+--------------------+------
 78 | 2019-11-23 07:34:46.743167 | 2019-11-23 07:37:28.869828 |                 0.1 |                278.3 |              279.1 |                  73 |                73 |            3 |               22 |      1 |           9 |          9 |                278.3 |              279.1 |             |  0.106973177777778 |

All looks good.

@ngardiner
Copy link
Contributor Author

ngardiner commented Nov 23, 2019

Just one more update from me, I've actually managed to already set the costs for all of my charges, and I've already updated the grafana dashboard, although not as part of this PR. The one thing I did want to mention is that I think it does make sense to leave the default value for this column as null, because I have been using the following method for setting the values:

  • First, I select address_id,charge_energy_used from charging_processes where cost is null; to show all rows where the value has not been set.
  • I then cross-check address-id to see what the nature of the charger is, the following query will do this in one operation:
select charging_processes.id,address_id,charge_energy_used,display_name from charging_processes LEFT JOIN addresses on addresses.id = charging_processes.address_id where cost is null;

I then either set the cost to 0.00 if the charging is free (for me, the majority are) or to the cost if it is paid charging. If we were to default the value to 0.00, it would make it very difficult as I'd need to review every charge entry to see if it is a free charge or not.

update charging_processes set cost = '1.23' where id = '<id of charge>';

Below is a look at what it looks like in my grafana dashboard. It is accurate (even if it seems as though it isn't) because the vast majority of my charging is free destination charging currently.

image

@ngardiner
Copy link
Contributor Author

ngardiner commented Nov 23, 2019

Another thought I have is whether grafana itself can allow users to update the cost column per charging session once I have updated the charges dashboard to show it per session? I have no idea if it's an option, but if so it may save us the UI work - I am going to follow it up.

@adriankumpf
Copy link
Collaborator

[...] so I wouldn't think it would be any impediment to adding it to postgres and grafana now? It is entirely possible that these wouldn't be in scope for TeslaMate (that's entirely up to the developer) in which case it may be an external module or process of some sort which then updates the column. Looks good!

I wouldn't mind an add-on that covers the various ways to calculate charging costs at all. But in general, tracking charging costs is in scope for this project.

What's kept me from implementing it so far is, that I personally have no need for this feature. Additionally, like you and @toerb said, charging costs can be calculated in various ways (automatic cost calculation etc.) and I just don't know the specific requirements people have.

There would also need to be grafana dashboard updates which haven't happened yet, but I'd propose pushing that into the next PR

That would be a great follow-up PR!

Alongside this, what would you think about an API endpoint that takes either a PUT or POST value to update cost for a given charge id?

I think that's a good idea. It could be implemented very quickly. From my point of view, building a simple UI wouldn't be much trouble either, as long as it's only about annotating recorded charging sessions. I could do that next.

Another thought I have is whether grafana itself can allow users to update the cost column per charging session once I have updated the charges dashboard to show it per session? I have no idea if it's an option, but if so it may save us the UI work - I am going to follow it up.

I don't think that's possible with Grafana. But I'll be happy if it proves to be otherwise.

@ngardiner
Copy link
Contributor Author

ngardiner commented Nov 24, 2019

I wouldn't mind an add-on that covers the various ways to calculate charging costs at all. But in general, tracking charging costs is in scope for this project.

Perfect, I have thought some more about how this could be implemented and I'd see this as being a 3rd changeset, after the simple static charging cost code is integrated.

I have some ideas around real-world use but in general I think the majority of people will be entering this statically (sometimes calculating charging from kWh alone will be inaccurate as there may be other costs involved or charging methods). For me, the only dynamic charging calculation I'd use would be for home charging based off my grid rates (even though I have solar, making it impossible to calculate accurately, so I'd still be logging worst case costs). Everything else for me would be on the amount invoiced which is not going to be effectively automated.

It would also need to be a regularly executed routine or a dynamically generated value to be particularly useful - ie if I happen to charge after a charging rate is changed, I would want to be able to go back to the address record in TM and update the new charging rate and have it dynamically recalculated.

That would be a great follow-up PR!

Just opened a new PR for this and the API functions, and given you've answered all the questions I had here I am confident I know what to try out next.

I think that's a good idea. It could be implemented very quickly. From my point of view, building a simple UI wouldn't be much trouble either, as long as it's only about annotating recorded charging sessions. I could do that next.

Perfect, something like the geo-fence link in Grafana would be perfect I think? I will update the dashboard with a link to something that could be used for this in the other PR.

I don't think that's possible with Grafana. But I'll be happy if it proves to be otherwise.

Yep, you're 100% right - after some research I have come to the same conclusion. If only it were that easy :)

@adriankumpf
Copy link
Collaborator

Perfect, something like the geo-fence link in Grafana would be perfect I think? I will update the dashboard with a link to something that could be used for this in the other PR.

Yeah, that'd be perfect!

@adriankumpf adriankumpf merged commit a0618d3 into teslamate-org:master Nov 24, 2019
@vkarthikbalaji
Copy link

vkarthikbalaji commented Nov 26, 2019

Perfect, something like the geo-fence link in Grafana would be perfect I think?

@ngardiner , I think this is the most practical way for anyone to add the cost information.

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.

5 participants