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

Skip endpoints that are in error state when requesting vehicle data #300

Merged
merged 3 commits into from
Jan 10, 2025

Conversation

prvakt
Copy link
Collaborator

@prvakt prvakt commented Dec 27, 2024

seems like some cars are having problems requesting some capabilities even supported by car (for example citigoe iv and departure timers). In those cases, users are not able to get any data because there is an error from specific endpoint during every update which makes the integration unusable..

We should ignore such errors and continue with data which is available

@WebSpider
Copy link
Contributor

I dont agree with this approach.

IMHO the library should be as agnostic as possible. Any implementation specific problems, such as specific platforms (HA) or specific cars (Citigoe) should be handled inside the implementation

@prvakt
Copy link
Collaborator Author

prvakt commented Dec 27, 2024

yeah but this can happen for any other car as well, in case any of the responses will fail for whatever reason, nothing will be updated during periodic update which is also not right approach.. And in case something is failing right in the beginning, you will be not able to configure your car at all, no sensors will be loaded because of error in a single endpoint ..

There is a special "citigo" case where we are not asking for some capabilities, but not for the first time when we connect and configure the integration.. and because citigo does have the departure timer capability, but responding with error 500, initial connect is failing and resulting in no sensors loaded for those cars.. this can also happen for any other car with same result

@WebSpider
Copy link
Contributor

I would expect the library to throw an exception then, and the implementation can handle this gracefully or ungracefully, depending on the case and requirements

@prvakt
Copy link
Collaborator Author

prvakt commented Dec 27, 2024

I would expect the library to throw an exception then, and the implementation can handle this gracefully or ungracefully, depending on the case and requirements

but that would require rewriting the entire polling logic inside HA integration because we currently rely only on this single method from myskoda lib during vehicle configuration and also during periodical update.. as I don't have time for such a change in following weeks, feel free to take over or close this one

@WebSpider
Copy link
Contributor

I would expect the library to throw an exception then, and the implementation can handle this gracefully or ungracefully, depending on the case and requirements

but that would require rewriting the entire polling logic inside HA integration because we currently rely only on this single method from myskoda lib during vehicle configuration and also during periodical update.. as I don't have time for such a change in following weeks, feel free to take over or close this one

I'm working on splitting initialization from regular polling in skodaconnect/homeassistant-myskoda#447

@WebSpider
Copy link
Contributor

Do we still need this PR?

@prvakt
Copy link
Collaborator Author

prvakt commented Jan 8, 2025

Do we still need this PR?

it depends if we really want to throw away all responses in case a single endpoint will fail or not. Yesterday there was some problem with air-conditioning endpoint (returning 500 from time to time) on my Superb and because of that all my sensors went to unavailable until next refresh .. it happened 3 or 4 times.. with this PR, only AC data would be unavailable but the rest of the sensors would be present.

Or then we can implement this in HA integration to request capabilities one by one and trace error in case one of the endpoint will fail, but in the end, it will be the very same approach as in this PR

@WebSpider
Copy link
Contributor

I made the wrong assumption here, looking at the title and description of the PR, versus its actual content. I've just checked the code, and this is absolutely fine.

Apologies for the delay!

@WebSpider WebSpider added the bug Something isn't working label Jan 10, 2025
@WebSpider WebSpider changed the title ignore errors when requesting data from vehicle Skip endpoints that are in error state when requesting vehicle data Jan 10, 2025
Copy link
Contributor

@WebSpider WebSpider left a comment

Choose a reason for hiding this comment

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

lgtm

@prvakt prvakt merged commit e79e424 into skodaconnect:main Jan 10, 2025
1 of 2 checks passed
@prvakt prvakt deleted the ignore-error-from-get-vehicle branch January 10, 2025 10:19
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.

2 participants