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

Adding the PyEvJosev module to replace the JsCarV2G module #261

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Adding the PyEvJosev module to replace the JsCarV2G module #261

merged 1 commit into from
Jul 4, 2023

Conversation

SebaLukas
Copy link
Contributor

@SebaLukas SebaLukas commented Jun 20, 2023 •
edited
Loading

@SebaLukas SebaLukas force-pushed the sl-adding-pyevjosev branch from 8ea6a5b to 7086e55 Compare June 20, 2023 07:15
@SebaLukas
Copy link
Contributor Author

SebaLukas commented Jun 20, 2023 •
edited
Loading

Missing: The private contract cert key is not saved in the correct format (.pem) during the certificate installation process.
Done!

sdrabb
sdrabb reviewed Jun 20, 2023
View reviewed changes
@SebaLukas SebaLukas force-pushed the sl-adding-pyevjosev branch from 0b28cfb to 3517d17 Compare June 21, 2023 08:16
@sdrabb
Copy link

sdrabb commented Jun 22, 2023 •
edited
Loading

@SebaLukas what about EVSlac side? Are you working on it?
ok I found it https://github.com/EVerest/everest-core/tree/feature/ev_slac

Thanks

@SebaLukas
Copy link
Contributor Author

@sdrabb we are working on it :)

@SebaLukas SebaLukas force-pushed the sl-adding-pyevjosev branch 2 times, most recently from 6ba456e to 3dfc4e7 Compare June 27, 2023 12:54
@SebaLukas SebaLukas marked this pull request as ready for review June 27, 2023 12:57
@SebaLukas
Copy link
Contributor Author

For PnC testing I will provide a extra pnc config

@SebaLukas SebaLukas requested a review from a-w50 June 28, 2023 07:06
a-w50
a-w50 reviewed Jun 28, 2023
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.

See my comments, depending on if we want to postpone these issues or not, this PR can be approved or not

modules/PyEvJosev/module.py
Comment on lines +11 to +19
JOSEV_WORK_DIR = Path(__file__).parent / '../../3rd_party/josev'
sys.path.append(JOSEV_WORK_DIR.as_posix())

from iso15118.evcc import EVCCHandler
from iso15118.evcc.controller.simulator import SimEVController
from iso15118.evcc.evcc_config import EVCCConfig
from iso15118.evcc.everest import context as JOSEV_CONTEXT
from iso15118.shared.exificient_exi_codec import ExificientEXICodec
from iso15118.shared.settings import set_PKI_PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer having this solved differently / we need to come up with a best practice how external python modules are distributed to the system

  • either by copying them directly
  • or by pip
  • wheels?

modules/PyEvJosev/utilities.py
Comment on lines +1 to +24
# SPDX-License-Identifier: Apache-2.0
# Copyright 2020 - 2023 Pionix GmbH and Contributors to EVerest
import logging
import netifaces

from everest.framework import log

from iso15118.evcc.evcc_config import EVCCConfig
from iso15118.shared.utils import load_requested_protocols

class EverestPyLoggingHandler(logging.Handler):

def __init__(self):
logging.Handler.__init__(self)

def emit(self, record):
msg = self.format(record)

log_level: int = record.levelno
if log_level == logging.CRITICAL:
log.critical(msg)
elif log_level == logging.ERROR:
log.error(msg)
elif log_level == logging.WARNING:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is, I guess, exactly the same as in the EVSE part. I'd prefer removing this duplication by creating a shared library for this inside 'everest-core/lib' - again we would need to think on how to package and distribute/install that later on

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's the same code

@SebaLukas
Copy link
Contributor Author

Those are both good points. However, I would not solve them in this PR, but first discuss them and then possibly solve them in a separate PR.

a-w50
a-w50 approved these changes Jun 29, 2023
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.

Fine, please rebase and squash

@SebaLukas SebaLukas force-pushed the sl-adding-pyevjosev branch 2 times, most recently from 812720b to 627a79e Compare June 30, 2023 09:04
@corneliusclaussen corneliusclaussen marked this pull request as draft July 4, 2023 09:16
@corneliusclaussen
Copy link
Contributor

Converted to draft, SIL is not working at the moment for ISO. This branch should fix SIL before merging

@SebaLukas @corneliusclaussen
Adding the PyEvJosev module to replace the JsCarV2G module
96bf365 
Signed-off-by: Sebastian Lukas <sebastian.lukas@pionix.de>
@corneliusclaussen corneliusclaussen marked this pull request as ready for review July 4, 2023 12:34
corneliusclaussen
corneliusclaussen approved these changes Jul 4, 2023
View reviewed changes
Copy link
Contributor

@corneliusclaussen corneliusclaussen left a comment

Choose a reason for hiding this comment

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

I split the AC and DC nodered flows into two seperate files, this is less confusing

@corneliusclaussen corneliusclaussen merged commit 452e98b into main Jul 4, 2023
@corneliusclaussen corneliusclaussen deleted the sl-adding-pyevjosev branch July 4, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdrabb sdrabb sdrabb approved these changes

@corneliusclaussen corneliusclaussen corneliusclaussen approved these changes

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

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

@mooraby mooraby Awaiting requested review from mooraby

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SebaLukas @sdrabb @corneliusclaussen @a-w50