Open GitOldGrumpy opened 10 months ago
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
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"}]}}.
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.
After modifing to
imei: Optional[str] = None
, I am receiving more information now (ServiceHistoryResponseModel
,NotificationResponseModel
,TelemetryResponseModel
,LocationResponseModel
,VehicleHealthResponseModel
), AfterTripsResponseModel
, 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
?
Attention: Patch coverage is 63.63636%
with 8 lines
in your changes missing coverage. Please review.
Project coverage is 70.09%. Comparing base (
43c8189
) to head (d7e4b2d
). Report is 31 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
mytoyota/controller.py | 12.50% | 7 Missing :warning: |
mytoyota/client.py | 50.00% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@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 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()
.
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.
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?
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.
After modifing to
imei: Optional[str] = None
, I am receiving more information now (ServiceHistoryResponseModel
,NotificationResponseModel
,TelemetryResponseModel
,LocationResponseModel
,VehicleHealthResponseModel
), AfterTripsResponseModel
, 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.
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.
My thoughts around brands:
My thoughts are credential caching:
It seems to me that it is easier to keep the API single branded which we are told, although easier isn't always correct.
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.
👍🏻
guys, i'd like to support, e.g. with testing. i have lexus rx. feel free to come back to me
Is one of you CafeAlpha on reddit?
If not, it might be interesting to know what integration he uses, because it seems to work with his Lexus: https://www.reddit.com/r/homeassistant/comments/1ejirlx/comment/lgezsr5/
Is one of you CafeAlpha on reddit?
If not, it might be interesting to know what integration he uses, because it seems to work with his Lexus:
https://www.reddit.com/r/homeassistant/comments/1ejirlx/comment/lgezsr5/
Unfortunately he must be in America as the IS500 is not available in Europe...
Unfortunately he must be in America as the IS500 is not available in Europe...
Arg, you're right. And the APIs have nothing in common, correct?
- 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.
Wouldn't it be simpler (for the developers) and clearer (for the end user) to have two integrations? If you have a car of each brand, you install both integrations...
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.
Could you add a bit of randomness? Check every 3 to 7 minutes during the day, more during the night or something?
Absolutely right the API's are different even if the features are pretty much the same
I'm gonna patiently wait until you guys have a working solution for Lexus in Europe, then
@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.