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

New EVCC config isAliveAfterSession introduced #361

Merged
merged 3 commits into from
Jan 22, 2024
Merged

New EVCC config isAliveAfterSession introduced #361

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
719715a
New EVCC config isAliveAfterSession introduced
touchlinux Jan 10, 2024
162e4c3
EVCC terminates after session is finished
touchlinux Jan 22, 2024
607b03a
Merge branch 'master' into isAliveAfterSession
Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ Supported settings in EVCC configuration are given below:
| sdpRetryCycles | `1` | Indicates how often shall SDP (SECC Discovery Protocol) retries happen before reverting to using nominal duty cycle PWM-based charging |
| maxContractCerts | `3` | Maximum amount of contract certificates the EV stores. |
| maxSupportingPoints | `1024` | Indicates the maximum number of entries the EVCC supports within the sub-elements of a ScheduleTuple |
| isAliveAfterSession | `True` | Whether or not the EVCC keeps running after a charging session is finished |
Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies I didn't clarify this under issues. As the user can't proceed until the session is killed anyway, I don't see much value in making this a configurable field. The behaviour you have added to gracefully exit is the only change required. Do you have a use case in mind where keeping the session alive would be useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't have any use case in mind for keeping the session alive. I wanted to keep the session by default and stop the session when the configuration is used. However, it seems like the new configuration key is not necessary as stopping session can be the default behavior👍. I personally think, evcc should be terminated as a default after session, because evcc is not starting a new session again.



The SECC and EVCC have been tested together under:
Expand Down
7 changes: 5 additions & 2 deletions iso15118/evcc/comm_session_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ async def __init__. Therefore, we need to create a separate async

logger.info("Communication session handler started")

await wait_for_tasks(self.list_of_tasks)
await wait_for_tasks(self.list_of_tasks, asyncio.ALL_COMPLETED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this stop the session from ending if say for instance the udp server fails to start? As self.get_from_rcv_queue() wouldn't have exited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are right, asyncio.ALL_COMPLETED is not necessary. wait_for_tasks() default argument asyncio.FIRST_EXCEPTION should suffice.
"FIRST_EXCEPTION | The function will return when any future finishes by raising an exception. If no future raises an exception then it is equivalent to ALL_COMPLETED." - https://docs.python.org/3/library/asyncio-task.html#waiting-primitives
Confirmed by testing without it.


async def send_sdp(self):
"""
Expand Down Expand Up @@ -550,7 +550,10 @@ async def get_from_rcv_queue(self, queue: asyncio.Queue):
elif isinstance(notification, StopNotification):
await cancel_task(self.comm_session[1])
del self.comm_session
if not notification.successful:
if notification.successful:
if not self.config.is_alive_after_session:
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Following my suggestion from above, I think this would suffice. What do you think?

Suggested change
if notification.successful:
if not self.config.is_alive_after_session:
break
if notification.successful:
break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks good to me. Added another commit to fix all together.

else:
try:
await self.restart_sdp(True)
except SDPFailedError as exc:
Expand Down
1 change: 1 addition & 0 deletions iso15118/evcc/evcc_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ class EVCCConfig(BaseModel):
# ISO 15118-20 as well as PMaxSchedule and SalesTariff in ISO 15118-2).
# The SECC must not transmit more entries than defined in this parameter.
max_supporting_points: Optional[int] = Field(1024, alias="maxSupportingPoints")
is_alive_after_session: bool = Field(True, alias="isAliveAfterSession")

def load_raw_values(self):
# conversion of list of strings to enum types.
Expand Down