Add EV side Se/Deserialization for session_setup and unit tests #81
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cienporcien
requested review from
a-w50,
SebaLukas and
MarzellT
as code owners
SebaLukas
requested changes
Feb 17, 2025
There was a problem hiding this comment.
Looking good! I just have a few small things to say:
- Look at my comments in the files.
LF Energyprojects needsigned-off-bycommits. Please addsigned-off-byto your commit: https://git-scm.com/docs/git-commit#Documentation/git-commit.txt--s- Linting job is failing. Please run
clang-formaton your changes. I will provide you the command in the next comment. - I will check why the CI job
deploy html code coverageis failling.
There was a problem hiding this comment.
Please remove your comments.
There was a problem hiding this comment.
Also the comments in the code file?
src/iso15118/message/session_setup.cpp
test/exi/cb/iso20/session_setup.cpp
Outdated
Comment on lines
12
to
53
| //Note that the doc-raw as well as the verbose v2g representation come from the dsV2Gwireshark plugin | ||
| //monitoring the communication between the EV and the EVSE in a typical EVerest ISO 15118-20 DC SIL session using config-sil-dc-d20 | ||
| //The verbose v2g representation is not used in the tests, but is included for reference | ||
|
|
||
| /* V2G Message | ||
| WARNING: Preliminary ISO 15118-20 support! | ||
| Metadata | ||
| EXI: 808C0400000000000000000C9F9C2BD0620B2BA6A4AB1899199A1A9B1B9C1C9820A121A222AC00 | ||
| Message: SessionSetupReq | ||
| Decoded XML […]: <?xml version="1.0" encoding="UTF-8"?><ns1:SessionSetupReq xmlns:ns1="urn:iso:std:iso:15118:-20:CommonMessages" xmlns:ns2="urn:iso:std:iso:15118:-20:CommonTypes"><ns2:Header><ns2:SessionID>0000000000000000</ns2:SessionID | ||
| Message Validation: Successful | ||
| Schema: urn:iso:std:iso:15118:-20:CommonMessages | ||
| SessionSetupReq | ||
| [XML Attributes: xmlns:ns1="urn:iso:std:iso:15118:-20:CommonMessages" xmlns:ns2="urn:iso:std:iso:15118:-20:CommonTypes"] | ||
| Header | ||
| SessionID: 0000000000000000 | ||
| TimeStamp: 1739635913 | ||
| EVCCID: WMIV1234567890ABCDEX */ | ||
|
|
||
| /* unsigned char bytes[] = {0x80, 0x8c, 0x4, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc, 0x9f, 0x9c, 0x2b, 0xd0, 0x62, 0xb, | ||
| 0x2b, 0xa6, 0xa4, 0xab, 0x18, 0x99, 0x19, 0x9a, 0x1a, 0x9b, 0x1b, 0x9c, 0x1c, 0x98, 0x20, 0xa1, 0x21, 0xa2, 0x22, 0xac, 0x0}; | ||
| */ | ||
|
|
||
| /* V2G Message | ||
| WARNING: Preliminary ISO 15118-20 support! | ||
| Metadata | ||
| EXI: 809004177D0C4A6E3DC8088C9F9C2BD062040451114A9413960A914C4C8CCD0D4A8C40 | ||
| Message: SessionSetupRes | ||
| Decoded XML […]: <?xml version="1.0" encoding="UTF-8"?><ns1:SessionSetupRes xmlns:ns1="urn:iso:std:iso:15118:-20:CommonMessages" xmlns:ns2="urn:iso:std:iso:15118:-20:CommonTypes"><ns2:Header><ns2:SessionID>2EFA1894DC7B9011</ns2:SessionID | ||
| Message Validation: Successful | ||
| Schema: urn:iso:std:iso:15118:-20:CommonMessages | ||
| SessionSetupRes | ||
| [XML Attributes: xmlns:ns1="urn:iso:std:iso:15118:-20:CommonMessages" xmlns:ns2="urn:iso:std:iso:15118:-20:CommonTypes"] | ||
| Header | ||
| SessionID: 2EFA1894DC7B9011 | ||
| TimeStamp: 1739635913 | ||
| ResponseCode: OK_NewSessionEstablished | ||
| EVSEID: DE*PNX*E12345*1 | ||
| */ | ||
|
|
||
| /* unsigned char bytes[] = {0x80, 0x90, 0x4, 0x17, 0x7d, 0xc, 0x4a, 0x6e, 0x3d, 0xc8, 0x8, 0x8c, 0x9f, 0x9c, 0x2b, 0xd0, 0x62, | ||
| 0x4, 0x4, 0x51, 0x11, 0x4a, 0x94, 0x13, 0x96, 0xa, 0x91, 0x4c, 0x4c, 0x8c, 0xcd, 0xd, 0x4a, 0x8c, 0x40}; */ |
There was a problem hiding this comment.
Of course I understand why you need a Json/XML representation of the Exi stream for the unit tests. To write the tests, I used the Json message myself.
Nevertheless, I would not commit the Json/XML representation. Due to the individual catch2 assertions, there is a duplication with the Json/XML representation. Theoretically, you can read out how the Json/XML message is structured from the individual test steps.
There was a problem hiding this comment.
I was just following the advice of Moritz in Zulip... Personally, I like to have the comments so I can remember what is being tested. I imagine that a more complicated message might have more parameter combinations and more test cases, where these sort of comments could help keep things straight. maybe we can discuss in the car comm meeting?
There was a problem hiding this comment.
Okay, then lets have a discussion in the car WG meeting 👍
|
In our EVerest projects we use a specific And commit the changes. |
|
Please update your branch with the latest main commits. @andistorm fixed the |
Signed-off-by: Roger Bedell <[email protected]>
Signed-off-by: Roger Bedell <[email protected]>
|
Because it was a mess on my end, adding the DCO and also the pull request was done from my main instead of a branch. I couldn´t figure out how to clean it up without making a new pull request. Sorry for the confusion. Live and learn.
…________________________________
From: Sebastian Lukas ***@***.***>
Sent: Tuesday, February 18, 2025 8:11 AM
To: EVerest/libiso15118 ***@***.***>
Cc: Roger Bedell ***@***.***>; State change ***@***.***>
Subject: Re: [EVerest/libiso15118] Add EV side Se/Deserialization for session_setup and unit tests (PR #81)
[SebaLukas]SebaLukas left a comment (EVerest/libiso15118#81)<#81 (comment)>
Why did you close this PR and open a new PR with the same content?
—
Reply to this email directly, view it on GitHub<#81 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7DXQUZLZXKDQY3WBRIQCIT2QLMJFAVCNFSM6AAAAABXGU6TGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRUG44DQMJXHA>.
You are receiving this because you modified the open/close state.
________________________________
From: Sebastian Lukas ***@***.***>
Sent: Tuesday, February 18, 2025 8:11 AM
To: EVerest/libiso15118 ***@***.***>
Cc: Roger Bedell ***@***.***>; State change ***@***.***>
Subject: Re: [EVerest/libiso15118] Add EV side Se/Deserialization for session_setup and unit tests (PR #81)
[SebaLukas]SebaLukas left a comment (EVerest/libiso15118#81)<#81 (comment)>
Why did you close this PR and open a new PR with the same content?
—
Reply to this email directly, view it on GitHub<#81 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7DXQUZLZXKDQY3WBRIQCIT2QLMJFAVCNFSM6AAAAABXGU6TGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNRUG44DQMJXHA>.
You are receiving this because you modified the open/close state.
|
|
Thats alright! I read your message in Zulip too late 😅 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes
Add EV side Se/Deserialization for session_setup and unit tests
Issue ticket number and link
#63 Session Setup Conversion Functions
Checklist before requesting a review