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

Remote start and stop could fail in core, we should be able to report that #817

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

marcemmers
Copy link
Contributor

Describe your changes

Our implementation could result in a failure to start or stop and there was no option to report that to the CSMS. With this change we force the core to provide a result. It will still only get called if libocpp thinks it is accepted.

Issue ticket number and link

Checklist before requesting a review

… that

Signed-off-by: Marc Emmers <m.emmers@alfen.com>
Signed-off-by: Marc Emmers <m.emmers@alfen.com>
Copy link
Contributor

@Pietfried Pietfried left a comment

Choose a reason for hiding this comment

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

We have only discussed but not taken any action yet to include statusInfo information more in all kinds of responses. For this case this probably results in a different return type RequestStartTransactionResponse / RequestStoptTransactionResponse.

For this case you can add it as part of this PR or we do this in a subsequent PR (which we need anyways), in which we address other callbacks and internal derivation of statusInfo if possible. I'm approving this and leave that up to you.

@marcemmers
Copy link
Contributor Author

We have only discussed but not taken any action yet to include statusInfo information more in all kinds of responses. For this case this probably results in a different return type RequestStartTransactionResponse / RequestStoptTransactionResponse.

For this case you can add it as part of this PR or we do this in a subsequent PR (which we need anyways), in which we address other callbacks and internal derivation of statusInfo if possible. I'm approving this and leave that up to you.

I'd rather first determine what we are going to do exactly and then change these again to the final result. Otherwise chances are high that it needs breaking changes in the future again anyway.

@marcemmers marcemmers merged commit fa80193 into main Oct 7, 2024
4 checks passed
@marcemmers marcemmers deleted the me-allow-remote-start-stop-to-fail branch October 7, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants