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

Refactored OCMF powermeter transaction handling #640

Merged
merged 15 commits into from
May 2, 2024
Merged

Refactored OCMF powermeter transaction handling #640

merged 15 commits into from
May 2, 2024

Conversation

Pietfried
Copy link
Contributor

@Pietfried Pietfried commented Apr 18, 2024 •
edited
Loading

Describe your changes

Refactored OCMF powermeter transaction handling:

  • Powermeter interface now contains proper user identification arguments (IS, IT, IF, IT, ID) to start the transaction
  • Refactored EvseManager and LemDCBM400600 modules to address the changed types
  • Added module config params for LemDCBM400600 : cable_id, tariff_id
  • Fixed test cases in LemDCBM400600 due to changes made

Why changes are required:
The cable_id is no OCMF parameter but it is specific to the LEM powermeter and shall therefore not be an argument within the TransactionReq type. A tariff_id is also no OCMF parameter (but Tariff Text (TT) is) and in the LEM communication protocol it is unclear how this parameter shall be set. This might require further changes in the LEM module, but it doesnt belong to the TransactionReq type.

Issue ticket number and link

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

@Pietfried Pietfried force-pushed the feature/extend-ocmf-data branch from 19a80ca to 792a509 Compare April 18, 2024 18:13
@Pietfried
Refactored OCMF powermeter transaction handling:
e42515c 
* Powermeter interface now contains proper user identification arguments (IS, IT, IF, IT, ID) to start the transaction
* Refactored EvseManager and LemDCBM400600 modules to address the changed types
* Added module config params for LemDCBM400600 : cable_id, tariff_id
* Fixed test cases in LemDCBM400600 due to changes made

Signed-off-by: pietfried <[email protected]>
@Pietfried Pietfried force-pushed the feature/extend-ocmf-data branch from 792a509 to e42515c Compare April 18, 2024 18:18
a-w50
a-w50 requested changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@a-w50 a-w50 left a comment

Choose a reason for hiding this comment

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

Missing some documentation about why for ex. cable_id and tariff_id have been removed from the powermeter type and are now only local config options for the LEM powermeter

@Pietfried Pietfried force-pushed the feature/extend-ocmf-data branch from 9d735d6 to c669ff6 Compare April 19, 2024 11:21
@Pietfried
Copy link
Contributor Author

Missing some documentation about why for ex. cable_id and tariff_id have been removed from the powermeter type and are now only local config options for the LEM powermeter

The cable_id is no OCMF parameter but it is specific to the LEM powermeter and shall therefore not be an argument within the TransactionReq type. A tariff_id is also no OCMF parameter (but Tariff Text (TT) is) and in the LEM communication protocol it is unclear how this parameter shall be set. This might require further changes in the LEM module, but it doesnt belong to the TransactionReq type.

@Pietfried
fix RsIskraMeter tests
097a571 
Signed-off-by: pietfried <[email protected]>
@Pietfried Pietfried requested a review from a-w50 April 19, 2024 11:48
@a-w50
Copy link
Contributor

a-w50 commented Apr 19, 2024

Missing some documentation about why for ex. cable_id and tariff_id have been removed from the powermeter type and are now only local config options for the LEM powermeter

The cable_id is no OCMF parameter but it is specific to the LEM powermeter and shall therefore not be an argument within the TransactionReq type. A tariff_id is also no OCMF parameter (but Tariff Text (TT) is) and in the LEM communication protocol it is unclear how this parameter shall be set. This might require further changes in the LEM module, but it doesnt belong to the TransactionReq type.

That is exactly what I would like to see in the commit or PR message, so that we're able to reason about why those changes happened.

@Pietfried Pietfried marked this pull request as ready for review April 22, 2024 07:39
@SirVer
Copy link
Contributor

SirVer commented Apr 22, 2024

@Pietfried The MR description explains the "what" happened, but not the "why", please extend, i.e. I have a similar question than @a-w50.

@Pietfried
Copy link
Contributor Author

Pietfried commented Apr 22, 2024 •
edited
Loading

@Pietfried The MR description explains the "what" happened, but not the "why", please extend, i.e. I have a similar question than @a-w50.

Hi @SirVer , this response to @a-w50 request explains "why" the changes are required: #640 (comment) .

Please have a look at the Rust build, which is currently failing. It looks like minItems and maxItems keywords are not supported in the rust part.

golovasteek
golovasteek reviewed Apr 22, 2024
View reviewed changes
@SirVer
Copy link
Contributor

SirVer commented Apr 22, 2024

Hi @SirVer , this response to @a-w50 request explains "why" the changes are required: #640 (comment) .

Yes, I saw that @Pietfried. My statement was more that this should go into the MR description, because it is important context for posterity and should not be buried in the discussion.

@Pietfried
add to_string
9b76183 
Signed-off-by: pietfried <[email protected]>
@Pietfried
fix build
a782858 
Signed-off-by: pietfried <[email protected]>
@Pietfried
Adressed comment to change type of identification_status bool to OCMF…
5d3e1bf 
…UserIdentificationStatus

Signed-off-by: pietfried <[email protected]>
@Pietfried Pietfried requested a review from dorezyuk April 22, 2024 20:07
SirVer
SirVer approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@SirVer SirVer left a comment

Choose a reason for hiding this comment

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

Great, thanks for addressing the boolean. The resulting code is much better I think!

SebaLukas
SebaLukas approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

👍

@a-w50
Copy link
Contributor

a-w50 commented Apr 30, 2024

@Pietfried The MR description explains the "what" happened, but not the "why", please extend, i.e. I have a similar question than @a-w50.

Exactly, that's what I was trying to say.

@Pietfried Pietfried merged commit b4194c8 into main May 2, 2024
4 of 5 checks passed
@Pietfried Pietfried deleted the feature/extend-ocmf-data branch May 2, 2024 08:58
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

@SirVer SirVer SirVer approved these changes

@dorezyuk dorezyuk dorezyuk approved these changes

@SebaLukas SebaLukas SebaLukas approved these changes

@corneliusclaussen corneliusclaussen corneliusclaussen approved these changes

@a-w50 a-w50 a-w50 approved these changes

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

Assignees

@corneliusclaussen corneliusclaussen

@a-w50 a-w50

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Pietfried @a-w50 @SirVer @golovasteek @dorezyuk @SebaLukas @corneliusclaussen