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

Adding powermeter interface to DcSupplySimulator #758

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

SebaLukas
Copy link
Contributor

Describe your changes

see title

Issue ticket number and link

Missing powermeter interface in DcSupplySimulator

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

Signed-off-by: Sebastian Lukas <sebastian.lukas@pionix.de>
@SebaLukas SebaLukas force-pushed the feat/adding-powermeter-to-DCSupplySimulator branch from 10668fc to c277302 Compare July 3, 2024 11:47
@SebaLukas SebaLukas merged commit 8c129e6 into main Jul 3, 2024
7 of 8 checks passed
@SebaLukas SebaLukas deleted the feat/adding-powermeter-to-DCSupplySimulator branch July 3, 2024 12:01
Copy link
Contributor

@barsnick barsnick left a comment

Choose a reason for hiding this comment

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

Nice. Very welcomed.

In the original PR for DcSupplySimulator, the wish was expressed to obsolete the JsDcSupplySimulator in the same step. Perhaps this should be done here.

energy_import_total += (connector_voltage * connector_current * 0.5) / 3600;
}
if (connector_current < 0) {
energy_export_total += (connector_voltage * -connector_current * 0.5) / 3600;
Copy link
Contributor

Choose a reason for hiding this comment

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

This 0.5 directly correlates with LOOP_SLEEP_MS. Perhaps you should use that constant (i.e. LOOP_SLEEP_MS / 1000.0) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍 I'll change that quickly

Comment on lines +15 to +25
types::powermeter::TransactionStartResponse
powermeterImpl::handle_start_transaction(types::powermeter::TransactionReq& value) {
return {types::powermeter::TransactionRequestStatus::OK};
}

types::powermeter::TransactionStopResponse powermeterImpl::handle_stop_transaction(std::string& transaction_id) {
return {types::powermeter::TransactionRequestStatus::NOT_SUPPORTED,
{},
{},
"DcSupplySimulator does not support stop transaction request."};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit peculiar that this module would "successfully" handle StartTransaction, but not StopTransaction. I have not found this combination anywhere else.

This is about OCMF. right? So types::powermeter::TransactionRequestStatus::NOT_SUPPORTED in both cases, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this directly from the JsDcSupplySimulator module. But it makes sense that the StartTransaction is not supported here either. I will change that too.

@barsnick
Copy link
Contributor

barsnick commented Jul 3, 2024

Oops, too late, already merged. 😁

@SebaLukas SebaLukas mentioned this pull request Jul 4, 2024
3 tasks
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