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

rework authentication and API structure after VW has switched off CarNet #216

Closed
wants to merge 28 commits into from

Conversation

oliverrahner
Copy link
Collaborator

No description provided.

@robinostlund
Copy link
Owner

@oliverrahner i let you decide when you feel this is ready for release, i believe it may be better to release it once when it is sort of working as i guess people will find missing things as there are different values depending on which type of car you have 😄

@den4uk
Copy link

den4uk commented Nov 28, 2023

@oliverrahner i let you decide when you feel this is ready for release, i believe it may be better to release it once when it is sort of working as i guess people will find missing things as there are different values depending on which type of car you have 😄

Could you do a pre-release, so it would be possible to install it via HACS, and tested by wider audiences?

@oliverrahner
Copy link
Collaborator Author

Thanks @robinostlund
I currently have access to a diesel, a hybrid and an electric car, and I’m also trying to cater for some things like missing glass roof and the like.
I tend to go for one release once I have „all“ entities readable, and another one for actually controlling settings.

@oliverrahner
Copy link
Collaborator Author

@oliverrahner i let you decide when you feel this is ready for release, i believe it may be better to release it once when it is sort of working as i guess people will find missing things as there are different values depending on which type of car you have 😄

Could you do a pre-release, so it would be possible to install it via HACS, and tested by wider audiences?

Unfortunately, it’s not the integration I’m reworking, but the underlying library.
I don’t have a creative idea right now how we could make these „alphas“ available for preview… do you, @robinostlund?

@den4uk
Copy link

den4uk commented Nov 28, 2023

@oliverrahner i let you decide when you feel this is ready for release, i believe it may be better to release it once when it is sort of working as i guess people will find missing things as there are different values depending on which type of car you have 😄

Could you do a pre-release, so it would be possible to install it via HACS, and tested by wider audiences?

Unfortunately, it’s not the integration I’m reworking, but the underlying library. I don’t have a creative idea right now how we could make these „alphas“ available for preview… do you, @robinostlund?

The owner (@robinostlund) could take a tag of your branch, and create a pre-release. I would test it on my ID.3 tonight.

@TheBlackBGD
Copy link

I would test it also.

@LucTs
Copy link

LucTs commented Nov 28, 2023

Mee too, I do have a 2022 Touran.

@robinostlund
Copy link
Owner

robinostlund commented Nov 28, 2023

Is it ok if i do it tomorrow morning? Dont have any computer infront of me now 😊

@oliverrahner i think you should have access to do this also now when you are a collaborator 😊

@TheBlackBGD
Copy link

Let us know when we can update from hacs ill do it roght away, for me is also ok for tomorrow as i see @oliverrahner used all my apicalls and the vw application is frozen 😂😂😂
No worries :)

@virtualdj
Copy link

I don’t have a creative idea right now how we could make these „alphas“ available for preview…

Actually if one runs the Python code in the example with the library saved as a subfolder ("download as ZIP") and with the correct login information, we/you could get information about the sensors that are working or not.

It is not the easy of use of Home Assistant sensors, but for debugging a lot of configurations it could be a viable solution.

@oliverrahner
Copy link
Collaborator Author

Let us know when we can update from hacs ill do it roght away, for me is also ok for tomorrow as i see @oliverrahner used all my apicalls and the vw application is frozen 😂😂😂 No worries :)

That is very strange... I tested only very little with your car and mostly with mine and the one from @michiii1337, and ours still work... But I can confirm that I also get HTTP 429s from the VW API for your credentials.

We need to monitor that, because I have set up all our 3 cars in my Dev HA environment, so it might be some case that produces more API calls for electric cars than for others.

@TheBlackBGD
Copy link

Let us know when we can update from hacs ill do it roght away, for me is also ok for tomorrow as i see @oliverrahner used all my apicalls and the vw application is frozen 😂😂😂 No worries :)

That is very strange... I tested only very little with your car and mostly with mine and the one from @michiii1337, and ours still work... But I can confirm that I also get HTTP 429s from the VW API for your credentials.

We need to monitor that, because I have set up all our 3 cars in my Dev HA environment, so it might be some case that produces more API calls for electric cars than for others.

@oliverrahner it is possible i dont know i use the app also almoast every time i go somewhere to heat the car, and i have also the other integration installed. But to have some ideeas i will delete the other integration and see from tomorrow what is hapening, ok?

@oliverrahner
Copy link
Collaborator Author

@oliverrahner it is possible i dont know i use the app also almoast every time i go somewhere to heat the car, and i have also the other integration installed. But to have some ideeas i will delete the other integration and see from tomorrow what is hapening, ok?

Sounds good! I also removed it now from my Dev instances, so we can investigate better.

@TheBlackBGD
Copy link

@oliverrahner it is possible i dont know i use the app also almoast every time i go somewhere to heat the car, and i have also the other integration installed. But to have some ideeas i will delete the other integration and see from tomorrow what is hapening, ok?

Sounds good! I also removed it now from my Dev instances, so we can investigate better.

@oliverrahner I have deleted the other integration. As it happened last time after midnight it worked again, so from now it will be my mobile app and you for the api calls. If i can help with something let me know!

Comment on lines 206 to 208
data = await self._connection.getParkingPosition(self.vin)
if data:
self._states.update(data)
Copy link

Choose a reason for hiding this comment

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

Suggested change
data = await self._connection.getParkingPosition(self.vin)
if data:
self._states.update(data)
if data := await self._connection.getParkingPosition(self.vin):
self._states.update(data)

Just a perfect use for a walrus operator 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same argument again :) I tried to copy the original style as much as possible, and that's how it was done before.
My perspective on this is: rather have uniform code than optimizing every little detail once you stumble over it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, resolved the wrong convo

@stickpin
Copy link
Collaborator

stickpin commented Dec 5, 2023

Interestingly, I am calling parkingposition all day long every 5 minutes, and still have not reached the rate limit.

@stickpin Ok now this is really interesting... have you tried spamming the API with the script I posted above? Because that API was the only one where I was able to artificially hit the rate limit.

No, I am using the integration which is running 4 API's every 5 minutes + Login flow or Refresh token flow every hour.

@oliverrahner
Copy link
Collaborator Author

Gets confusing now :) I'll repost my comments in the PR, where they belong and remove them here.

Regarding the rate limit: Could you try to spam the parking position API? I'll update the script in a second. Because I would really like to know if you can also reach the limit and at how many requests.

@oliverrahner
Copy link
Collaborator Author

@stickpin
Copy link
Collaborator

stickpin commented Dec 5, 2023

I updated the Gist: https://gist.github.com/oliverrahner/4c9b616b1f934ac7df9ec423b78723dc

The problem with this script is that is hammering the server every second. That might be the reason why you are getting rate-limited. I am not saying that there is no rate limit, I am just saying that there might be a combination of both amount of API requests and how frequently you do it. At least this is what I would do on WAF. :)

@oliverrahner
Copy link
Collaborator Author

Now the change I intended is there, 6d12a41... Difficult decision if we should leave it in or not. It doesn't seem to make a difference for you, but I'm also look at all the other people complaining about being rate limited 🤔

I agree that the actual rate and not only the pure number of requests should make a difference, but then it doesn't explain why the API doesn't bother at all about the other endpoints being hammered in the same way, but the do limit the parking position one...

@oliverrahner
Copy link
Collaborator Author

Right now I am at 820 requests to parkingposition in a row, with some occasional 403s in between, but that happens on all services and recovers after a short time... so now I can't even reproduce hitting the limit with that 1s interval 😬

@stickpin
Copy link
Collaborator

stickpin commented Dec 5, 2023

Right now I am at 820 requests to parkingposition in a row, with some occasional 403s in between, but that happens on all services and recovers after a short time... so now I can't even reproduce hitting the limit with that 1s interval 😬

it might also be that VW moved to Prod and is still fine-tuning the configs.
I guess I will move my setup from dev to prod in will run with the get_parkingposition() with default 5-minute intervals, let's see if I will hit the rate limit.

@oliverrahner oliverrahner force-pushed the rework-api-after-carnet branch from 6d12a41 to 8fedaa8 Compare December 5, 2023 21:59
@virtualdj
Copy link

Difficult decision if we should leave it in or not. It doesn't seem to make a difference for you, but I'm also look at all the other people complaining about being rate limited 🤔

@oliverrahner
Isn't it possible to pass a flag to the library to instruct it whether it should poll vehicle_moving or not?

If a user has the vehicle_moving entity added and enabled in HA, then poll. Otherwise, use the other algorithm based on the last trip to produce less API calls.

In this way both kind of users will be happy...

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

@oliverrahner I don't know if you paid attention but currently if VW servers go down, all the sensors are still available, and it gives you the wrong impression that everything is fine. We can introduce the connection status sensor that will show the service status. I have such a sensor for my Bosch/Siemens home appliances. OR we can drop all the sensors in an unavailable state which will indicate the problem. OR maybe even both. I think I like the last option more, connection status sensor + all the sensors in an unavailable state.
Screenshot 2023-12-08 at 11 19 42

I will work on the implementation of both. Let me know if you see it differently.

@oliverrahner
Copy link
Collaborator Author

I liked to proposal.
What do you say, you start working on that status stuff and in the meantime I can look at restoring the control functions?

@oliverrahner
Copy link
Collaborator Author

If a user has the vehicle_moving entity added and enabled in HA, then poll. Otherwise, use the other algorithm based on the last trip to produce less API calls.

In this way both kind of users will be happy...

@virtualdj Just noticed I didn't answer. Right now, we hope that @stickpin's fix really solves the problem in total, so that the users don't need to make that choice.

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

I liked to proposal. What do you say, you start working on that status stuff and in the meantime I can look at restoring the control functions?

yes, sounds good. :)

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

@oliverrahner well...
There is an extra logic to persist the previous state in the frontend integration:

    def async_write_ha_state(self) -> None:
        """Write state to HA, but only if needed."""
        backend_refresh_time = self.instrument.last_refresh
        # Get the previous state from the state machine if found
        prev: Optional[State] = self.hass.states.get(self.entity_id)

        # This is not the best place to handle this, but... :shrug:..
        if self.attribute == "requests_remaining" and self.state in [-1, STATE_UNAVAILABLE, STATE_UNKNOWN]:
            restored = prev or self.restored_state
            if restored is not None:
                try:
                    value = int(restored.state)
                    _LOGGER.debug(f"Restoring requests remaining to '{restored.state}'")
                    self.vehicle.requests_remaining = value
                except ValueError:
                    pass
            else:
                _LOGGER.debug(f"Not changing requests remaining to '{self.state}'")
                return

        # need to persist state if:
        # - there is no previous state
        # - there is no information about when the last backend update was done
        # - the state has changed (need to do string conversion here, as "new" state might be numeric,
        #   but the stored one is a string... sigh)
        if (
            prev is None
            or str(prev.attributes.get("last_updated", None)) != str(backend_refresh_time)
            or str(self.state or STATE_UNKNOWN) != str(prev.state)
        ):
            super().async_write_ha_state()
        else:
            _LOGGER.debug(f"{self.name}: state not changed ('{prev.state}' == '{self.state}'), skipping update.")

Then I guess I will keep it this way and introduce only a connection status sensor for the time being.

@robinostlund what was the initial idea to keep the previous state of the sensors if VW servers are down?

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

@oliverrahner the funny part is that every VW endpoint is acting differently.

Now for example:

  • authorize - HTTP 200
  • token - HTTP 200
  • vehicles - HTTP 200
  • parkingposition - HTTP 502
  • capabilities - HTTP 504

Which connection status we should choose? :))

Or maybe create some string-based sensor and have 3 statuses? "Up", "Down", "Warning" with attributes that will show each endpoint status?

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

Attributes are a bad idea. But to create a binary_sensor for each endpoint is also not very elegant.
But this solution might show a real status on the platform.

I guess I will start moving in this direction.

@oliverrahner
Copy link
Collaborator Author

That's also why I had a hard time reacting to the request for "last update" time that some people mentioned... there is not a specific time for everything, but rather a specific time per endpoint, sometimes even below that... I don't have a clear answer to this yet

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

@oliverrahner wow. now I managed to get a response from selectivestatus like this:
HTTP Status 207
Screenshot 2023-12-08 at 17 37 12

...
    "oilLevel": {
        "oilLevelStatus": {
            "error": {
                "message": "Bad Gateway",
                "errorTimeStamp": "2023-12-08T16:32:42Z",
                "info": "Something went wrong. Please try to re-login. If the problem persists, please contact our support.",
                "code": 4004,
                "group": 3,
                "retry": true
            }
        }
    },
    "measurements": {
        "rangeStatus": {
            "value": {
                "carCapturedTimestamp": "2023-12-08T14:58:25Z",
                "gasolineRange": 520,
                "totalRange_km": 520
            }
        },
        "odometerStatus": {
            "value": {
                "carCapturedTimestamp": "2023-12-08T14:58:25Z",
                "odometer": 33922
            }
        },
...

hmmm... one more use case to consider... :(

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

Screenshot 2023-12-08 at 19 52 04

@oliverrahner what do you say?

The current logic is:

        if response_code == 200 or response_code == 204:
            status = "Up"
        elif response_code == 207:
            status = "Warning"
        elif response_code == 429:
            status = "Rate limited"
        else:
            status = "Down"

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

@oliverrahner please check: #228

I think we can merge it into the next stable branch as well.
So in case people complain, they will be able to see the status of the APIs by themselves. :)

@stickpin
Copy link
Collaborator

stickpin commented Dec 8, 2023

@oliverrahner wow. now I managed to get a response from selectivestatus like this: HTTP Status 207 Screenshot 2023-12-08 at 17 37 12

...
    "oilLevel": {
        "oilLevelStatus": {
            "error": {
                "message": "Bad Gateway",
                "errorTimeStamp": "2023-12-08T16:32:42Z",
                "info": "Something went wrong. Please try to re-login. If the problem persists, please contact our support.",
                "code": 4004,
                "group": 3,
                "retry": true
            }
        }
    },
    "measurements": {
        "rangeStatus": {
            "value": {
                "carCapturedTimestamp": "2023-12-08T14:58:25Z",
                "gasolineRange": 520,
                "totalRange_km": 520
            }
        },
        "odometerStatus": {
            "value": {
                "carCapturedTimestamp": "2023-12-08T14:58:25Z",
                "odometer": 33922
            }
        },
...

hmmm... one more use case to consider... :(

@oliverrahner Ignore it... this API always returns 207. I will update API status codes.

@robinostlund
Copy link
Owner

@oliverrahner should we close this PR?

@oliverrahner
Copy link
Collaborator Author

All relevant work of this PR has been merged as part of #226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants