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

Add SignedMeterValue type powermeter types #461

Merged
merged 20 commits into from
Feb 23, 2024
Merged

Add SignedMeterValue type powermeter types #461

merged 20 commits into from
Feb 23, 2024

Conversation

hikinggrass
Copy link
Contributor

This allows the encapsulation of additionaly metadata like the signing method, encoding method or the public key next to the signed meter data

@hikinggrass hikinggrass force-pushed the kh-signed-meter-value-extension branch from fb6938b to cc3e181 Compare December 4, 2023 12:59
@hikinggrass hikinggrass marked this pull request as ready for review December 5, 2023 17:14
valentin-dimov
valentin-dimov previously requested changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@valentin-dimov valentin-dimov left a comment

Choose a reason for hiding this comment

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

We also need to adjust the TransactionFinished in the evse_manager interface to use the SignedMeterValue type.

types/powermeter.yaml Outdated Show resolved Hide resolved
@hikinggrass hikinggrass force-pushed the kh-signed-meter-value-extension branch from cc3e181 to 437d9e3 Compare December 7, 2023 17:23
valentin-dimov
valentin-dimov reviewed Dec 11, 2023
View reviewed changes
Copy link
Contributor

@valentin-dimov valentin-dimov 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, only some minor questions left

types/evse_manager.yaml Outdated Show resolved Hide resolved
types/evse_manager.yaml Outdated Show resolved Hide resolved
types/powermeter.yaml Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
@hikinggrass hikinggrass force-pushed the kh-signed-meter-value-extension branch from 4ac9522 to a030958 Compare December 21, 2023 15:40
@hikinggrass hikinggrass requested a review from Pietfried January 8, 2024 20:31
golovasteek
golovasteek reviewed Feb 2, 2024
View reviewed changes
types/units_signed.yaml
L1:
description: AC L1 value only
type: object
$ref: /units_signed#/SignedMeterValue
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, are there any meters out there that provide signed Voltage readings?

Same for frequency and other detailed measurements, every signed separtely.

Copy link
Contributor

Choose a reason for hiding this comment

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

After Conversation in person I understood that we don't want to limit signed values by the transactional singed values only. And the transaction values are just

Nevertheless I propose to not create types that are not used and we don't know how to use them. We can leave a comment in example how to extend it if needed.

@golovasteek
Copy link
Contributor

Looking forward for this to be applied. Preferrably reduced to "SignedMeterValues" only.

Can we push this PR forward? So that further work on the power meters can be based on it already?

@hikinggrass
Copy link
Contributor Author

Looking forward for this to be applied. Preferrably reduced to "SignedMeterValues" only.

Can we push this PR forward? So that further work on the power meters can be based on it already?

Yes, I'll rebase it and will try to restructure it a bit

@hikinggrass hikinggrass force-pushed the kh-signed-meter-value-extension branch from 8d12f73 to 76e277e Compare February 9, 2024 11:50
@hikinggrass hikinggrass force-pushed the kh-signed-meter-value-extension branch from 76e277e to b311fd1 Compare February 19, 2024 20:23
Pietfried
Pietfried requested changes Feb 22, 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.

Requested changes do not fix changes of this PR, but fix bugs that have been introduced earlier and I'd like to see fixed in this PR

modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
modules/OCPP201/OCPP201.cpp Outdated Show resolved Hide resolved
@hikinggrass
Add SignedMeterValue type powermeter types
7198a86 
This allows the encapsulation of additionaly metadata like the signing method, encoding method or the public key next to the signed meter data

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
leasve signing method tempty
6e24163 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Fix description
7e0b3f7 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Update signed meter value type usage in evse manager transaction types
76b0110 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Support new signed meter value type in OCPP 1.6
0fa185d 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Support new signed meter value type in OCPP 2.0.1 stop transaction
697067c 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Introduce signed units and move the signed meter value definition
6de528d 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Use session event type enum instead of string
de6afa4 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Fixed descriptions
569c8dc 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
WIP: signed meter values in OCPP201 module
9259c1c 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Signed meter values in OCPP201 module
dda2bfa 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
clang-format
98a3a25 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Simplify signed meter value support for now
30c7447 
Only support signed values from transaction or the powermeter, not individual measurands

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Add signed meter value support for periodic readings
55a56d9 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
hikinggrass and others added 4 commits February 22, 2024 18:03
@hikinggrass
Re-add individual signed meter values
2dfbbb9 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Interpret non-individual signed meter value as energy import Wh
1afcdb4 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass
Fix energy_Wh_import_signed handling
473c086 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass @Pietfried
Apply suggestions from code review
40f3775 
Co-authored-by: Piet Gömpel <37657534+Pietfried@users.noreply.github.com>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass force-pushed the kh-signed-meter-value-extension branch from 6f20c95 to 40f3775 Compare February 22, 2024 17:03
@hikinggrass
Fix optional access
d8d32da 
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass merged commit 9b66f99 into main Feb 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@golovasteek golovasteek golovasteek left review comments

@Pietfried Pietfried Pietfried approved these changes

@valentin-dimov valentin-dimov Awaiting requested review from valentin-dimov

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

Successfully merging this pull request may close these issues.
4 participants
@hikinggrass @golovasteek @Pietfried @valentin-dimov