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

Improvement: Add get_cp_state method to iso15118 interface controller and include cp_status handler #77

Merged
merged 11 commits into from
Sep 22, 2022

Conversation

ikaratass
Copy link
Collaborator

@ikaratass ikaratass commented Jul 12, 2022

improvement on AB#2162

@ikaratass ikaratass requested review from tropxy and shalinnijel2 July 12, 2022 16:29
f"waiting for C2 or D2"
)
# wait for state C or D 250ms
await asyncio.sleep(0.25)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I understand this correctly - is this 250ms from the specification? Or do we actually have a little less than 3 seconds to detect state C2 or D2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is from spec req number V2G2-847. << The EV shall signal CP State C or D no later than 250ms after sending the first PowerDeliveryReq with ChargeProgress equals "Start" within V2G Communication SessionPowerDeliveryReq. >>

Copy link
Contributor

Choose a reason for hiding this comment

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

Okie dokie...a comment with the spec reference would be nice here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, actually, it was there, I just rearranged it.

f"waiting for C2 or D2"
)
# wait for state C or D 250ms
await asyncio.sleep(0.25)
Copy link
Contributor

Choose a reason for hiding this comment

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

Okie dokie...a comment with the spec reference would be nice here?

@ikaratass ikaratass changed the title Cs status Improvement: Add get_cp_state method to iso15118 interface controller and include cp_status handler Aug 1, 2022
@ikaratass ikaratass force-pushed the cs_status branch 2 times, most recently from 4a92c81 to 88d8bac Compare August 1, 2022 13:30
@ikaratass ikaratass requested a review from shalinnijel2 August 1, 2022 14:14
shalinnijel2
shalinnijel2 previously approved these changes Aug 2, 2022
@shalinnijel2
Copy link
Contributor

shalinnijel2 commented Aug 2, 2022

I missed to add this comment - would be good to add a reference to [V2G2-847] - "The EV shall signal CP State C or D no later than 250ms after sending the first PowerDeliveryReq with ChargeProgress equals "Start" within V2G Communication SessionPowerDeliveryReq." where we have the 250ms sleep.

unitest for cp_state
@pytest.mark.parametrize(
"get_state_return_value, expected_next_state",
[
(CpState.C2, CurrentDemand),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to check C1, D1 ends up in Terminate as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -1554,6 +1565,32 @@ async def process_message(

self.expecting_power_delivery_req = False

async def wait_for_state_c_or_d(self) -> bool:
STATE_C_TIMEOUT = 0.25
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to mention [V2G2-847] as the reason for 0.25.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@ikaratass ikaratass merged commit 5e20e5a into master Sep 22, 2022
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.

2 participants