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

Initial support for Toyota Supra #565

Merged
merged 8 commits into from
Jul 6, 2024

Conversation

vanshg
Copy link
Contributor

@vanshg vanshg commented Sep 14, 2023

Proposed change

This is a first attempt to add support for Toyota Supra, for which the Supra Connect service uses the same underlying APIs.

Type of change

  • New feature (which adds functionality to this library)

Additional information

The API expects the value toyota in the request, however, it returns DRITTKUNDE as the response (German for "third party vendor"). To address this, the fallback parsing was updated to default to CarBrand.Toyota if DRITTKUNDE is returned. However, for all I know there may be other brands that also use DRITTKUNDE...(Rolls Royce infotainments are also iDrive based)

I have not yet had a chance to test this. Here is the fingerprint:
toyota_supra_fingerprint.zip

Checklist

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works.

@rikroe
Copy link
Member

rikroe commented Sep 14, 2023

Nice, thanks! Didn't know that there are even more cars and brands using NBT systems. Is it only the Supra? And only in the US? Or also globally/other cars?

Regarding the CarBrands issue: I would rather only have TOYOTA and DRITTKUNDE as valid options and no parsing from DRITTKUNDE to TOYOTA in the enum.
When initializing a vehicle, the brand could be added as additional parameter. Then we could check in MyBMWVehicle.brand property if the value in JSON is DRITTKUNDE and replace it with the init parameter?

@rikroe rikroe self-assigned this Sep 14, 2023
@rikroe rikroe added the enhancement 🆕 New feature or request label Sep 14, 2023
@vanshg
Copy link
Contributor Author

vanshg commented Sep 14, 2023

For Toyota, yeah only the Supra. For other brands it's only Rolls Royce as far as I can tell (see the last paragraph in the intro here: https://en.wikipedia.org/wiki/BMW_iDrive). The Supra is available in at least Europe but no clue about China.

I would rather only have TOYOTA and DRITTKUNDE as valid options and no parsing from DRITTKUNDE to TOYOTA in the enum

The problem with this is that drittkunde is not a valid value in API requests. So e.g. if we're iterating through all CarBrands, where DRITTKUNDE has been included as a value, the API will return a 400 Bad Request.

One approach might still be to include DRITTKUNDE as an enum, but explicitly filter it out when iterating over all brands -- maybe a CarBrands.validBrands() helper?

@rikroe
Copy link
Member

rikroe commented Sep 15, 2023

Interesting!

I could imagine something like this:

    @classmethod
    @property
    def api_brands(cls) -> list[str]:
        """Return members for API calls."""
        return (cls._member_map_[name] for name in cls._member_names_ if name != "DRITTKUNDE")

@vanshg
Copy link
Contributor Author

vanshg commented Nov 13, 2023

IMO, I think that implementation would become a footgun, as it would break the normal/expected Enum iteration (i.e. you would have to know to do for brand in CarBrands.api_brands when making requests as opposed to for brand in CarBrands)

@vanshg
Copy link
Contributor Author

vanshg commented Nov 28, 2023

@rikroe Thoughts?

@rikroe
Copy link
Member

rikroe commented Nov 30, 2023

I see your point. But to be honest, I also have no other idea.
Could you please rebase so that test are green? Then we should be able to merge

@vanshg
Copy link
Contributor Author

vanshg commented Dec 15, 2023

Sorry for the delay @rikroe. Updated

@rikroe
Copy link
Member

rikroe commented Feb 11, 2024

With #591 merged, could you please have another look? The URLs and return values do differ for the vehicle list, and I'm especially curious if there is any change regarding the returned brands.

Also, I have updated the tests so it is hopefully easier to understand where to adjust them.

@sslobodyan98
Copy link

sslobodyan98 commented Mar 10, 2024

@rikroe @vanshg Hey everyone, I was looking for an API to interact with my Supra and stumbled upon this over some Home Assistant forums. For my use case I just want a local project to mess around and don't need all the bulk of HA. I installed this package and tried to use the CLI tool to login, there are no obvious errors but it returns vehicles as 0 so assume it can't find any cars on my profile. Any ideas?

Update: after running bimmerconnected vehiclefinder
I got another failure, reran: bimmerconnected status
And got back:
Unable to update charging_profile - (TypeError) argument of type 'NoneType' is not iterable
Found 1 vehicles: SPX 40i
Along with all the JSON associated with the car, but mileage isn't accurate as of yet. Will continue to explore it's capabilities :)

Copy link

github-actions bot commented Jun 9, 2024

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale 🤖 Used by Probot label Jun 9, 2024
@rikroe
Copy link
Member

rikroe commented Jun 9, 2024

The charging_profile has been fixed with #615.
I'll see if I find some time to update this PR and get it merged (as it is mostly issues in the tests).

@rikroe rikroe removed the stale 🤖 Used by Probot label Jun 9, 2024
@rikroe
Copy link
Member

rikroe commented Jun 30, 2024

I had a look into this and wanted to add some tests.

This lead to a question regarding your fingerprint @vanshg: Do you disable sending vehicle data automatically to you the API?
Your fingerprint does not show any information (like open windows or lock state of the doors) which should be possible according to Toyota's website.

If yes, could you please run a new fingerprint with this enabled?

@vanshg
Copy link
Contributor Author

vanshg commented Jun 30, 2024

I don't have anything disabled. When I check the Supra Connect app, I only have the ability to perform actions (lock, unlock, horn, lights, ventilation) -- Besides location, I don't see anything that indicates current vehicle state (door locks or windows)

@rikroe
Copy link
Member

rikroe commented Jul 1, 2024

Ah, so BMW gives you the same level of access as their pre-2015 cars...

As I've adjusted some things (however mainly for the tests), could you please test the latest version of this branch with your car to verify everything still works?

@vanshg
Copy link
Contributor Author

vanshg commented Jul 1, 2024

Yep, I can confirm this works. I am able to run various subcommands successfully (I tried status, vehiclefinder, and horn)

@rikroe rikroe requested a review from gerard33 July 1, 2024 17:33
Copy link
Member

@gerard33 gerard33 left a comment

Choose a reason for hiding this comment

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

LGTM

@rikroe rikroe merged commit 94efbde into bimmerconnected:master Jul 6, 2024
11 checks passed
@vanshg vanshg deleted the toyota-supra-support branch July 8, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🆕 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants