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.

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

Group motors #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Group motors #6

wants to merge 4 commits into from

Conversation

jkira1
Copy link

@jkira1 jkira1 commented Feb 16, 2023

No description provided.

@bennowotny bennowotny self-requested a review February 16, 2023 14:53
Copy link
Contributor

@bennowotny bennowotny left a comment

Choose a reason for hiding this comment

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

I think you may have to push again, I thought you made changes that aren't here. Might fix some of the problems.

Also, try building the code. It's a good way to avoid issues during the PR and can allow you to test your changes. If you are having trouble building/getting Intellisense to work, let me know and we can work through it.

Comment on lines +15 to +22
auto motor1{std::make_shared<Hardware::Motor>(1, "LeftBackMotor_1")};
auto motor2{std::make_shared<Hardware::Motor>(2, "LeftMiddleMotor_2")};
auto motor3{std::make_shared<Hardware::Motor>(3, "LeftFrontMotor_3")};

// motors 4 - 6
auto motor4{std::make_shared<Hardware::Motor>(4, "RightBackMotor_4")};
auto motor5{std::make_shared<Hardware::Motor>(5, "RightMiddleMotor_5")};
auto motor6{std::make_shared<Hardware::Motor>(6, "RightFrontMotor_6")};
Copy link
Contributor

Choose a reason for hiding this comment

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

The number can be dropped to avoid duplicating ID information.

auto motor6{std::make_shared<Hardware::Motor>(6, "RightFrontMotor_6")};

// motor group 1
auto motorGroup1{std::make_shared<Hardware::Group>("LeftGroupMotors")};
Copy link
Contributor

Choose a reason for hiding this comment

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

For compatibility reasons, this group should be called left.

motorGroup1->addControlGroup(motor3);

// motor group 2
auto motorGroup1{std::make_shared<Hardware::Group>("RightGroupMotors")};
Copy link
Contributor

Choose a reason for hiding this comment

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

For compatibility reasons, this group should be called right.

std::cout << "Motor setup complete!" << std::endl;

// Setup ROS Interface
ros::init(argc, argv, "RosControlTest");
Copy link
Contributor

Choose a reason for hiding this comment

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

The node name should change, since this would not be a test node.

#include <vector>

auto main(int32_t argc, char **argv) -> int32_t {
std::cout << "wrevolution ROS test start..." << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

This print can be removed.

motorGroup1->addControlGroup(motor3);

// motor group 2
auto motorGroup1{std::make_shared<Hardware::Group>("RightGroupMotors")};
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a motor group called motorGroup1, so this doesn't compile.


std::vector<std::unique_ptr<RosHandler::ControlGroupRosHandler>> controlGroupHandlers{};

controlGroupHandlers.reserve(testingGroups.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think testingGroups is declared, so this doesn't compile.

ros::init(argc, argv, "RosControlTest");
ros::NodeHandle node{};

std::vector<std::unique_ptr<RosHandler::ControlGroupRosHandler>> controlGroupHandlers{};
Copy link
Contributor

Choose a reason for hiding this comment

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

There are only 2 groups of motors, so we probably don't need a dynamic list.

Comment on lines +46 to +48
for (const auto &controlGroup : testingGroups) {
controlGroupHandlers.push_back(std::make_unique<RosHandler::ControlGroupRosHandler>(node, controlGroup));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be converted to 2 separate constructions.

@@ -0,0 +1,58 @@
#include "ControlGroup.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs to move to its own folder with its own CMakeLists.txt to respect the structure of the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants