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

Integrated libocpp patch #28

Merged

Conversation

louisg1337
Copy link

Describe your changes

This PR integrates the libocpp.patch file that is currently being using in the charin-e2e-demo. The goal is to completely get rid of all of the patches so that the charin-e2e-demo can be fully functional and not rely on hacky patches.

Testing Done

This code was tested alongside all of the other patches in everest-demo/charin-e2e-demo. In the demo, setChargingProfile, which this patch helps implement, seems to work on a basic level.

Issue ticket number and link

The issue that this is related to can be found here.

Checklist before requesting a review

Copy link

@shankari shankari left a comment

Choose a reason for hiding this comment

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

@louisg1337 we should clean up this patch a bit before having others review it. Concretely, we should:

  • downgrade all the EVLOG_errors to EVLOG_warning or EVLOG_info or...
  • remove out unnecessary commented out code
  • make the MessageType::SetChargingProfile handling be similar to the others

A lot of this was hacking the code together to figure out what was going on, we should clean it up before submission

@shankari shankari marked this pull request as draft June 28, 2024 16:02
@louisg1337 louisg1337 marked this pull request as ready for review June 28, 2024 17:30
lib/ocpp/v201/charge_point.cpp Outdated Show resolved Hide resolved
Signed-off-by: louisg1337 <[email protected]>
Copy link

@shankari shankari left a comment

Choose a reason for hiding this comment

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

@drmrd @couryrr-afs @wjmp please let us know if AFS is able to and wants to review this. Otherwise, we will just merge!

@couryrr-afs
Copy link

This looks fine to me for the demo code.

@shankari
Copy link

shankari commented Jul 2, 2024

Note that we are requesting to merge this into the main libocpp, not into the demo code.
We currently have this only in the demo code through patches, but that is not a viable long-term solution.
Also the code to handle the incoming SetChargingProfile method is required to hook up the CSMS to the station, so it is most assuredly not demo code. Or are you saying that it will be reimplemented by AFS later?

@shankari
Copy link

shankari commented Jul 2, 2024

I am going to merge this in the next few days

@couryrr-afs
Copy link

Note that we are requesting to merge this into the main libocpp, not into the demo code. We currently have this only in the demo code through patches, but that is not a viable long-term solution. Also the code to handle the incoming SetChargingProfile method is required to hook up the CSMS to the station, so it is most assuredly not demo code. Or are you saying that it will be reimplemented by AFS later?

@shankari we are actively working on this implementation with test coverage.

@shankari
Copy link

shankari commented Jul 2, 2024

ok I will merge this change, we can submit to the main everest repo once the tests are done!

@shankari shankari merged commit 232bf8b into US-JOET:charin-e2e-demo Jul 2, 2024
@shankari
Copy link

shankari commented Jul 8, 2024

@louisg1337 can you please change this to point to the charin-e2e-demo-with-revisions branch? That is the one that is used in the demo

https://github.com/US-JOET/everest-core/blob/60b4062862460fa7c48a2d79c32c20106d8b8e4a/dependencies.yaml#L59

@louisg1337
Copy link
Author

@louisg1337 can you please change this to point to the charin-e2e-demo-with-revisions branch? That is the one that is used in the demo

@shankari new PR which points to charin-e2e-demo-with-revisions can be found here.

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.

3 participants