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

fix(db): Update ecu state after reconnect #220

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(db): Update ecu state after reconnect #220

wants to merge 2 commits into from

Conversation

peckto
Copy link
Contributor

@peckto peckto commented Jul 15, 2022

After reconnect, the ECU might have lost its state (eg. session).
This PR updates the state after the reconnect.

@peckto peckto added the bug Something isn't working label Jul 15, 2022
@peckto peckto requested a review from fkglr as a code owner July 15, 2022 07:08
try:
await self.read_session()
except UDSException as e:
self.logger.log_warning(f'Could not read current session as part of refresh_state: {e}')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we reset the state in this case?

@peckto
Copy link
Contributor Author

peckto commented Jul 18, 2022

The error handling is not so trivial for this case.
We should keep in mind, that we have not only the state logged into the db (which is stored in the ecu), but also the internal state of the scanner, which is implicit.
If we change the state in the ecu, without notifying the scanner, we are in trouble.
For this reason, I think we should not change the state from inside the reconnect function.
Only the scanner itself should change the state.
This means, that if the state has changed because of the reconnect, we need to propagate the error back to the scanner.
The scanner should thus expect the reconnect to fail, not only because of a connection loss, but also due to a change in the ECU state.

Unfortunately, this can cause quite deep try-except blocks:

try:
    ecu.do_something()
except Error:
    try:
        ecu.reconnect()
    except ConnectionError:
        ecu.power_cycle()
        try:
            ecu.wait_for_ecu()
        except Error:
            # Fatal Error
            sys.exit(1)
    except StateChanged:
        try:
            ecu.set_session(session)
        except Error:
            # Fatal Error
            sys.exit(1)

A related problem is, that we currently don't have higher level exceptions.
When a request fails, we typically reraise the last UDS Exception.
For the reconnect this means, that in case the read_session fails, it would raise a UDS RequestOutOfRange exception.
This is not intuitive and probably not what the user expects.
One solution would be to convert the reconnect function to type bool and communicate the error via the return code.
We could do this for the "changed state" case, but what about the "connection error" case?
I'm not sure, if it's a good idea, to mix return code and exceptions for error handling.

This is all not so convincing I must admit.
Does anyone have an idea how to solve this nicely?

@peckto
Copy link
Contributor Author

peckto commented Jul 26, 2022

The problematic error handling maybe shows, that it's not a good idea to combine the update state with the reconnect function.
Maybe it's easier if the scanner is responsible to restore the desired state.

I updated this PR and added check_and_set_session after reconnect where its necessary.

Copy link

stale bot commented Jul 14, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant