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 JsEvManager as a JsCarSimulator replacement #549

Merged
merged 1 commit into from Apr 17, 2024
Merged

Adding the JsEvManager as a JsCarSimulator replacement #549

merged 1 commit into from Apr 17, 2024

Conversation

SebaLukas
Copy link
Contributor

The aim is to rewrite the JsCarSimulator so that the new JsEvManager can also run on "real" hardware. The EV counterpart of the EvseManager module, so to speak.

For this purpose, a new ev_board_support interface and a suitable YetiEvDriver were written. The yeti_simulation_control was removed.

The new JsEvManager now also uses the ev_slac interface. The ev slac state machine and the JsSlacSimulator module have been adapted for this purpose.

@SebaLukas SebaLukas force-pushed the feature/adding-JsEvManager branch 2 times, most recently from 93d20b9 to 0659153 Compare March 6, 2024 07:47
@SebaLukas SebaLukas marked this pull request as ready for review March 6, 2024 18:58
@corneliusclaussen corneliusclaussen added the include-in-release Tag that this PR should be included in the current merge window for the upcoming release if possible label Mar 22, 2024
corneliusclaussen
corneliusclaussen approved these changes Mar 22, 2024
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.

lgtm

@SebaLukas
Copy link
Contributor Author

SebaLukas commented Mar 22, 2024 •
edited

Todo from my side before merge: 1. Fix integration test 2. squash all commits together

@SebaLukas SebaLukas added post-release Tag that this PR should not go into the current merge window for the upcoming release and removed include-in-release Tag that this PR should be included in the current merge window for the upcoming release if possible labels Mar 22, 2024
@SebaLukas
Copy link
Contributor Author

I found a few things that still need to be changed here (node-red flows changes, PnC tests). Therefore I would suggest to move the merge after the release.

@SebaLukas SebaLukas marked this pull request as draft March 22, 2024 14:43
@SebaLukas SebaLukas marked this pull request as ready for review April 15, 2024 11:34
@SebaLukas SebaLukas force-pushed the feature/adding-JsEvManager branch 2 times, most recently from e25ef5a to d0fd603 Compare April 15, 2024 13:09
@SebaLukas SebaLukas removed the post-release Tag that this PR should not go into the current merge window for the upcoming release label Apr 15, 2024
hikinggrass
hikinggrass requested changes Apr 15, 2024
View reviewed changes
config/config-sil-ocpp201-pnc.yaml Outdated Show resolved Hide resolved
modules/simulation/JsEvManager/index.js Show resolved Hide resolved
a-w50
a-w50 requested changes Apr 17, 2024
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.

Some more remarks:

  • would it be possible, to have the slac changes (lib/staging/slac) in a separate PR?
  • naming could be a bit more consistent: YetiEVDriver and evyeti_comms
  • moving the evSerial things into a separate library would reduce code duplication

modules/PySlacLoopback/manifest.yaml Outdated Show resolved Hide resolved
modules/PySlacLoopback/module.py Outdated Show resolved Hide resolved
@hikinggrass
Copy link
Contributor

hikinggrass commented Apr 17, 2024 •
edited

  • moving the evSerial things into a separate library would reduce code duplication

There's Everest::Serial in everest-framework which was written with the intention to have a common implementation, but that hasn't been widely used yet (I think only in the pn532 rfid reader driver)

@SebaLukas
Copy link
Contributor Author

I think separating the lib/slac stuff from this PR into another PR is possible. I can do that right now

@SebaLukas SebaLukas mentioned this pull request Apr 17, 2024
Merged
3 tasks
@SebaLukas @a-w50
@corneliusclaussen @Pietfried
Adding the JsEvManager as a JsCarSimulator replacement
0ac1776 
Signed-off-by: Sebastian Lukas <sebastian.lukas@pionix.de>
Co-authored-by: aw <aw@pionix.de>
Co-authored-by: Cornelius Claussen <cc@pionix.de>
Co-authored-by: pietfried <pietgoempel@gmail.com>
@SebaLukas SebaLukas merged commit 4d270c9 into main Apr 17, 2024
4 of 5 checks passed
@SebaLukas SebaLukas deleted the feature/adding-JsEvManager branch April 17, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hikinggrass hikinggrass hikinggrass approved these changes

@Pietfried Pietfried Pietfried approved these changes

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

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

Assignees

@hikinggrass hikinggrass

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@SebaLukas @hikinggrass @Pietfried @corneliusclaussen @a-w50