anson/robocup-sw-tutorial #2115
Conversation
|
nodes: /control, /radio, /coach_node |
There was a problem hiding this comment.
please don't commit your personal .vscode configuration
There was a problem hiding this comment.
again, don't commit this either
| publisher->publish(message); | ||
| } | ||
|
|
||
| void SoccerMom::topic_callback(const std_msgs::msg::String & msg) const { |
There was a problem hiding this comment.
are you doing anything with this callbacK?
| //RCLCPP_INFO(this->get_logger(), teamColor); | ||
| }); | ||
| publisher = this->create_publisher<std_msgs::msg::String>("/team_fruit", 10); | ||
| timer = this->create_wall_timer(std::chrono::milliseconds(500), std::bind(&SoccerMom::timer_callback, this)); |
There was a problem hiding this comment.
using a wall timer isn't necessarily incorrect, but we can be more precise in this case. since we know that the fruit is only updated with team color, we ought to only publish in reaction to a new team color (in other words, directly in your subscription callback)
There was a problem hiding this comment.
this prevents both over and under-publishing
| @@ -35,12 +36,12 @@ enum MatchSituation { | |||
| in_play, // normal play | |||
| }; | |||
|
|
|||
| enum Positions { Goalie, Defense, Offense, PenaltyPlayer, GoalKicker }; | |||
| enum Positions { Goalie, Defense, Offense, Runner, PenaltyPlayer, GoalKicker }; | |||
There was a problem hiding this comment.
you updated this enum, but didn't correctly update the switch-case in AAC that corresponds to this enum; so all of the positions assigned by coach are offset now. I see that you attempted to but commented it out. The correct behavior would have been to chance case 3: to be Runner, and change the old case 3 to 4, and 4 to 5.
| * @brief Send request to the other robots to see if this robot should be the scorer | ||
| * | ||
| */ | ||
| void send_scorer_request(); |
There was a problem hiding this comment.
you definitely don't need all of this stuff about scoring. Runners don't score.
| @@ -14,6 +14,9 @@ | |||
| #include "rj_geometry/geometry_conversions.hpp" | |||
| #include "rj_geometry/point.hpp" | |||
|
|
|||
| #include <rclcpp/rclcpp.hpp> | |||
There was a problem hiding this comment.
try not to update or reformat files that your PR isn't intended to fix. if you notice bugs open a separate one
| //SPDLOG_INFO("ANSON: test test test"); | ||
|
|
||
| // if no ball found, stop and return to box immediately | ||
| if (!world_state->ball.visible) { |
There was a problem hiding this comment.
why does the Runner care about the ball?
| if (current_state_ == IDLING) { | ||
| //send_scorer_request(); | ||
| next_state = MOVING_LSIDE; | ||
| } else if (current_state_ == MOVING_LSIDE && at_corner) { |
There was a problem hiding this comment.
something like at_corner should be calculated in update_state() (since that is part of your state transition) and not in state_to_task, which just converts states to tasks.
There was a problem hiding this comment.
However, for a simple position like Runner, we shouldn't be manually calculating things like at_corner. Use the provided check_is_done() function to check if the previous motion command has completed.
| return std::nullopt; | ||
| } | ||
|
|
||
| void Runner::receive_communication_response(communication::AgentPosResponseWrapper response) { |
There was a problem hiding this comment.
again, none of this scoring code should have been included. I assume you copy-pasted from offense or some other position. make sure to understand any code you copy-paste and see if it is necessary.
filepath:
/home/anson/robojackets/robocup-software/rj_msgs/msg/RobotIntent.msg