search

AnishShr / autoware_mini_practice

Practice Sessions and Labs for the Autonomous Vehicles Project (Autoware mini)
MIT License
0 stars 0 forks source link

Practice 4 - code comments #4

Closed geopimik closed 5 months ago

geopimik commented 6 months ago

Please fix the following:

  1. You are not using this anywhere: https://github.com/AnishShr/autoware_mini_practice/blob/8dd232a77319ab8eb8a20a69bd2aceb8b6246b32/practice_4/nodes/planning/global/lanelet2_global_planner.py#L45

  2. This does not need to be the global variable, used only in one callback. And you could also do without it, since you have self.goal.pos, but it's on you to decide https://github.com/AnishShr/autoware_mini_practice/blob/8dd232a77319ab8eb8a20a69bd2aceb8b6246b32/practice_4/nodes/planning/global/lanelet2_global_planner.py#L41

  3. As stated in Task 4 the point 5, there should be a separate function for publishing waypoints: publish_waypoints(waypoints)

  4. You could make this code more compact by using the min operator. Then you wouldn't need if and else here: https://github.com/AnishShr/autoware_mini_practice/blob/8dd232a77319ab8eb8a20a69bd2aceb8b6246b32/practice_4/nodes/planning/global/lanelet2_global_planner.py#L65-L70

  5. Correct way to do it, but a bit clumsy by having these multiple numbers. Could do also / 3.6 https://github.com/AnishShr/autoware_mini_practice/blob/8dd232a77319ab8eb8a20a69bd2aceb8b6246b32/practice_4/nodes/planning/global/lanelet2_global_planner.py#L73

AnishShr commented 6 months ago

1 & 2. Removed all unused class variables, and set them to local variables

  1. Created a function to create the list of waypoints https://github.com/AnishShr/autoware_mini_practice/blob/3aec090f4175fc6fe6067ff03cba0b64d799a368/practice_4/nodes/planning/global/lanelet2_global_planner.py#L54-L81

  2. Used min replacing the if block https://github.com/AnishShr/autoware_mini_practice/blob/3aec090f4175fc6fe6067ff03cba0b64d799a368/practice_4/nodes/planning/global/lanelet2_global_planner.py#L63-L64

  3. Computed speed in a more concise manner https://github.com/AnishShr/autoware_mini_practice/blob/3aec090f4175fc6fe6067ff03cba0b64d799a368/practice_4/nodes/planning/global/lanelet2_global_planner.py#L66

global planner node latest commit

geopimik commented 5 months ago
  1. OK
  2. OK, but You had two global variables: self.goal_point and self.goal_pos. I would have kept one of them. You went for creating self.waypoints as a global variable. I think it makes things a bit less transparent and more complicated for you.
  3. OK
  4. OK
  5. OK
geopimik commented 5 months ago

To me it seems that you have made the node a bit more complicated than it was before. The previous solution was almost working and needed some tiny adjustments.

In current solution you loop over all of the waypoints twice, here:

https://github.com/AnishShr/autoware_mini_practice/blob/eb15e62f07de5033d49a650e61b8340e53223f19/practice_4/nodes/planning/global/lanelet2_global_planner.py#L68-L79

and then again here:

https://github.com/AnishShr/autoware_mini_practice/blob/eb15e62f07de5033d49a650e61b8340e53223f19/practice_4/nodes/planning/global/lanelet2_global_planner.py#L137-L143

  1. You should manage with one loop.
  2. You should not publish waypoints within current_pose_callback - it will do it with 50Hz!

My suggestion:

  1. Go back to previous solution
  2. In the goal_callback
    • prepare the waypoints in the lanelet_sequence_to_waypoints and handle everything you are trying to achieve with the second loop (projecting goal point to path is one of the valid solutions) also inside that function.
    • the returned list of waypoints should be sent to another function that is creating the Lane message and will publish the list of waypoints in that message
  3. current_pose_callback checks if goal is reached and once it is, it will call the same function with empty list to publish empty Lane message.
AnishShr commented 5 months ago

  1. The lanelet_sqeuqnce_to_waypoints function now returns the list of waypoints where waypoints projected distance in the path is less than or equal to the goal point in path. However, I could not think of anything simpler than using another loop. Is there a simpler method? https://github.com/AnishShr/autoware_mini_practice/blob/e0f954de60f512726e37c8a4a1c7d7d7ce474d30/practice_4/nodes/planning/global/lanelet2_global_planner.py#L63-L103

  2. publishing Lane msg from current_pose_callback is removed. Once the goal is reached the same function (create_and_publish_lane_msg) publishes empty waypoints once. https://github.com/AnishShr/autoware_mini_practice/blob/e0f954de60f512726e37c8a4a1c7d7d7ce474d30/practice_4/nodes/planning/global/lanelet2_global_planner.py#L169-L172

Latest commit: https://github.com/AnishShr/autoware_mini_practice/commit/e0f954de60f512726e37c8a4a1c7d7d7ce474d30

geopimik commented 5 months ago

  1. Here you should have the warning, because it is a problem when do to have the current_location https://github.com/AnishShr/autoware_mini_practice/blob/5d312201cd897519df89f0435e257897b6112d0f/practice_4/nodes/planning/global/lanelet2_global_planner.py#L117-L118

  2. I would keep only one of them as global variables https://github.com/AnishShr/autoware_mini_practice/blob/5d312201cd897519df89f0435e257897b6112d0f/practice_4/nodes/planning/global/lanelet2_global_planner.py#L44-L45

    • I would prefer goal_pos, and it would be more logical then that in current_pose_callback you check if the goal_pos is not none and in the goal_callback you check current_pose
    • I don't think waypoints should be global variable, it more like a result from goal_callback

  3. I don't like also taking last wp as goal point on path. I would just reset its value when the path is created and you have synced the goal point with path ending. https://github.com/AnishShr/autoware_mini_practice/blob/5d312201cd897519df89f0435e257897b6112d0f/practice_4/nodes/planning/global/lanelet2_global_planner.py#L162

  4. You should add z coordinate also for the goal point (last wp on the path). All the other waypoints have the z coordinate. https://github.com/AnishShr/autoware_mini_practice/blob/5d312201cd897519df89f0435e257897b6112d0f/practice_4/nodes/planning/global/lanelet2_global_planner.py#L96-L99

AnishShr commented 5 months ago

  1. Added rospy warning log in console https://github.com/AnishShr/autoware_mini_practice/blob/f9132c4de756dcbc10dca970f90e2089eaa3f208/practice_4/nodes/planning/global/lanelet2_global_planner.py#L120-L122

  2. Removed self.waypoints as class variables. In the current_pose_callback, it checks and runs according to the self.goal_pos variable https://github.com/AnishShr/autoware_mini_practice/blob/f9132c4de756dcbc10dca970f90e2089eaa3f208/practice_4/nodes/planning/global/lanelet2_global_planner.py#L42-L44 https://github.com/AnishShr/autoware_mini_practice/blob/f9132c4de756dcbc10dca970f90e2089eaa3f208/practice_4/nodes/planning/global/lanelet2_global_planner.py#L158-L160

  3. In the lanelet_sequence_to_waypoints function, the self.goal_pos variable is updated in the end with the coordinates of the goal point in path. https://github.com/AnishShr/autoware_mini_practice/blob/f9132c4de756dcbc10dca970f90e2089eaa3f208/practice_4/nodes/planning/global/lanelet2_global_planner.py#L101-L102

  4. Added z coordinate of the goal point as well in the last waypoint https://github.com/AnishShr/autoware_mini_practice/blob/f9132c4de756dcbc10dca970f90e2089eaa3f208/practice_4/nodes/planning/global/lanelet2_global_planner.py#L95-L99

AnishShr commented 5 months ago

latest commit: https://github.com/AnishShr/autoware_mini_practice/commit/f9132c4de756dcbc10dca970f90e2089eaa3f208

geopimik commented 5 months ago

  1. It would also be nice to have a node name at the beginning of the info, log, warning and error messages. Similarly as you can see here. https://github.com/AnishShr/autoware_mini_practice/blob/a750cd623ce0c1a6fcfd52bd598459fb2946784b/practice_4/nodes/planning/global/lanelet2_global_planner.py#L124

  2. OK

  3. and 4.

    • you receive a goal point and get xyz in goal callback. - OK
    • you update the location of the goal point to be on the path in the lanelet_sequence_to_waypoints and update its x and y coordinates, but you keep the z? Why not update also z?
    • I understand getting the new z value is tricky, but what if the location change in x and y is placing it 30m away? Would you keep the z value from user user-entered pose or update it somehow? I would lean towards updating, and there is no perfect option, but you might:
      • interpolate it using the 2 neighbouring waypoints from the path
      • or just take from the closest waypoint - currently that will also do!
    • For me, it seems right if x and y are updated, then z is also updated.
    • and you shouldn't use the local variable goal_pos to eventually find goal_point_in_path and then use the global variable self.goal_pos to set the z value. The global variable might have already changed because there was a new goal point entered.

  4. I would not create that variable

  5. Currently, if speed_ref would not be present, it would skip that if and you have an error that there is no variable speed defined https://github.com/AnishShr/autoware_mini_practice/blob/a750cd623ce0c1a6fcfd52bd598459fb2946784b/practice_4/nodes/planning/global/lanelet2_global_planner.py#L63-L66

AnishShr commented 5 months ago

3 and 4. Modified the code to update the z-coordinate for goal pose taking the z-coordinate of the closest waypoint. https://github.com/AnishShr/autoware_mini_practice/blob/88aa679257b27f8de6cb0d80a6a00370dbe4eabf/practice_4/nodes/planning/global/lanelet2_global_planner.py#L97-L112

  1. added default speed to be self.speed_limit. If there is speed_ref, it will replace the speed with minimum from the two speeds. https://github.com/AnishShr/autoware_mini_practice/blob/88aa679257b27f8de6cb0d80a6a00370dbe4eabf/practice_4/nodes/planning/global/lanelet2_global_planner.py#L66-L70

Latest commit: https://github.com/AnishShr/autoware_mini_practice/commit/78ac6f1205ea00c723de6c0b2709274b425b866b

geopimik commented 5 months ago

OK