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 EvManager module #643

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Adding EvManager module #643

merged 1 commit into from
Jul 3, 2024

Conversation

MarzellT
Copy link
Member

@MarzellT MarzellT commented Apr 19, 2024 •
edited
Loading

Describe your changes

  • rewrites the EvManager car simulator in C++
  • adds unit tests for the added functions
  • fixes a few small issues in the nodered config
  • small fixes in the JsEvManager

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

@MarzellT MarzellT self-assigned this Apr 19, 2024
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch from 5cf1891 to bc9f5e2 Compare April 19, 2024 13:50
a-w50
a-w50 reviewed Apr 19, 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.

Nicely done. Concerning the style - we don't have strict rules, but I think the majority of code is using only CamelCase for classes/structs and snake_case for variables and filenames. Constants are usually written as SOME_CONSTANT (also for static constexpr vars).

modules/CMakeLists.txt Outdated Show resolved Hide resolved
modules/EvManager/main/RegisteredCommand.hpp Outdated Show resolved Hide resolved
modules/EvManager/main/RegisteredCommand.hpp Outdated Show resolved Hide resolved
modules/EvManager/main/RegisteredCommand.hpp Outdated Show resolved Hide resolved
modules/EvManager/main/RegisteredCommand.hpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
@MarzellT MarzellT marked this pull request as draft April 23, 2024 12:31
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch from baa23b8 to d030106 Compare April 24, 2024 13:09
@barsnick barsnick changed the title Draft: addding cpp EvManager module Draft: adding cpp EvManager module Apr 24, 2024
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch 7 times, most recently from 74de9b9 to 6b661b8 Compare April 30, 2024 12:55
@MarzellT MarzellT changed the title Draft: adding cpp EvManager module adding cpp EvManager module Apr 30, 2024
@MarzellT MarzellT marked this pull request as ready for review April 30, 2024 12:56
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch 2 times, most recently from 1ce0fee to 8607282 Compare April 30, 2024 14:12
SebaLukas
SebaLukas requested changes May 2, 2024
View reviewed changes
Copy link
Contributor

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

A really well done from me too. But a few things should still be changed.

modules/EvManager/main/CommandRegistry.cpp Outdated Show resolved Hide resolved
config/config-sil-dc.yaml Outdated Show resolved Hide resolved
modules/EvManager/CMakeLists.txt Outdated Show resolved Hide resolved
modules/EvManager/CMakeLists.txt Outdated Show resolved Hide resolved
modules/EvManager/main/SimCommand.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulatorImpl.cpp Outdated Show resolved Hide resolved
@SebaLukas SebaLukas changed the title adding cpp EvManager module Adding cpp EvManager module May 2, 2024
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch from f7628cb to b7d6298 Compare May 6, 2024 15:16
@SebaLukas SebaLukas force-pushed the tm-adding-evmanager branch from 8c7c68c to eddd637 Compare May 8, 2024 10:48
@SebaLukas SebaLukas changed the title Adding cpp EvManager module Adding EvManager module May 14, 2024
@SebaLukas
Copy link
Contributor

SebaLukas commented May 14, 2024 •
edited
Loading

Todos before merging:

  • Adding documentation
  • Testing
  • Function names and variables in snake_case

@SebaLukas SebaLukas force-pushed the tm-adding-evmanager branch from 2610a92 to 3c3c54c Compare May 21, 2024 08:20
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch 4 times, most recently from d41c741 to 813bc27 Compare July 1, 2024 08:28
SebaLukas
SebaLukas approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

Very good 👍

@MarzellT MarzellT force-pushed the tm-adding-evmanager branch 3 times, most recently from 243cfd5 to 92e2f14 Compare July 1, 2024 12:35
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch from 206842a to 86be3f5 Compare July 3, 2024 12:20
@MarzellT @SebaLukas
Adding EvManager
7386401 
* rewrites the EvManager car simulator in C++
* adds unit tests for the added functions
* fixes a few small issues in the nodered config
* small fixes in the JsEvManager

Co-authored-by: Sebastian Lukas <sebastian.lukas@pionix.de>
Signed-off-by: MarzellT <tobias.marzell@pionix.de>
@MarzellT MarzellT force-pushed the tm-adding-evmanager branch from 86be3f5 to 7386401 Compare July 3, 2024 12:20
@MarzellT MarzellT merged commit a0f1cf1 into main Jul 3, 2024
7 of 8 checks passed
@MarzellT MarzellT deleted the tm-adding-evmanager branch July 3, 2024 12:33
@the-bay-kay the-bay-kay mentioned this pull request Oct 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-w50 a-w50 a-w50 left review comments

@SebaLukas SebaLukas SebaLukas approved these changes

@Pietfried Pietfried Pietfried approved these changes

@hikinggrass hikinggrass hikinggrass approved these changes

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

Assignees

@MarzellT MarzellT

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MarzellT @SebaLukas @hikinggrass @Pietfried @a-w50