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

Feature/powermeter interface adaptions #347

Merged
merged 11 commits into from
Sep 19, 2023

Conversation

klemmpnx
Copy link
Contributor

Adjusts the Powermeter Interface and types:

  • Start transaction: Returns status + optional time bounds for earliest and latest stop of the transaction; optional error message
  • Stop transaction: Returns status + ocmf string (if successful); optional error message
  • Tarif ID and Cable ID are switch to integers (as in LEM DCBM spec)

Follow up adaptions of the mopdules

  • GenericPowerMeter
  • PowermeterBSM
  • MicroMegaWattBSP
  • YetiDriver
  • JSYetiSimulator

@klemmpnx klemmpnx self-assigned this Sep 18, 2023
@klemmpnx klemmpnx force-pushed the feature/powermeter-interface-adaptions branch from 703097f to 8b8f78a Compare September 18, 2023 14:24
switch cable id and tariff ids to integer;
add start and stop transaction responses

Signed-off-by: Fabian Klemm <[email protected]>
Signed-off-by: Fabian Klemm <[email protected]>
Signed-off-by: Fabian Klemm <[email protected]>
Signed-off-by: Fabian Klemm <[email protected]>
Signed-off-by: Fabian Klemm <[email protected]>
@klemmpnx klemmpnx force-pushed the feature/powermeter-interface-adaptions branch from 2d50437 to 4b766f9 Compare September 18, 2023 14:49
Signed-off-by: Fabian Klemm <[email protected]>
Copy link
Contributor

@corneliusclaussen corneliusclaussen left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor things!

additionalProperties: false
required:
- status
- transaction_id
Copy link
Contributor

Choose a reason for hiding this comment

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

transaction_id is required but not defined

std::string powermeterImpl::handle_stop_transaction(std::string& transaction_id) {
return "NOT_SUPPORTED";
types::powermeter::TransactionStopResponse powermeterImpl::handle_stop_transaction(std::string& transaction_id) {
return {.status = types::powermeter::TransactionRequestStatus::NOT_SUPPORTED,
Copy link
Contributor

Choose a reason for hiding this comment

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

The designated initializers for structs (the dot notation) is valid C99 afaik but not in C++17 (it was added in C++20 I think). GCC supports a lot of C99 while compiling C++ but this may not be portable when using other compilers

return true;
types::powermeter::TransactionStartResponse
powermeterImpl::handle_start_transaction(types::powermeter::TransactionReq& value) {
return {.status = types::powermeter::TransactionRequestStatus::OK};
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment below about designated inits

@@ -45,18 +45,20 @@ std::string powermeterImpl::handle_stop_transaction(std::string& transaction_id)

bsm::SignedOCMFSnapshot signed_snapshot(data);

return signed_snapshot.O();
return {.status = types::powermeter::TransactionRequestStatus::OK, .ocmf = signed_snapshot.O()};
Copy link
Contributor

Choose a reason for hiding this comment

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

see below

return false;
types::powermeter::TransactionStartResponse
powermeterImpl::handle_start_transaction(types::powermeter::TransactionReq& value) {
return {.status = types::powermeter::TransactionRequestStatus::NOT_SUPPORTED,
Copy link
Contributor

Choose a reason for hiding this comment

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

see below

std::string powermeterImpl::handle_stop_transaction(std::string& transaction_id) {
return "NOT_SUPPORTED";
types::powermeter::TransactionStopResponse powermeterImpl::handle_stop_transaction(std::string& transaction_id) {
return {.status = types::powermeter::TransactionRequestStatus::NOT_SUPPORTED,
Copy link
Contributor

Choose a reason for hiding this comment

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

see below

return false;
types::powermeter::TransactionStartResponse
powermeterImpl::handle_start_transaction(types::powermeter::TransactionReq& value) {
return {.status = types::powermeter::TransactionRequestStatus::NOT_SUPPORTED,
Copy link
Contributor

Choose a reason for hiding this comment

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

see below

std::string powermeterImpl::handle_stop_transaction(std::string& transaction_id) {
return "NOT_SUPPORTED";
types::powermeter::TransactionStopResponse powermeterImpl::handle_stop_transaction(std::string& transaction_id) {
return {.status = types::powermeter::TransactionRequestStatus::NOT_SUPPORTED,
Copy link
Contributor

Choose a reason for hiding this comment

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

see below

@klemmpnx
Copy link
Contributor Author

@corneliusclaussen : thx for the review! I applied everything you mentioned. There is a final eslint complaint about unused variables; however, that is a common thing in the /modules/simulation/JsYetiSimulator/index.js - so I'd rather leave this consistent in this file

Copy link
Contributor

@corneliusclaussen corneliusclaussen left a comment

Choose a reason for hiding this comment

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

Please squash into single commit with proper description of the changes before merging

@klemmpnx klemmpnx merged commit c3f8a29 into main Sep 19, 2023
@klemmpnx klemmpnx deleted the feature/powermeter-interface-adaptions branch September 19, 2023 12:02
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.

2 participants