Remove custom message dependencies #3

ekampourakis · 2020-04-20T12:34:17Z

As mentioned in this issue:

Removed all the custom message dependencies and replaced them with more generic messages from my vehicle which I also included. Please see the README.
Added 2 more parameters to make the package more versatile for different TF configurations.
Updated the README.

Please review this PR and let me know.

Best regards,
Manos

MaxMagazin · 2020-04-21T04:37:58Z

Hi @ekampourakis,
thanks for uploading your updates, I took a quick look on them. Generally I am fine with them, but have a couple of comments. Unfortunately I am a bit busy now with my work, will finish the code review in the weekend. Cheers.

ekampourakis · 2020-05-07T12:56:02Z

Hello @MaxMagazin ,

I'm posting this comment as a reminder for you to review the code changes and notify me about accepting or rejecting them.

Thanks.

MaxMagazin · 2020-05-10T09:01:31Z

src/inc/ma_rrt.py

@@ -1,433 +0,0 @@
-"""


It looks like you wanted to move this file to src folder, but you commited the deleting of this file. Are you sure the package works as it is in your fork (https://github.com/ekampourakis/ma_rrt_path_plan)?

It should complain in line 172 of MaRRTPathPlanNode.py, where
rrt = ma_rrt.RRT

Yes you are absolutely right. It somehow got deleted. I removed the inc folder and added the file in the src folder directly because it was complaining. I fixed it in my fork.

MaxMagazin · 2020-05-10T23:53:09Z

README.md

 ## Inputs, outputs, params

 #### Inputs
- rospy.Subscriber("/map", Map, ...)
+- rospy.Subscriber("/map", Track, ...)


Lets have it consistent:
rospy.Subscriber("/track", Track, ...)

MaxMagazin · 2020-05-10T23:57:22Z

README.md

@@ -34,6 +37,8 @@ A brief introduction to the main steps of the proposed algorithm is given in my
 - rospy.Publisher("/visual/waypoints", MarkerArray, ...)

 #### Parameters to tune (main)
+- `odom_topic` = `/odometry` - The topic to get the odometry information from
+- `world_frame` = `world` - The world frame to use for the visualizations


This parameters list was intended for algorithm parameters. I would leave this info out, or if you think it should be there in ReadMe, please move it to another section called Frames

MaxMagazin · 2020-05-11T00:06:46Z

launch/startExploring.launch

@@ -3,6 +3,10 @@
    <node pkg="ma_rrt_path_plan" type="main.py" name="ma_rrt_path_plan_node" output="screen">
        <param name="desiredWaypointsFrequency" value="5" type="int" />

+        <!-- TODO: Comment me out -->
+        <!-- <param name="odom_topic" value="/odometry/real"/>
+        <param name="world_frame" value="map"/> -->


These three lines are confusing.

For who is this TODO then if it is there

Generally having the frames specified in launch files is cool, but the frames that you specified are unique to your system (again). For this purposes you could create a separate launch file, but I dont see a reason to have it this repo.

MaxMagazin · 2020-05-11T00:08:37Z

src/MaRRTPathPlanNode.py

-        rospy.Subscriber("/map", Map, self.mapCallback)
-        rospy.Subscriber("/odometry", Odometry, self.odometryCallback)
-        # rospy.Subscriber("/car_sensors", CarSensors, self.carSensorsCallback)
+        rospy.Subscriber("/track", Track, self.mapCallback)


Once it is all about track (instead of map), lets have a new callback name too (self.trackCallback)

MaxMagazin · 2020-05-11T00:11:47Z

src/MaRRTPathPlanNode.py


        #print "Estimated processing map callback: {0} ms".format((time.time() - start)*1000);

-    def mapCallback(self, map):
-        self.map = map
+    def mapCallback(self, track):


trackCallback

MaxMagazin · 2020-05-11T00:14:07Z

src/MaRRTPathPlanNode.py

-    def mapCallback(self, map):
-        self.map = map
+    def mapCallback(self, track):
+        self.map = track.cones


Lets have self.track (instead self.map)
and initialize it with received track object (not with cones directly)

MaxMagazin · 2020-05-11T00:16:05Z

src/MaRRTPathPlanNode.py

@@ -803,7 +810,7 @@ def getFrontConeObstacles(self, map, frontDist):
        frontDistSq = frontDist ** 2

        frontConeList = []
-        for cone in map.cones:
+        for cone in map:


based on my other comments here will be:
for cone in track.cones
note, additional renamings are needed

MaxMagazin · 2020-05-11T00:16:34Z

src/MaRRTPathPlanNode.py

@@ -818,7 +825,7 @@ def getHeadingVector(self):
    def getConesInRadius(self, map, x, y, radius):
        coneList = []
        radiusSq = radius * radius
-        for cone in map.cones:
+        for cone in map:


see above:
for cone in track.cones

MaxMagazin · 2020-05-11T00:26:40Z

src/MaRRTPathPlanNode.py

-        # rospy.Subscriber("/car_sensors", CarSensors, self.carSensorsCallback)
+        rospy.Subscriber("/track", Track, self.mapCallback)
+        rospy.Subscriber(self.odometry_topic, Odometry, self.odometryCallback)
+        rospy.Subscriber("/dvsim/cmd", Command, self.carSensorsCallback)


I don't really use the steering angle of the car for path planning (this subscriber was even commented out, but I tried to use this information during my development).
But in your version it appears to be confusing: you subscribe to a topic (which generally means to get something), and you receive a command (which generally would be understood as control request).
Maybe message should be named a bit differently, what about CarState?

MaxMagazin · 2020-05-11T00:37:03Z

Hey @ekampourakis, I added a few comments about the changes. My main concern is that some your changes are still unique and specific to your system, which do not help to improve generalization of the package. Although, the messages you created are the nice contribution, thank you for that.
If you don't feel like contributing more, I could still merge the PR as it is right now, but most probably will update/revert/improve a couple of your changes later. No worries, I would still mention you in contributors list later.

ekampourakis · 2020-05-11T07:43:12Z

@MaxMagazin Thanks for all the comments. I will look at them all and try to correct them but it will take some time as I'm working on many other projects as well.

Don't close the PR just yet if you don't need to and I'll keep updating my fork until all your suggestions are implemented.

ekampourakis added 2 commits April 20, 2020 15:19

Replace egn_messages with vehicle_msgs

c3cd0bd

Add vehicle_msgs

6b20b67

MaxMagazin reviewed May 10, 2020

View reviewed changes

Upload wrongly deleted file

8d93218

MaxMagazin requested changes May 11, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove custom message dependencies #3

Remove custom message dependencies #3

ekampourakis commented Apr 20, 2020

MaxMagazin commented Apr 21, 2020

ekampourakis commented May 7, 2020

MaxMagazin May 10, 2020 •

edited

MaxMagazin May 10, 2020

ekampourakis May 10, 2020

MaxMagazin May 10, 2020

MaxMagazin May 10, 2020

MaxMagazin May 11, 2020

MaxMagazin May 11, 2020

MaxMagazin May 11, 2020

MaxMagazin May 11, 2020

MaxMagazin May 11, 2020

MaxMagazin May 11, 2020

MaxMagazin May 11, 2020

MaxMagazin commented May 11, 2020

ekampourakis commented May 11, 2020

Remove custom message dependencies #3

Are you sure you want to change the base?

Remove custom message dependencies #3

Conversation

ekampourakis commented Apr 20, 2020

MaxMagazin commented Apr 21, 2020

ekampourakis commented May 7, 2020

MaxMagazin May 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxMagazin commented May 11, 2020

ekampourakis commented May 11, 2020

MaxMagazin May 10, 2020 •

edited