The roboclaw_params.yaml uses has params for encoder ticks per revolution, currently set to 2000. This 2000 is not quite the number. Will provide details in a write up shortly

See the comment in #71
The value in physical properties is 130rpm, which is true for my rover, but not for the rovers built according to the default spec. The default ones only do 31rpm.

Depending on the setting's wheel distance you can cause a divide by zero error when updating position, breaking the normal joystick behavior. This can be easily remedied with a Try-Catch block or a check for equality

the motor_controller node takes parameter of a serial device to use on the RPi to communicate to the motor controllers. This should be /dev/ttyS0 or /dev/serial0. Also might think about having it try /dev/ttyAMA0 or /dev/serial1 if those fail.

This includes:

• more inline comments
• file descriptors (multi-line comments at top of file)
• docstrings
• update and clean up main README.md

Currently crucial nodes are respawned when they crash. Instead, they should be set to required=True (perhaps if they repeatedly fail) so that everything stops when this happens. No use in trying to run when a crucial node doesn't work.

Would anyone be interested in a PR for this?

motor_controller.py and rover.py have a topic name mismatch, needs to be fixed to point to the same topic.

It appears that there is a discrepancy between the hardware docs assigning left/right of the robot and what the code currently has. This can be fixed by inverting the list directions on all the functions that calculate the speeds and the corner angles.

As a note about this. The hardware I have built on my robot is such that the software is correct for it, it must have gotten put into the hardware docs incorrect somewhere. As I don't want to redo all the hardware on my robot my proposition is to add a variable managed to do left-right swap for the robot.

Working on adding a .travis.yml file as per #9. This will be more useful once a unit test folder is in place. The current script just consists of a pep8 linters for now.

Forked branch: https://github.com/capsulecorplab/osr-rover-code/tree/capsulecorplab/add-travis-ci

The robot_description package contains the URDF/XACRO files used to represent the robot which is useful for visualization in RViz, running the robot in simulation with Gazebo, and sensor placement.

rename scripts folders to src. It's common to put actual scripts in a scripts folder and nodes and other default running code in src.

ROS has its own logging system with varying log levels (debug, info, warn, error, fatal). Instead of using python prints, which cannot be filtered or display different severity levels, all print statements used in ROS nodes should be converted to rospy logs. This is done in some places already but not in all.

Instead of commenting out some debugging print statements, you can also decide to log them at the debug level, which can be conveniently turned off or filtered (e.g. rosconsole)

The rosnodes need a boot sequence forced to make sure that the motor_controller node completes its' initialization before other nodes start

Instead of duplicating the source for the roboclaw code (roboclaw.py), we should clone the official repo, perhaps fork it to turn it into a catkin or pip package.

This ensure we can get updates properly, keep our codebase smaller and cleaner, and report issues related to roboclaw code in their repo where we will find official support.

RoboClaw updated their software to coincide with firmware update, this will need to be updated to allow information querry of some information from them now.

https://www.basicmicro.com/downloads

Need to uncomment and fix path name to start the led screen node

The code for calculateTargetDeg contains something like:

ang1 = int(math.degrees(math.atan(self.d1 / (abs(r) + self.d3))))
ang2 = int(math.degrees(math.atan(self.d2 / (abs(r) + self.d3))))
ang3 = int(math.degrees(math.atan(self.d2 / (abs(r) - self.d3))))
ang4 = int(math.degrees(math.atan(self.d1 / (abs(r) - self.d3))))

angles = [ang1, ang2, ang3, ang4]
for i in range(4):
	if angles[i] < -45:
		angles[i] = -43
	elif angles[i] > 45:
		angles[i] = 43

if radius > 0:
	return [ang2, -ang1, -ang4, ang3]
else:
	return [-ang4, ang3, ang2, -ang1]

The middle section seems to be clipping values outside the range abs(45) to abs(43) but those values are assigned to the variables angles, which isn't used in the following return value calculation.

Options:

1. remove clipping, it's not functional code
2. fix the clipping if it needs to stay

In addition to a requirements.txt as mentioned in #8, here are some other suggestions for creating a contributor-friendly python repo:

• Add a tests/ folder containing unit tests - this can be useful for promoting a TDD workflow, as it gives contributors a metric for what the code should do without actually having to open up the source code. Personally, I like pytest, but there are other options.
• Enable continuous integration, ci, s.a., travis-ci or circle-ci, with a config file containing scripts for running unit tests - helps automate PR reviews, as the ci can check to ensure the suggested code changes haven't broken core functionality.
• Add a CONTRIBUTING.md - helps promote best practices for working on an issue or submitting a PR, sometimes occompanied by a code of conduct. (This one's also still on my backlog of things to do)
• Add pep8 linter to ci config - optional, but helps keep code readable.
• Enable code coverage s.a., codecov or coveralls - optional, but serves as an additional confidence metric in source code.

EDIT: POCS has a fairly python contributor-friendly repo.

User name

LaunchOSR.sh is based on username pi
osr_startup.service is based on username ubuntu

They should be consistent.

Plus the Software Controls PDF should mention an user may need to edit the username in these files to match what they have configured.

Syntax errors in osr_startup.service

WorkingDirectory=/home/ubuntu/LaunchOSR.sh
This points to the shell script, should point to a directory instead.

ExecStart=/home/ubuntu/
This is pointing to a directory, should point to the script instead.

Wantedby=multi-user.target
This parameter is case sensitive and should be WantedBy

Hello,

In the code roboclaw.py there is a section denoted (by comment)

# Private Functions (at line: 109)

Yet the first two functions break the convention of starting with an underscore:

crc_clear(self)
crc_update(self, data)

So, from my quick overview of the file either they need to be moved down to the section of "User Accessible Functions" or they need to be prefixed?

Either way, this is a really small issue.
If they can be moved or prefixed, and if you want to change it, I can make a PR to do so but I thought I'd rather first ask ;-)

EDIT: I quickly made a PR anyway since I had the code locally and it takes less than a minute to do this. So no real time lost if it wasn't necessary 😄

Some information related to this issue is on the forum at https://www.tapatalk.com/groups/jpl_opensource_rover/serial-serialutil-serialexception-errno-2-could-no-t132-s10.html

Add unit tests to the code using the unittest library included with Python.

First round of tests will focus on testing the modules and not their implementation (a baseline to make sure the basics work as they should).

See pulls

• #39
• #29

Currently the messages contain an ordered list for each motor which is hard to interpret and error prone. Either the messages should be converted to contain specific fields, e.g.:

drive:
  int32 left_front
  int32 left_middle
  int32 left_back
  int32 right_front
  int32 right_middle
  int32 right_back
steer:
  int32 left_front
  int32 left_back
  int32 right_front
  int32 right_back

OR the order should be better documented. I believe currently the order is 131M1, 131M2, 132M1, 132M2. Perhaps we want to change this to be clockwise starting front right or similar.

I have the option to install ROS on Raspbian, my usual and familiar RPi OS, or on Ubuntu. Which should I use, or does it matter? I'm following a tutorial on ROS that wants to demo using turtlebot/turtlesim which doesn't exist on my Raspbian stretch / ROS kinetic image. The online instructions I find to install it all relate to Ubuntu.

Being a newbie to ROS I suspect the rover code it isn't working because I have to source the catkins on my machine but I could use some direction here.

Thanks!
JHP

The repository contains the compiled pyc files. This is unnecessary because the interpreter creates them when it runs. Their pattern should go into .gitignore and they should be removed.

Fix typo in main.py with wrong path to config.json

All custom messages should be populated with the header data field as well.

Instead of using position control, use a velocity controller (or even torque) on the corner steering motors. This would allow higher control loop rates to ensure that all wheels are synced up while turning.
Perhaps also the time to switch to ros_control

Convert joystick.py and potentially other code to a Python class instead of using global variables. Globals are bad.

Probably want to solve #45 first.

This line tries to change the sign of rmax variable which is undefined.

I'm trying to follow the steps in the osr-ROS README to load the code onto my Arduino. After my workaround for issue #23, I got the code downloaded and I got the sample project loaded into the Arduino IDE. When I try to upload this to my Arduino, though, I get the following error:

Osr_Screen:10:28: error: RGBmatrixPanel.h: No such file or directory
compilation terminated.
exit status 1
RGBmatrixPanel.h: No such file or directory

Looking through the source code, I can confirm that there is no RGBMatrixPanel.h file present.

FIles should be cleaned up and a .gitignore added to avoid committing them again

To be consistent with the robotics and ROS community, it would make sense to switch to SI units for all calculations. See ROS' REP103. Instead of degrees and degrees per second, we would use radian and rad/s as well as meter and m/s iso inches.

Happy to do this if there is consensus on this.

For some reason the joystick continuously publishes the last message it received to the rover. As a result, when you're driving forward at full speed and the joystick disconnects, the rover will keep going in whatever the last direction was.

The fix is as simple as setting the autorepeat_rate in osr.launch for the joy node back to 0.0, or simply deleting that line.

Indentation bug causes

AttributeError: Threads instance has no attribute 'SendData'

Nonexistent socket object (should be self.ser)

NameError: global name 'socket' is not defined

Missing flush() causes sendData() to not reliably send out data

It's common for mobile robots to use the geometry_msgs/Twist message to command velocities. This would make it work out of the box with the ROS navigation stack as well as the commonly used keyboard teleop package among others.
This can be on top of the current osr_msgs/Commands and should probably replace the custom osr_msgs/Joystick message.

Although easy to calculate and independent of encoder model,

• this method can only be used for corner encoders as it assumes a range of 90 degrees
• the range isn't exactly 90 degrees, due to mechanical inaccuracies and soft limits.

My proposal is to convert ticks based on the constant ration tick/radian or tick/degree (related: #59) and have a setting for this ratio for those who have different encoders/motors.

Description

Use of ROS services to expose important/commonly changed settings such as calibration values, min/max encoder values, speed/accel profiles and more
This means an easier way to change settings and allows for algorithms to change things on the go. Also allows for changing settings remotely through distributed computing (which ROS handles)

It's not very difficult, and I might be able to do it if there are no takers. Python 2 will EOL on 1 January 2020.

Rosparameters should live in a config file instead of in the launch file

The main README.md incorrectly lists the face as static. This should be updated as well as information about how to send messages that update the face (and what faces are available, and how to add your own)

Publish messages of type nav_msgs/Odometry to the /odom or /wheel_odom topic.

Currently the turning radius used for calculating drive velocities is the commanded velocity as opposed to the current turning radius. This should be easy to do after the overhaul described in #55, #45 and others

Following PEP8 convention, method names should be lowercase separated by underscores as opposed of mixedCase.

Currently the message osr_msgs/Status contains:

int64     battery
int64[5]  error_status
int64[5]  temp
int64[10] current

I propose:

• battery should have its own topic, with message sensor_msgs/BatteryState
• error_status should be reported in /diagnostics using diagnostics_msgs/DiagnosticArray which would allow the use of diagnostics tools
• temperature should have its own topic, with message type sensor_msgs/Temperature
• current should be reported in the same sensor_msgs/JointState message used to report feedback on the wheel positions and velocities under 'effort'.

Description

• The rover.py node doesn't really do much other than relay updates from the joy node and encoder updates to commands to the robot. Propose to merge rover.py and robot.py and move joy code to joystick.py
• Similar for roboclaw_wrapper.py and motor_controller.py. Naming is also confusing (class MotorController lives in roboclaw_wrapper.py) and should just be one file.

The source has some pip dependencies which aren't explicitly documented.

Using a requirements.txt or a workflow like Pipenv would aid developers with getting started. Note that Pipenv is the recommended tool by the Python for dependency management.