OCPP 2.0.1: Refactor Smart Charging Persistence #790
Conversation
christopher-davis-afs
requested review from
Pietfried,
hikinggrass,
marcemmers and
maaikez
as code owners
|
@shankari Ready for review :) |
christopher-davis-afs
changed the title
There was a problem hiding this comment.
Oh wait, I forgot that this was the in-memory DB change, for which I have lots of high-level comments. I had noted them when I looked at the high level, but got forgot when I came back to finish the review and got caught up in the minutia.
... where valid profiles were stored in both an in-memory data structure and a sqlite database. After this the database is only used when the system power cycles to read the profile from the database back into memory. This was useful during the early stages of development. Looking at this now it seems to add complexity to the code to keep the two storage mechanisms in sync and duplicates data amongst memory and hdd space.
I understand the benefit of the change in terms of simiplicity and maintainability, but there are also potential cons in terms of performance since I/O is slower than in-memory accesses. Have you done a performance evaluation of this change
? Is it on the critical path during handshakes (I assume yes, at least for the ISO handshake)? What, if anything, is the impact of this change on the timing of the handshake, particularly since the station is designed to be run as an embedded system?
Please summarize the design discussions around this decision
Partially includes #782 in order to resolve an issue with clashing test fixture names that resulted in a segfault.
Please link to the related issue for the record.
|
We have not done a full performance evaluation, but we discussed this with the community on a previous working group call, and asked about performance as a reason for the existing cache. The consensus we came to was that performance should not be an issue. We've asked for the maintainers to provide additional input on this here.
By issue I did not mean GitHub issue. Rather, a problem occurred while trying to rebase this branch that caused a segfault specific to this branch. Applying the changes from #782 resolved the problem. |
|
Glad to see we are simplifying this and moving to a single storage location! I am on the road and did not do a full review of the code yet. One thing I would ask is to combine the migration files into a single file. There is not going to be a difference functionality wise but it just keeps it a bit cleaner in my opinion. |
christopher-davis-afs
force-pushed
the
feature/smartcharging-db-cleanups
branch
from
128754e
to
a7cf7ac
Compare
|
I am not able to resolve comments, so I have added 👍 to comments that I agree with. There is only one left, and I am willing to make it non-blocking if it is complex to fix. |
| const std::optional<ClearChargingProfile>& criteria); | ||
|
|
||
| /// \brief Get all profiles from table CHARGING_PROFILES matching \p profile_id or \p criteria | ||
| std::vector<std::tuple<int, v201::ChargingProfile, ChargingLimitSourceEnum>> |
There was a problem hiding this comment.
It might be better to make a helper struct for this return value so that you can actually name the int for example.
There was a problem hiding this comment.
Would it be acceptable to use a type alias, e.g using EvseId = int? And/or tuple destructuring where this function is used?
There was a problem hiding this comment.
That would improve it a bit, but I still think a struct is easier to read and maintain. Is there a reason you would like to stick to the tuple?
There was a problem hiding this comment.
Mainly: It seems odd to add a struct for one specific use case.
It seems I still left the ReportedChargingProfiles struct in tact, so I'll re-use that.
There was a problem hiding this comment.
Err, that might need to be moved somewhere else because of a recursive import. Where would be a good place to put that struct?
When calling `validate_and_add_profile()`, `add_profile()`, and `insert_or_update_charging_profile()`, we now can be given a source for the profile. This source is stored in our database and can be retrieved with the new method, `get_charging_limit_source_for_profile()`. We use this new method to retrieve the source when retrieving profiles for K09. All new functionality is under test. Co-authored-by: Coury Richards <[email protected]> Signed-off-by: Christopher Davis <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
Remove the map of profiles from the smart charging handler and use the database for storing, loading, and finding profiles. We've removed the function for getting all profiles from the smart charging handler, and in all cases where that was used, including tests, we retrieve the profiles from the database. Signed-off-by: Christopher Davis <[email protected]>
SQLite needs to hold onto a copy of the string in order for it to be stored properly. Signed-off-by: Christopher Davis <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
Use the database handler to select matching profiles instead of iterating over the entire list of profiles. Signed-off-by: Christopher Davis <[email protected]>
Use new functions on the database handler to reimplement clearing charging profiles by a given set of criteria. This removes the existing implementation within the smart charging handler. Signed-off-by: Christopher Davis <[email protected]>
christopher-davis-afs
force-pushed
the
feature/smartcharging-db-cleanups
branch
2 times, most recently
from
84226e3
to
a977746
Compare
Use new functions on the database handler to implement getting charging profiles by criteria. This allows us to remove more places where we were iterating over the entire list of charging profiles. This commit includes a fix for a bug where we were only accepting searches if one of the sources we were searching for is CSO. Co-authored-by: Christopher Davis <[email protected]> Signed-off-by: Coury Richards <[email protected]> Signed-off-by: Christopher Davis <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
The name wasn't quite right, leading to some confusion. Signed-off-by: Christopher Davis <[email protected]>
…rofiles, removed logic that adds profiles and replaced it with logic that only removes invalid profiles Signed-off-by: Peter Giavotto <[email protected]>
christopher-davis-afs
force-pushed
the
feature/smartcharging-db-cleanups
branch
from
a977746
to
f1ceecc
Compare
Describe your changes
Reworks how we handle the storage and loading of charging profiles, making the database the sole source of truth.
Partially includes #782 in order to resolve an issue with clashing test fixture names that resulted in a segfault. We also include a change to the list of dependencies to rename the
json_schema_validatordep, as using the wrong username generated some confusion.Issue ticket number and link
Resolves #713 for v201
Checklist before requesting a review