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

Feature/controller handler #13

Merged
merged 23 commits into from
Jun 1, 2023
Merged

Feature/controller handler #13

merged 23 commits into from
Jun 1, 2023

Conversation

altex111
Copy link
Contributor

No description provided.

@altex111 altex111 added the enhancement New feature or request label May 23, 2023
@altex111 altex111 requested review from VX792 and Svastits May 23, 2023 13:42
@altex111 altex111 self-assigned this May 23, 2023
@altex111 altex111 linked an issue May 23, 2023 that may be closed by this pull request
Support for control mode changes in Real Time kroshu/kuka_drivers#74
Closed
@Svastits
Copy link
Member

General commennts:

  • there are quite a lot of typos in the comments, I have not pointed them out one by one, please re-read them
  • please use the naming conventions for funtion names and enums:
    • functions: functionName, e.g. updateControllerName (snake_case (e.g. get_active_controllers) can also be ok, but try to stick to this convention)
    • enums: EnumName, e.g. ControllerType
  • CI should be green

Svastits
Svastits requested changes May 23, 2023
View reviewed changes
include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Show resolved Hide resolved
src/ControllerHandler.cpp Show resolved Hide resolved
include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
VX792
VX792 requested changes May 23, 2023
View reviewed changes
include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
VX792
VX792 requested changes May 23, 2023
View reviewed changes
Copy link
Contributor

@VX792 VX792 left a comment

Choose a reason for hiding this comment

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

I've left another batch of comments, since for some reason github showed the previous ones as outdated. Hopefully I did it right this time!

include/kroshu_ros2_core/ControllerHandler.hpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
VX792
VX792 requested changes May 26, 2023
View reviewed changes
src/ControllerHandler.cpp Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Show resolved Hide resolved
@Svastits Svastits mentioned this pull request May 26, 2023
Closed
@altex111 altex111 requested review from Svastits and VX792 May 26, 2023 15:11
@altex111
rewrite controller calculator
a9dbf8f
@altex111
correction
c8487a6
@altex111
every vector changed to set
e7774a1 
The maps vectors changed to a struct, possibly should change them into a class
@altex111
build fix
049c70c
VX792
VX792 requested changes May 31, 2023
View reviewed changes
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
@altex111 altex111 requested review from Svastits and VX792 May 31, 2023 14:29
VX792
VX792 approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@VX792 VX792 left a comment

Choose a reason for hiding this comment

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

Other than a few typos here and there, this looks fine to me now, I'll approve, just make sure to test this again later because a lot of stuff was changed, and there's always a chance I've missed something

VX792
VX792 reviewed Jun 1, 2023
View reviewed changes
src/ControllerHandler.cpp Outdated Show resolved Hide resolved
@Svastits Svastits merged commit b6243f7 into master Jun 1, 2023
2 checks passed
@Svastits Svastits deleted the feature/controller_handler branch June 1, 2023 07:57
@sonarcloud
Copy link

sonarcloud bot commented Jun 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VX792 VX792 VX792 approved these changes

@Svastits Svastits Svastits approved these changes

Assignees

@altex111 altex111

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for control mode changes in Real Time
3 participants
@altex111 @Svastits @VX792