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

WIP: Lexus Work #316

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

WIP: Lexus Work #316

wants to merge 6 commits into from

Conversation

GitOldGrumpy
Copy link
Collaborator

@CM000n and @ellvisss branch for working through getting a Lexus vehicle to work.

Currently contains all fixes we have found and a fix for the first pydantic error.

If we run and post the log files on errors, unless of course you can see the fix, then fix away.

@ellvisss
Copy link

After modifing to imei: Optional[str] = None, I am receiving more information now (ServiceHistoryResponseModel, NotificationResponseModel, TelemetryResponseModel, LocationResponseModel, VehicleHealthResponseModel), After TripsResponseModel, it is showing only the last trip and right after I get this:

Traceback (most recent call last):
File "C:\Temp\mytoyota-master\simple_client_example.py", line 88, in
asyncio.run(get_information())
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 190, in run
return runner.run(main)
^^^^^^^^^^^^^^^^
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 118, in run
return self._loop.run_until_complete(task)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\base_events.py", line 653, in run_until_complete
return future.result()
^^^^^^^^^^^^^^^
File "C:\Temp\mytoyota-master\simple_client_example.py", line 79, in get_information
Trips
NameError: name 'Trips' is not defined

@ellvisss
Copy link

Now my local time is after midnight, and when I tried again, I got this error for the Trips:

mytoyota.exceptions.ToyotaApiError: Request Failed. 400, {"status":{"messages":[{"responseCode":"400 BAD_REQUEST","description":"BAD_REQUEST","detailedDescription":"Invalid to date 2024-01-25. It should be in yyyy-MM-dd format and not a future date"}]}}.

@GitOldGrumpy
Copy link
Collaborator Author

GitOldGrumpy commented Jan 24, 2024

Now my local time is after midnight, and when I tried again, I got this error for the Trips:

mytoyota.exceptions.ToyotaApiError: Request Failed. 400, {"status":{"messages":[{"responseCode":"400 BAD_REQUEST","description":"BAD_REQUEST","detailedDescription":"Invalid to date 2024-01-25. It should be in yyyy-MM-dd format and not a future date"}]}}.

I think I understand this one, probably something to do with different time zones and not being able to request a "future" date. Will need looking into.

@GitOldGrumpy
Copy link
Collaborator Author

After modifing to imei: Optional[str] = None, I am receiving more information now (ServiceHistoryResponseModel, NotificationResponseModel, TelemetryResponseModel, LocationResponseModel, VehicleHealthResponseModel), After TripsResponseModel, it is showing only the last trip and right after I get this:

Traceback (most recent call last):
File "C:\Temp\mytoyota-master\simple_client_example.py", line 88, in
asyncio.run(get_information())
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 190, in run
return runner.run(main)
^^^^^^^^^^^^^^^^
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 118, in run
return self._loop.run_until_complete(task)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\base_events.py", line 653, in run_until_complete
return future.result()
^^^^^^^^^^^^^^^
File "C:\Temp\mytoyota-master\simple_client_example.py", line 79, in get_information
Trips
NameError: name 'Trips' is not defined

This seems odd. Have you un-commented any lines. Line 79 is a comment line that should be # Trips?

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (43c8189) 70.35% compared to head (d7e4b2d) 70.09%.

Files Patch % Lines
mytoyota/controller.py 12.50% 7 Missing ⚠️
mytoyota/client.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
- Coverage   70.35%   70.09%   -0.26%     
==========================================
  Files          32       32              
  Lines        1511     1518       +7     
==========================================
+ Hits         1063     1064       +1     
- Misses        448      454       +6     
Flag Coverage Δ
unittests 70.09% <63.63%> (-0.26%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GitOldGrumpy
Copy link
Collaborator Author

@CM000n what's the thinking for adding the brand to each of the endpoint calls? I don't think it can change once you are connected as the vehicles you get back will only be of the specific brand. Is this not something we could put on the initialiser of the API class?

@CM000n
Copy link
Collaborator

CM000n commented Jan 25, 2024

@CM000n what's the thinking for adding the brand to each of the endpoint calls? I don't think it can change once you are connected as the vehicles you get back will only be of the specific brand. Is this not something we could put on the initialiser of the API class?

Good point @GitOldGrumpy. I thought this was also required in the headers of the respective API calls? If we really only need it for the initial call of the vehicle, the whole thing could of course be simplified.
But I think we should at least parameterise it so that the desired brand can be passed when calling get_vehicles().

@GitOldGrumpy
Copy link
Collaborator Author

Its needed for all calls to any endpoint but only needs to be set the once so it could be set when the API class is constructed as opposed to on each API call? This simplifies all the endpoint calls.

There is one downside if a user has both Toyota's and Lexi then they will need too construct two API classes one for each brand. This will work as the first will login and the second will use the cached credentials. I think this is still preferable to having to specifying the brand in each API endpoint call as for all but this case you will always being passing the same value.

@CM000n
Copy link
Collaborator

CM000n commented Jan 25, 2024

Its needed for all calls to any endpoint but only needs to be set the once so it could be set when the API class is constructed as opposed to on each API call? This simplifies all the endpoint calls.

There is one downside if a user has both Toyota's and Lexi then they will need too construct two API classes one for each brand. This will work as the first will login and the second will use the cached credentials. I think this is still preferable to having to specifying the brand in each API endpoint call as for all but this case you will always being passing the same value.

That gives me an idea. The current way of caching (we store it as toyota_credentials_cache_contains_secrets file in the home directory) could possibly become a problem if the usere uses different accesses for his Toyota and Lexus vehicles, right?

@GitOldGrumpy
Copy link
Collaborator Author

Its needed for all calls to any endpoint but only needs to be set the once so it could be set when the API class is constructed as opposed to on each API call? This simplifies all the endpoint calls.
There is one downside if a user has both Toyota's and Lexi then they will need too construct two API classes one for each brand. This will work as the first will login and the second will use the cached credentials. I think this is still preferable to having to specifying the brand in each API endpoint call as for all but this case you will always being passing the same value.

That gives me an idea. The current way of caching (we store it as toyota_credentials_cache_contains_secrets file in the home directory) could possibly become a problem if the usere uses different accesses for his Toyota and Lexus vehicles, right?

Yes you're correct if they have different email addresses across the apps then they will get different tokens. In my test case I was using the same details across both.

@ellvisss
Copy link

After modifing to imei: Optional[str] = None, I am receiving more information now (ServiceHistoryResponseModel, NotificationResponseModel, TelemetryResponseModel, LocationResponseModel, VehicleHealthResponseModel), After TripsResponseModel, it is showing only the last trip and right after I get this:

Traceback (most recent call last):
File "C:\Temp\mytoyota-master\simple_client_example.py", line 88, in
asyncio.run(get_information())
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 190, in run
return runner.run(main)
^^^^^^^^^^^^^^^^
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 118, in run
return self._loop.run_until_complete(task)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\UserName\AppData\Local\Programs\Python\Python311\Lib\asyncio\base_events.py", line 653, in run_until_complete
return future.result()
^^^^^^^^^^^^^^^
File "C:\Temp\mytoyota-master\simple_client_example.py", line 79, in get_information
Trips
NameError: name 'Trips' is not defined

This seems odd. Have you un-commented any lines. Line 79 is a comment line that should be # Trips?

Yes, sorry, I think I un-commented it by mistake. Now it works with no error.

@CM000n
Copy link
Collaborator

CM000n commented Jan 27, 2024

Yes you're correct if they have different email addresses across the apps then they will get different tokens. In my test case I was using the same details across both.

I'm really excited about how we want to design it. 😊 Maybe we should write it down/record it?

My idea would be that a user does not have to specify the brand, but we use the get_vehicles_endpoint(self) endpoint to determine the vehicles for both Toyota and Lexus and combine them in the returned model first. We then decide which endpoint headers to use for further requests, depending on the brand present in the VehiclesResponseModel.

Do you already have an idea of how we want to deal with multi-accounting and caching? In the issues of the Home Assistant integration there are also issues from people who have authorization problems when the lib tries to create a cache folder + file in the home directory of the process user.

@GitOldGrumpy
Copy link
Collaborator Author

My thoughts around brands:

  • Do we want to combine them? It seems that users with both Lexi & Toyota's will be a fairly niche use case. Is it worth the extra complications to support given users can easily create two API instances? (Once we sort the issue with cache file). How does the effect home assistant? I think at the moment it only supports 1 car?
  • For the user to not specify the brand we would have to hit the endpoint twice, first with the brand set to "T" then to "L" the apps would never do this I have a small concern this could get us noticed as we are using the API not how its intended. The other issue is for users with only one brand we are always making the call twice even though its not required.
  • For users that do have both brands how do we deal with the username/password when they are different? They will need to pass us both with the brand information, which sort of negates the auto determination? (We could still make it work without the brand information but it would mean more calls to the get_vehicle_endpoint.)

My thoughts are credential caching:

  • It would appear we need to cache per brand, though we don't know the brand(unless told) until after the call to get_vehicle_endpoint.
  • On the HA issue its not bad if it fails to cache as the API is only constructed the once? If this is the case we could have a flag for DONT cache or just log if the cache file could not be created.
  • Does HA have a cache location for such things? Can we write into the custom_component/??? folder. We could always cache to /tmp?

It seems to me that it is easier to keep the API single branded which we are told, although easier isn't always correct.

@CM000n
Copy link
Collaborator

CM000n commented Jan 27, 2024

My thoughts around brands:

  • Do we want to combine them? It seems that users with both Lexi & Toyota's will be a fairly niche use case. Is it worth the extra complications to support given users can easily create two API instances? (Once we sort the issue with cache file). How does the effect home assistant? I think at the moment it only supports 1 car?

You're right, it's usually better to make the information explicit rather than implicit. After thinking about it again I think most people would expect the same behaviour as when using two different apps. With regard to Home Assitant, it should already be possible to specify several accounts. For each account, all associated vehicles should then actually be created as a device with their respective entities (sensors). Unfortunately, I haven't been able to test it yet, as I only have one vehicle myself and writing tests for Home Assistant custom components is a PITA

To be honest, I don't quite understand how you want to use the specified brand once when constructing the API class, instead of adding it to the endpoint calls. But I'm happy to be persuaded. 😊

  • For the user to not specify the brand we would have to hit the endpoint twice, first with the brand set to "T" then to "L" the apps would never do this I have a small concern this could get us noticed as we are using the API not how its intended. The other issue is for users with only one brand we are always making the call twice even though its not required.

I don't think that makes any difference or makes the situation worse. 😉 If anyone at Toyota takes a closer look, we are already conspicuous. Currently, the Home Assistant integration queries the API every 5 minutes. I don't think the apps do that with such regularity.

  • For users that do have both brands how do we deal with the username/password when they are different? They will need to pass us both with the brand information, which sort of negates the auto determination? (We could still make it work without the brand information but it would mean more calls to the get_vehicle_endpoint.)

As written under point 1, you are probably right. People would expect similar behaviour here as with different apps.

My thoughts are credential caching:

  • It would appear we need to cache per brand, though we don't know the brand(unless told) until after the call to get_vehicle_endpoint.

Wouldn't it be easiest to create a cache file with a name based on the user's email address? This would provide a clear distinction. Or is it possible to log in to Lexus and Toyota with the same e-mail address and then get different accounts?

  • On the HA issue its not bad if it fails to cache as the API is only constructed the once? If this is the case we could have a flag for DONT cache or just log if the cache file could not be created.

That would be a possible emergency solution

  • Does HA have a cache location for such things? Can we write into the custom_component/??? folder. We could always cache to /tmp?

To be honest, I don't know. The diverse installation options of HA make it difficult to determine how the respective user system is currently behaving.

It seems to me that it is easier to keep the API single branded which we are told, although easier isn't always correct.

👍🏻

@nrpetonr
Copy link

nrpetonr commented Apr 21, 2024

guys, i'd like to support, e.g. with testing. i have lexus rx. feel free to come back to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants