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

Add ConfirmationCompletedHandler #210

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

emilm
Copy link
Contributor

@emilm emilm commented Sep 21, 2022

Add a handler on each Confirmation perform actions after a confirmations are sent back to the Charge Point.
This is to send configuration variables etc after the BootNotification is confirmed from the server. Some CPs don't accept configs before the confirmation on BootNotification is returned.
This makes it possible within the synchronous nature of the handlers.

@mmauksch
Copy link
Contributor

Quick Question: Is this feature only needed for BootNotifications? In that case maybe it's better to just have BootNotificationConfirmation implement the ConfirmationCompletedHandler and check in the Communicator: Is it assignable to ConfirmationCompletedHandler? If so: call it.

I'm coming from the point of view, that i'm not really comfortable with the idea of changing all the interfaces there to abstract classes and tacking on the whole handler thing onto confirmations that have no need for it.

I hope my point of view is described in a understandable way :)

@emilm
Copy link
Contributor Author

emilm commented Sep 21, 2022

I understand your point and I was uncomfortable with it myself. But it's a generic library and I am not sure which confirmation should have this handler or not.
In my case it's only the bootnotification, so I could have a specific implementation there.

Should the communicator type check and check for an interface? Would need additional logic.

@krzysztofxkwiecien
Copy link

Great timing as I was just about to start implementing similar functionality myself - I need it not only for BootNotification, but also for SignCertificate request from the security extension, so having it in the parent class is the best solution.

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.

The code looks good. The only concern I have with changing an interface to an abstract class is the limit of one class inheritance, but since they're internal models it should be okay.
You should still put your own name on the new class, and consider appending your name to the modified classes :)


/*
ChargeTime.eu - Java-OCA-OCPP
Copyright (C) 2015-2022 Thomas Volden <tv@chargetime.eu>
Copy link
Member

Choose a reason for hiding this comment

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

Should be replaced with your own name. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks ! I will add my name to it !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it now! Hopefully it's fine :)

@TVolden
Copy link
Member

TVolden commented Sep 22, 2022

As far as I can see this change won't affect the external API. Community, please confirm.

@TVolden TVolden merged commit b550541 into ChargeTimeEU:master Sep 22, 2022
@TVolden
Copy link
Member

TVolden commented Sep 22, 2022

Okay, it's merged.

Thanks for you for the contribution, it really means a lot.

Sincerely,
Thomas Volden

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.

4 participants