Implement SetChargingProfileRequest and basic database storage/loading

[christopher-davis-afs]

Describe your changes

Implements handling SetChargingProfileRequest from K01

Checklist before requesting a review

• I have performed a self-review of my code
• I have made corresponding changes to the documentation
• If OCPP 2.0.1: I have updated the OCPP 2.0.1 status document
• I read the contribution documentation and made sure that my changes meet its requirements

Depends on #682

[christopher-davis-afs]

CC: @shankari

[shankari]

@louisg1337 @the-bay-kay @MukuFlash03 can you pre-review this before I take a look?

[christopher-davis-afs]

I'm very confused by the tests segfaulting on the last few CI runs. They do not segfault locally.

[marcemmers]

I'm very confused by the tests segfaulting on the last few CI runs. They do not segfault locally.

I am not sure if it's related but I have seen issues with the test databases before. It was the other way around then though, Segfaults locally and not on the server.

In general I think it's better to unit test against in-memory databases instead of actual file databases. In this case I would replace /tmp/ocpp201/cp.db with an in memory database. Make sure you don't use the same cache as the device model does ;)

[maaikez]

I'm very confused by the tests segfaulting on the last few CI runs. They do not segfault locally.

I am not sure if it's related but I have seen issues with the test databases before. It was the other way around then though, Segfaults locally and not on the server.

In general I think it's better to unit test against in-memory databases instead of actual file databases. In this case I would replace /tmp/ocpp201/cp.db with an in memory database. Make sure you don't use the same cache as the device model does ;)

I did make some changes on the database stuff, when it opens I am adding some flags. Can that be the reason for recent crashes?

The in memory database did not work because those flags are missing, but if you use the correct flags, it will work. I did not look into the test to see if they use those flags but if you want to change to an in memory database, you have to check that.

[maaikez]

I'm very confused by the tests segfaulting on the last few CI runs. They do not segfault locally.

I am not sure if it's related but I have seen issues with the test databases before. It was the other way around then though, Segfaults locally and not on the server.
In general I think it's better to unit test against in-memory databases instead of actual file databases. In this case I would replace /tmp/ocpp201/cp.db with an in memory database. Make sure you don't use the same cache as the device model does ;)

I did make some changes on the database stuff, when it opens I am adding some flags. Can that be the reason for recent crashes?

The in memory database did not work because those flags are missing, but if you use the correct flags, it will work. I did not look into the test to see if they use those flags but if you want to change to an in memory database, you have to check that.

Having said, this was when not looking into the logs. I now looked into the logs but I can not really see why the tests fail. They seem to stop, or crash maybe, but it is not really clear to me which tests fail and why.

[christopher-davis-afs]

The tests didn't segfault this time for some reason?

[shankari]

@christopher-davis-afs @couryrr-afs @wjmp @drmrd I have requested short descriptions/outlines of functionality as part of the commit messages or PRs before, notably in context of #685 (comment) and in sprint review meetings, but I don't see that in this PR. Do you have an ETA of when you plan to start including them?

I am fine with all the nits being addressed in a cleanup change. Before approval, I would like to understand:

1. the process for tracking the comments and ensuring that they are addressed
2. at least a high-level design of what the final error handling during load will be; even if the implementation waits until later

[shankari]

@couryrr-afs just to clarify, one of the two items I had asked for before approval was:

at least a high-level design of what the final error handling during load will be; even if the implementation waits until later

Here's the comment around error handling #711 (comment)
I don't see a response to it, either in this PR or in the non-blocking comments PR. The "non-blocking PR" also does not include the nits from this PR, only from #611

If you did add the response elsewhere and I missed it, can you add a link to it here?

Again, I am looking for two high level topics before I approve:

• Tracking the non-blocking comments so they don't get lost
• A high-level description of the design for the final error handling (Implement SetChargingProfileRequest and basic database storage/loading #711 (comment)) so that we can write it down before we forget it

[shankari]

Still not seeing the non-blocking issues from this PR in https://github.com/US-JOET/base-camp/issues/173

[couryrr-afs]

@couryrr-afs just to clarify, one of the two items I had asked for before approval was:

at least a high-level design of what the final error handling during load will be; even if the implementation waits until later

Here's the comment around error handling #711 (comment) I don't see a response to it, either in this PR or in the non-blocking comments PR. The "non-blocking PR" also does not include the nits from this PR, only from #611

If you did add the response elsewhere and I missed it, can you add a link to it here?

Again, I am looking for two high level topics before I approve:

* Tracking the non-blocking comments so they don't get lost

* A high-level description of the design for the final error handling ([Implement SetChargingProfileRequest and basic database storage/loading #711 (comment)](https://github.com/EVerest/libocpp/pull/711#discussion_r1690006967)) so that we can write it down before we forget it

@shankari the tracking ticket on the AFS side has been updated with what I believe to be a full list.

For the conversation around the database I asked for it to be discussed in the WG, 8/7/2024, and will update the issue referenced with those details. Outside of the comments here is there anything else that is needed for this topic?

[shankari]

I spot checked the tracking ticket and it has a couple of issues from here. Giving you the benefit of the doubt that all the pending changes have been tracked. Approving this now.

[shankari]

@marcemmers @hikinggrass @Pietfried I haven't seen any requests from the community to fix the static code analysis, so I didn't hold up my approval for it. However, for the projects that I maintain, I generally don't like to regress on the automated checks. Once you do that once, then the check is essentially useless until it is fixed, because the existing errors will mask any new ones. I look forward to seeing how you respond to this as part of your approval....

[marcemmers]

Just one small comment, otherwise looks good to me 👍

[marcemmers]

@shankari Thanks for bringing it up, I agree we don't always look that well at the static code analysis. Unfortunately this particular tool is flagging false positives sometimes so I have grown a little complacent with it.

For this ticket it is complaining about SQL issues and duplication in test code so for me there are no blockers there. Might be a good idea to disable some of the warnings we don't look at anyway @hikinggrass @Pietfried

[marcemmers]

Can this be merged @christopher-davis-afs ? Let me know and I'll press the button.

[christopher-davis-afs]

Yes, this is ready :)