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.

Sign up for GitHub

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

Jump to bottom

SmartChargingHandler: Add validation for TxProfiles #452

Merged
merged 3 commits into from
Mar 11, 2024
Merged

SmartChargingHandler: Add validation for TxProfiles #452

merged 3 commits into from
Mar 11, 2024

Conversation

christopher-davis-afs
Copy link
Contributor

@christopher-davis-afs christopher-davis-afs commented Feb 6, 2024 •
edited
Loading

Creates a skeleton for SmartChargingHandler and adds a method
to validate TxProfiles and TxProfiles only. This method
ensures that a given profile (assumed to be TxProfile) fits all of
the functional requirements specific to TxProfiles per K01.

The function, validate_tx_profile (), takes a profile and an
EVSE, and returns a variant of ProfileValidationResultEnum.
This enum allows us to have detailed responses for the different types
of invalid profiles, and eventually will let us log why a profile
is invalid if the general validation fails.

Part of the prerequisites for #361

Related to #467 and #468

@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch 3 times, most recently from d14ff09 to ff0316b Compare February 6, 2024 20:41
@christopher-davis-afs
Copy link
Contributor Author

Codacy is saying a private member field (charging_profiles) is never used, but that's objectively false - it's accessed and modified from within our source file. There's nothing we can really do in this case, since in C++ member fields must be set up in the header files.

@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch 4 times, most recently from a667350 to 26746ac Compare February 6, 2024 21:37
@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch 2 times, most recently from c803315 to f27f30c Compare February 7, 2024 16:58
drmrd
drmrd suggested changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@drmrd drmrd left a comment

Choose a reason for hiding this comment

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

Let's discuss tomorrow, but I think now is the time to norm on a naming convention for tests that verify FRs from the 2.0.1 spec.

tests/lib/ocpp/v201/test_smart_charging_handler.cpp Outdated Show resolved Hide resolved
@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch from f27f30c to f7db302 Compare February 8, 2024 21:01
@drmrd
Copy link
Collaborator

drmrd commented Feb 13, 2024

Thanks for adding the FRs! I went ahead and added an issue that we can point to in the future regarding the functional requirement naming convention: #467.

@drmrd
Copy link
Collaborator

drmrd commented Feb 14, 2024

@christopher-davis-afs Could you link to #467 and #468 from the PR description? They provide additional context for future developers that motivates how we are implementing functionality for 2.0.1.

@christopher-davis-afs
Copy link
Contributor Author

Done

shankari
shankari approved these changes Feb 16, 2024
View reviewed changes
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.

Glad to see the new naming convention in place here!

@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch from b9bda6b to 8c748bc Compare February 20, 2024 16:13
Pietfried
Pietfried requested changes Feb 25, 2024
View reviewed changes
Copy link
Contributor

@Pietfried Pietfried left a comment

Choose a reason for hiding this comment

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

This looks good to me except for some requested changes.

Two more general comments in addition to the inline comments:
In general we try to use snake_case for our variable and function definitions. I see some inconsistencies between snake_case and camelCase.

As already mentioned in the cloud working group meeting we would like to keep the scope of PRs as small as possible so we would rather see the ComponentStateManagerInterface refactor as a separate PR to the changes for SmartCharging. For this PR we can keep, but please consider this for future PRs.

include/ocpp/v201/smart_charging.hpp Outdated Show resolved Hide resolved
include/ocpp/v201/smart_charging.hpp Outdated Show resolved Hide resolved
.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
include/ocpp/v201/component_state_manager.hpp Show resolved Hide resolved
include/ocpp/v201/component_state_manager.hpp Show resolved Hide resolved
christopher-davis-afs and others added 2 commits February 26, 2024 20:12
@christopher-davis-afs @gberardi-pillar
ComponentStateManager: Create interface for testability
1725770 
Splits the ComponentStateManager API out into an interface so
that it's easier to write unit tests that make use of its functionality.

Co-authored-by: Gianfranco Berardi <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
@christopher-davis-afs @gberardi-pillar
DatabaseHandler: Fix empty check
d9536ad 
In `transaction_metervalues_insert ()`, we were accessing the
first element of a list, then checking whether or not it was
empty. This causes an exception since if this list *is* empty,
there's no element to access.

This commit shifts the code around so that the empty check occurs
before accessing any elements of the list.

Co-authored-by: Gianfranco Berardi <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch from 8c748bc to 47f612d Compare February 26, 2024 20:42
@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch 4 times, most recently from ce6b167 to 710d489 Compare February 27, 2024 20:58
@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch from 710d489 to d48f4d6 Compare February 29, 2024 20:36
@christopher-davis-afs @gberardi-pillar
SmartChargingHandler: Add validation for TxProfiles
49f4725 
Creates a skeleton for SmartChargingHandler and adds a method
to validate TxProfiles and TxProfiles only. This method
ensures that a given profile (assumed to be TxProfile) fits all of
the functional requirements specific to TxProfiles per K01.

The function, `validate_tx_profile ()`, takes a profile and an
EVSE, and returns a variant of `ProfileValidationResultEnum`.
This enum allows us to have detailed responses for the different types
of invalid profiles, and eventually will let us log *why* a profile
is invalid if the general validation fails.

Part of the prerequisites for EVerest#361

Co-authored-by: Gianfranco Berardi <[email protected]>
Signed-off-by: Christopher Davis <[email protected]>
@christopher-davis-afs christopher-davis-afs force-pushed the cdavis/add-v201-validate-profile branch from d48f4d6 to 49f4725 Compare March 6, 2024 14:57
@marcemmers marcemmers self-requested a review March 6, 2024 16:56
@christopher-davis-afs christopher-davis-afs mentioned this pull request Mar 8, 2024
Merged
3 tasks
@drmrd
Copy link
Collaborator

drmrd commented Mar 8, 2024

Thanks for the reviews, @Pietfried, @hikinggrass, and @marcemmers! When can we expect this to be merged?

@Pietfried Pietfried merged commit 51b3a62 into EVerest:main Mar 11, 2024
4 checks passed
@christopher-davis-afs christopher-davis-afs deleted the cdavis/add-v201-validate-profile branch April 29, 2024 15:24
This was referenced May 2, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gberardi-pillar gberardi-pillar gberardi-pillar left review comments

@Pietfried Pietfried Pietfried approved these changes

@marcemmers marcemmers marcemmers approved these changes

@drmrd drmrd drmrd approved these changes

@shankari shankari shankari approved these changes

@hikinggrass hikinggrass hikinggrass approved these changes

@maaikez maaikez Awaiting requested review from maaikez maaikez is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@christopher-davis-afs @drmrd @hikinggrass @shankari @marcemmers @Pietfried @gberardi-pillar