-
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 location topic #3729
Add location topic #3729
Conversation
✅ Deploy Preview for teslamate ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Not tested yet :-) |
b0e4f15
to
6834fd5
Compare
Not sure I understand why this test is failing:
The test already has at the top: assert_receive {VehiclesMock, {:subscribe_to_summary, 0}} |
6834fd5
to
609deff
Compare
Test now works, but I don't understand why. Seems dodgy. |
609deff
to
9338e3e
Compare
Not sure how the tests were even passing before. All fixed now. |
Problem with this is that there are times when we have the latitude, longitude, but not the elevation. So as a result nothing gets published. Arrgh. Think I will remove elevation. I think the Tesla API treats elevation is very different from latitude and longitude anyway. |
68576b9
to
c958404
Compare
Seems to be working so far. latitude/longitude are being encoded as quoted string values, not floats. Which is a bit unexpected. I might try to get them as floats. |
teslamate uses Decimal type everywhere for latitude/longitude. Suspect this is a misunderstanding here. Decimal is required for applications that keep track of numbers - like $ and cents, where it is vital that we represent values like 0.01 exactly. But this is not one of those applications. It doesn't matter really if a latitude of 0.01 gets converted to 0.0099999997 (https://hexdocs.pm/elixir/1.13.4/Float.html). In fact it is likely the values came out of floating point numbers in Tesla's code anyway. But will keep it as is for now. Values are now coming out as floats. And tests appear to be passing. Early on one of the tests passed, which I hadn't even updated. That was weird. Fixed now. |
Thanks for your deditcation! |
Arggh. Looks like some parts of the code use the Decimal type, and other parts use the floating point type. I like having compile time type checking... I think I might get rid of all the Decimal values. For latitude/longitude. Just use floats everywhere.
|
Database values are stored in decimal, since 45d94d5. Supposedly this makes things more efficient. Values from the TeslaApi are retrieved at floats. I think I am will put all the Decimal types back and have this new code deal with both cases. |
f186445
to
5612ee7
Compare
We seem to have an unrelated intermittent test failure. https://github.com/teslamate-org/teslamate/actions/runs/8240221994/job/22535198738?pr=3729 |
Sorry, didn't mean to close, pushed the wrong thing. |
Testing again. But next drive not scheduled until this afternoon. |
Just noticed we always publish this every time. We should be checking if the value has changed since the last time. Will fix. |
406f9c4
to
7b60382
Compare
See #3736 concerning the test failures, All the tests passed on my box :-) |
This seems to be working correctly now. |
Test issue should be resolved by #3733. If you don't mind, update from master. Edit: issue with async run still persist on master, "solved" by rerun action. |
7b60382
to
4839e3d
Compare
Fixes #3660.
4839e3d
to
b7bcdbe
Compare
Rebased against latest master. Not 100% comfortable adding 30 extra lines to the Refactoring probably should happen if we are going to output other stuff as json blobs however. |
Had to retry CI tests three times to get a pass :-( I think they work OK from my box however. |
Will merge as soon as I came up with a good changelog for this, as it will break some third party integrations |
Shouldn't cause any breakage, the current topics remain unchanged. Unless I missed something here? |
You are absolutely right, and I did not look close enough. Will double-check and merge tomorrow. Thanks for your dedication! |
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.
lgtm, I added the doc entry for the new topic
What a hero, thanks for the efforts <3 |
@tobiasehlert I think this new topic, generate a lot of warning on teslamateapi |
@vbarrier, noticed some errors as well at home. Going to add a fix very soon 👌 |
Fixes #3660.