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/dc states till session stop #21

Conversation

lukaslombriserdesignwerk
Copy link
Contributor

@lukaslombriserdesignwerk lukaslombriserdesignwerk commented Mar 2, 2022

No description provided.

@lukaslombriserdesignwerk lukaslombriserdesignwerk force-pushed the feature/dc_states_till_session_stop branch from 4ca3bf9 to be53227 Compare March 7, 2022 08:30
@lukaslombriserdesignwerk lukaslombriserdesignwerk force-pushed the feature/dc_states_till_session_stop branch from be53227 to a3b0683 Compare March 18, 2022 13:57
Copy link
Contributor

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

First my sincere apologies for not reviewing this earlier, but as I suspected, it took me a considerable amount of time, time that it has been hard to get lately.

That said, thank you so much for this great contribution, it really makes us happy to see how fast and well you understood the code architecture and provided such good quality. There are quite a few notes that I have left and that we need to discuss, so please check them and take your time and come back to me if you have any questions.

Cheers ;)

Copy link
Contributor

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

Lukas I think you havent pushed your last changes. can you check that?
Also you need to rebase your code against master

@shalinnijel2
Copy link
Contributor

Hi Lukas, main now has the fix for the issue you commented about WeldingDetectionRes exi encoding failing as well.

Copy link
Contributor

@shalinnijel2 shalinnijel2 left a comment

Choose a reason for hiding this comment

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

The make file has flake8 and black utilities to help with formatting.
make flake8
make black
black iso15118/

@lukaslombriserdesignwerk lukaslombriserdesignwerk force-pushed the feature/dc_states_till_session_stop branch from bf54fef to 064bfbf Compare March 24, 2022 13:51
@lukaslombriserdesignwerk
Copy link
Contributor Author

Hi Lukas, main now has the fix for the issue you commented about WeldingDetectionRes exi encoding failing as well.

Thank you Shalin. Now it's working.

Copy link
Contributor

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

cool stuff, thanks for the improvements

if not self.cable_check_req_was_reveived:
self.comm_session.evse_controller.set_cable_check()
self.cable_check_req_was_reveived = True
self.comm_session.evse_controller.set_ev_soc(cable_check_req.dc_ev_status.ev_ress_soc)
Copy link
Contributor

Choose a reason for hiding this comment

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

oh no, that I got it and is fine, I meant the set_ev_soc, why do we need that method here?

Copy link
Contributor

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

A few more notes ;)

if not self.cable_check_req_was_reveived:
self.comm_session.evse_controller.set_cable_check()
self.cable_check_req_was_reveived = True
self.comm_session.evse_controller.set_ev_soc(cable_check_req.dc_ev_status.ev_ress_soc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see...I think we should have a structure that holds the current EV information, like present current, present voltage, SoC and here as an example, we would call update_ev_data(soc=cable_check_req.dc_ev_status.ev_ress_soc)
Then, the method updates some data structure like a dataclass:

@daclass
class ev_data_context:
    dc_current: Optional[int] = None
    dc_voltage: Optional[int] = None
    ac_current: Optional[dict] = None # {"l1": 10, "l2": 10, "l3": 10}
    ac_voltage: Optional[dict] = None # {"l1": 230, "l2": 230, "l3": 230}
    soc: Optional[int] = None # 0-100

the method would accept more arguments which would be optional
what do you think?

@lukaslombriserdesignwerk
Copy link
Contributor Author

I don't know why, but I can't comment the conversation about the set_soc function.

i made a dataclass ev_data_context and a method update_ev_data. Is it how you figured?
But I think in DC-Charging we just use the SOC, because we already know the current and voltage from the EVSE.
And I also checked the ChargingStatusReq Message for AC charging. In this message no EV information are provided.

What do you think?

@lukaslombriserdesignwerk
Copy link
Contributor Author

A few more improvements :)

@tropxy
Copy link
Contributor

tropxy commented Mar 30, 2022

I don't know why, but I can't comment the conversation about the set_soc function.

i made a dataclass ev_data_context and a method update_ev_data. Is it how you figured? But I think in DC-Charging we just use the SOC, because we already know the current and voltage from the EVSE. And I also checked the ChargingStatusReq Message for AC charging. In this message no EV information are provided.

What do you think?

Yes, I think there are some bugs in Github recently and prevent comments in certain spots.
Regarding your approach: yes, that is more or less how I figured it out; for AC charging I am aware there is no way to get the SoC, but that is ok, that is why in my example I added the fields as Optional

Copy link
Contributor

@tropxy tropxy left a comment

Choose a reason for hiding this comment

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

just added a few more notes

@lukaslombriserdesignwerk
Copy link
Contributor Author

lukaslombriserdesignwerk commented Mar 31, 2022

Hi André

I did some improvements.
And I also reformated the code to fullfill flake8 and black.
But here it looks like code-quality-check is still not passed (I don't know if it has something to do with black and flake8).

From tomorrow till easter I'll be on holidays. If you have any questions Martin is the man.

And until tomorrow morning, I could also do some improvements for the pullrequests :)

@tropxy
Copy link
Contributor

tropxy commented Apr 1, 2022

Hi André

I did some improvements. And I also reformated the code to fullfill flake8 and black. But here it looks like code-quality-check is still not passed (I don't know if it has something to do with black and flake8).

From tomorrow till easter I'll be on holidays. If you have any questions Martin is the man.

And until tomorrow morning, I could also do some improvements for the pullrequests :)

Hi Lukas, I have fixed the code quality and the tests of the code.
I had to do some changes on how we create the EXI instance to make the code pass, but it works now.

I think this code can be merged now ;)

Just need to figure out why are not the CI/CD job running in this PR

@tropxy tropxy merged commit be38593 into EcoG-io:master Apr 1, 2022
tropxy added a commit that referenced this pull request Apr 12, 2022
* Updated branch to use jar with DINSpec support

* Updated timeout values for DINSpec. Minor formatting to stop black from complaining.

* Added reference schema files.

* DIN SPEC schema representation - first draft

* Updated din_spec/datatypes.py

* Updated comm_session.py and exi_codec to accept DINSPEC message types.

* Updated abstract method definition in states.py and updated associated implementations in -2 and -20 on both secc and evcc sides.

* Setting preferred protocol to DIN SPEC on evcc side for now (to be changed later.)

* Fixed flake8 and black errors. Removed message types not used by DINSpec.

* Fixed flake8 and black errors. Moved EnergyTransferMode to common enums.

* Initial checkin - DIN SPEC states for EVCC and SECC.

* Code format mods to pass tests on GitHub.

* Support for remaining states till SessionStop

* Refactoring to move common classes between -2 dc and dinspec messages.

* Updated EXICodec.jar with fix for WeldingDetectionRes message (#27)

* feat: Addressed some of the comments on PR(AB#1439)

* feat: DIN SPEC should not be listed in supported protocols if use_tls set to false (AB#1439)

Also, Major version for DIN SPEC is 2 (in supportedAppProtocolReq).

* feat: Add timeout handling to CableCheck, renamed ServiceAndPaymentSelection to ServicePaymentSelection

* feat: Added tests. Fixed identified issue with CableCheck.

* Feature/dc states till session stop (#21)

* ability to add existing EVSEController

* Separate config validataion in order to have another way to configure iso15118

* Added some some abstract methods for DC-Charging

* Completed State CableCheck. But is not tested

* Added State Precharge

* Implemented CableCheck in EVCC states.
Reached State Precharge with EVCC/EVSE simulation

* simulatin with SECC/EVCC works until beginning of CurrentDemand

* simulatin with SECC/EVCC works until beginning of CurrentDemand

* Simulation with SECC/EVCC reaches CurrentDemand and is constantly sending CurrendDemandReq/Res

* Just Layout changes and some comments. No changes in productive Code

* First sucessfull unittest

* wip DC_state current_demand and pytests

* DC-States until Welding-Detection.

* Added some pytests

* added timeouts for CableCheck and Precharge

* some changes in interface

* implemented feedback from André

* review from Shalin

* improvements #2 from Pullrequest

* improvements # from Pullrequest

* improvements #4 from pullrequest

* reformat code to fullfill black and flake8

* Decreased the number of Precharge cycles to avoid the Timeout. Reformated a few error messages around the code

* Reformated the Exi Codec class creation as its former use complicated testing; fixed the tests

* reformated the code

* fixed flake8 issues

* Revert "Reformated the Exi Codec class creation as its former use complicated testing; fixed the tests"

This reverts commit 1b6e798.

* fixed bugs inserted after the git reverse

* fixed the tests by patching the to_exi method

* flaked 8 the code

* marked tests as async for future compatability and added  mode on the pytest init config as specified here: https://github.com/pytest-dev/pytest-asyncio\#modes

* added conftest

* fixed flake eror

Co-authored-by: Martin Bachmann <[email protected]>
Co-authored-by: tropxy <[email protected]>

* feat: More fixes. (AB#1439)

* Updated timeout values for DINSpec. Minor formatting to stop black from complaining.

* Added reference schema files.

* DIN SPEC schema representation - first draft

* Updated din_spec/datatypes.py

* Updated comm_session.py and exi_codec to accept DINSPEC message types.

* Updated abstract method definition in states.py and updated associated implementations in -2 and -20 on both secc and evcc sides.

* Setting preferred protocol to DIN SPEC on evcc side for now (to be changed later.)

* Fixed flake8 and black errors. Removed message types not used by DINSpec.

* Fixed flake8 and black errors. Moved EnergyTransferMode to common enums.

* Initial checkin - DIN SPEC states for EVCC and SECC.

* Code format mods to pass tests on GitHub.

* Support for remaining states till SessionStop

* Refactoring to move common classes between -2 dc and dinspec messages.

* feat: Addressed some of the comments on PR(AB#1439)

* feat: DIN SPEC should not be listed in supported protocols if use_tls set to false (AB#1439)

Also, Major version for DIN SPEC is 2 (in supportedAppProtocolReq).

* feat: Add timeout handling to CableCheck, renamed ServiceAndPaymentSelection to ServicePaymentSelection

* feat: Added tests. Fixed identified issue with CableCheck.

* feat: More fixes. (AB#1439)

* feat: Rebased to master. Re-added missed (AB#1439)

* feat: Updated failing tests(AB#1439)

* feat: Addressed comments in PR (AB#1439)

* feat: Fixed failing tests(AB#1439)

* feat: added timeout checkj to CableCheck, WeldingDetection, updated tests., Also added timeout check to WeldingDetectionRes in -2

* feat: Addressed comments on PR. Added missing docstrings.(AB#1439)

* feat: Addressed comments on PR(AB#1439)

* feat: Renamed shared/messages/datatypes_iso15118_2_dinspec to shared/messages/datatypes. Fixed dinspec related mypy issues.

* feat: Fixed flake8 errors

* feat: Updated tests following file rename (AB#1439)

Co-authored-by: lukaslombriserdesignwerk <[email protected]>
Co-authored-by: Martin Bachmann <[email protected]>
Co-authored-by: tropxy <[email protected]>
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.

4 participants