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

Propagate code and reason from disconnect from server on client #212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

emilm
Copy link
Contributor

@emilm emilm commented Sep 21, 2022

The reason for this change is to have different reconnect strategies depending on why you are disconnected.
If the OCPP rejected you because of a 404 or something like that, or if you simply lose connection.

My question here is what do do with explicit disconnects from within the code?
Right now I pass 0, "disconnect() method called" - but it looks a bit hacky.
2 other alternatives are:

a separate disconnect method without arguments - but then the implementation must handle 2 disconnect() methods.
nullable Integer so no error code is passed instead of a magic 0 or -1 or something

Copy link
Member

@TVolden TVolden left a comment

Choose a reason for hiding this comment

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

Code looks fine. I think it's a good idea to only have one disconnected method and more explicit information is a good idea, since it allows implementers to act on the information.

That said. I can see that a code and reason is good for debug purposes, but anything other than that, would require some form of custom protocol contracts between communicating parties, right?

@emilm
Copy link
Contributor Author

emilm commented Sep 21, 2022

Yes it feels better to have separate methods.
The ocpp server in question gives a 404 but the code is 1002. this is when the charge point does not exist on the server . it's before websocket is even established i think.

It's necessary to have code and reason to have different behaviors based on the disconnection reason . i believe a physical disconnect will give another code etc.

@emilm emilm force-pushed the feature-propagate-close-codes branch 2 times, most recently from 0de3f8a to f041a38 Compare September 21, 2022 22:33
@emilm
Copy link
Contributor Author

emilm commented Sep 21, 2022

I think it's a good idea to only have one disconnected method and more explicit information is a good idea, since it allows implementers to act on the information.

I added a separate commit showing how it would look like with separate method for disconnect without params, but I forced pushed the first commit again after reading your post again! So now it is in it's original state.

@TVolden
Copy link
Member

TVolden commented Sep 22, 2022

We should note that this change has breaking changes to some external APIs, namely:

  • ClientEvents.java
  • SessionEvents.java

I don't know how to avoid it. But since it would trigger a major version change on the API, I would either create version 2 branch or welcome any ideas on how to avoid it.

@emilm
Copy link
Contributor Author

emilm commented Sep 22, 2022

Would it be better with the commit I did with keeping the parameterless version + add with code / reason? Would it still be considered a breaking change? 0de3f8a

@TVolden
Copy link
Member

TVolden commented Sep 22, 2022

Would it be better with the commit I did with keeping the parameterless version + add with code / reason? Would it still be considered a breaking change? 0de3f8a

The fact that they are interfaces will cause the breaking change as long as a new method is added, an existing one is removed or modified. As far as I know, there isn't alot we can do to avoid it

@emilm emilm force-pushed the feature-propagate-close-codes branch from f041a38 to 9980299 Compare September 22, 2022 13:10
@TVolden TVolden added the Change of API A change that will trigger a major version change according to Semantic Versioning label Oct 3, 2022
@emilm
Copy link
Contributor Author

emilm commented Feb 7, 2023

So what's the verdict on this @TVolden ? :)

@TVolden
Copy link
Member

TVolden commented Feb 8, 2023

Hi @emilm,
I seem to have created a v2 branch for these changes, you just need to change the PR to target that branch.

@conrad-uraga-power-x
Copy link

First off, thank you TVolden for making this library. I’m just wondering what’s the status of this PR? These changes would be helpful.


Best regards,

@TVolden
Copy link
Member

TVolden commented Jun 2, 2023

Hi @conrad-uraga-power-x

The PR should target the v2 branch instead of master. That's it, really 🙂

Sincerely
Thomas Volden

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change of API A change that will trigger a major version change according to Semantic Versioning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants