-
Notifications
You must be signed in to change notification settings - Fork 27
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
Skip endpoints that are in error state when requesting vehicle data #300
Conversation
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 |
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 |
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 |
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 |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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