Autodocking

[tul1]

Autodocking routine: when the battery of Leonardo is running low, Leonardo have to go docking.

[julianmateu]

Please remove, this file should not be changed.

[tul1]

done!

[tul1]

@julianmateu the code is ready for a review.

[julianmateu]

Well done @tul1! I left some comments. There are some of them that apply for future work (you could address them in a different CL). The logging and style ones should be addressed before merging this one.

[julianmateu]

ROS provides a rosout topic for logging information from nodes. I think that is the preferred method, right @ernestmc?

[ernestmc]

Yes, you can use

rospy.logdebug("Whatever you want to log")

[julianmateu]

Style: alignment is inconsistent in these lines.

[julianmateu]

Extra line

[julianmateu]

Extra line. See PEP-8:

Surround top-level function and class definitions with two blank lines.
Method definitions inside a class are surrounded by a single blank line.

[julianmateu]

For a first version this might be OK, but I would suggest to use a more descriptive name, and make sure the behavior is the intended one. With the current implementation the robot will go docking only once and never dock again.

For example, here this variable will be set to true when the docking action has started, so a more descriptive name would be _docking_action_started or _docking_in_progress. In that case, you would need to make sure that it is set to false again once the action finishes. But you would also need another flag that tells you if the robot is already docked and charging, so that you don't trigger the auto docking in the case it is charging (you could read the battery state for that).

So the logic will be something like:
If not charging and not docking_in_progress then dock (if the battery level is low).

[tul1]

I forgot to change the state of _go_docking when is called _doneCb. Changing that state there I think the Leonardo is going to behave as expected.

[julianmateu]

This is OK for now, but it would be good to make it a parameter configurable from the launch file.

[tul1]

@ernestmc I'd added the autodocking routine to a package. Would you give a look of what I've done?

[tul1]

@ernestmc I could be able to build the whole project with catkin_make as you can see in the screenshot beneath:

However, I cannot run the auto_docking with rosrun, it doesn't even show this package as an option when I press tab. Running rosrun auto_docking I get the following error:

Usage: rosrun [--prefix cmd] [--debug] PACKAGE EXECUTABLE [ARGS]
  rosrun will locate PACKAGE and try to find
  an executable named EXECUTABLE in the PACKAGE tree.
  If it finds it, it will run it with ARGS.

So the package isn't in the namespace.

I pushed the code so you can have a look @ernestmc . Let me know if you find anything wrong that could cause this problem. Thanks :-)

[julianmateu]

@tul1 did you source the setup.sh file?

[tul1]

Hey @julianmateu ! Yes, I did. Maybe the problem wasn't the package but the python script. I'm getting the following error when running: rosrun auto_docking auto_docking_client.py

[rosrun] Couldn't find executable named auto_docking_client.py below /home/tul1/Desktop/proyectos/leonardo_turtlebot/src/auto_docking
[rosrun] Found the following, but they're either not files,
[rosrun] or not executable:
[rosrun]   /home/tul1/Desktop/proyectos/leonardo_turtlebot/src/auto_docking/scripts/auto_docking_client.py

(the autocomplete tab just works until auto_docking)

[ernestmc]

I left some comments. Please address them and let me know if you need any help.

[ernestmc]

The name of the package seems too generic. I think what we are doing is monitoring the battery state and executing actions, maybe it can be something like leonardo_battery_guard or something similar.

[ernestmc]

Please add a better description of what the package does.

[ernestmc]

Since this is a python package and nothing gets compiled, the build_depends are not really needed.

[tul1]

I don't really know why but apparently these build deps are necessary to build the package. The laptop monitor package also includes them.

[ernestmc]

Again, a better and more descriptive name other than auto_docking_client which is too generic.

[ernestmc]

This is not needed. Please remove the line. (Also why are you using ; this is not C++!!)

[ernestmc]

kuboki --> kobuki

[ernestmc]

Use ros parameters for these values.

[ernestmc]

Remove debugging code or use ros logging

[ernestmc]

It seems reasonable that this method is called do_docking or go_dock since it invokes the action, doesn't it?

[tul1]

It does!

[ernestmc]

I understand correctly this flag is actually telling if the robot is in auto docking mode, right? Shouldn't it then be called something like docking, doing_docking, docking_invoked or something like that?

[tul1]

I like doing_docking. I'm going to change it.

[tul1]

@julianmateu We solved the problem with @ernestmc just changing permissions of the python script auto_docking_client.py to be executable.

[tul1]

@ernestmc trying to set argument from the launch file I've gotten the following error:

ERROR: cannot launch node of type [leonardo_battery_guard/dock_drive_client_py]: can't locate node [dock_drive_client_py] in package [leonardo_battery_guard]

would you have any idea where is this coming from?
I've just pushed the launch file.

[tul1]

@ernestmc @julianmateu I'm ready for a new review!

[julianmateu]

Missing new line at the end of the file.

[julianmateu]

Extra line.

[julianmateu]

What happens here if the robot is docked and charging but the battery level is below the threshold? Does the docking action get triggered? Have you tested this case?

[tul1]

Good point @julianmateu ! I added some logic to prevent that case.

[tul1]

@julianmateu @ernestmc I added a navigation action client to the node to send Leonardo close to the dock station before it starts autodocking.
I tried the code in Leonardo and in the turtlebot gazebo simulation but I could make it work and no error were printed. Tomorrow I'll continue testing different alternatives but I think that by this point the code I pushed is ready for a new review.
I added some TODO comments in the code, please let me know what do you think about them.

[tul1]

@julianmateu @ernestmc the code is ready for a new review.
It's working and it has been tested satisfactory twice on Leonardo. It's going to need further field tests to adjust details.

[tul1]

@julianmateu FYI with @ernestmc we handled to correct the last code errors and we were able to autodock Leonardo.

[tul1]

changes done

[tul1]

This code here is ready to be reviewed, it's "working" and it has been tested several times in Leonardo. I quoted "working" because the code or system is going to need further adjustments. For instance, the biggest issue we have lies in the autodocking routine. In fact, Leonardo rarely docks, it just approaches the dock and hits the base.

Another possible issue is that we think that the routine is being triggered in condition where it shouldn't. For example, while the turtlebot is charging. We couldn't verify this but it happened twice that Leonardo was running the autodocking routine after being docked.

Also we talked offline with @ernestmc about some changes that occurred from master to dev, and before merging this branch the file system should be reorganized.

Last comment, in order to run the routine for debugging proposes I added a service named /run_autodocking and to call it the following line should be invoked in the terminal: rosservice call /run_autodocking "{}"