18 Motor, 6 Leg robot Walking code - Python / ROS - follow-up

I have re-designed my robot walking code from feedback I received on previous question and would love to hear some feedback and pointers.

Main.py:

```
#!/usr/bin/env python
#

## launch start_robot_controller.launch before running this.

import roslib; roslib.load_manifest('my_dynamixel_tutorial')
import rospy
import math
import operator
import States #Import created states.py

from std_msgs.msg import Float64
from dynamixel_msgs.msg import MotorStateList
from dynamixel_msgs.msg import JointState

## define the command publishers for joints, Legs are named as: L - left; R - Right; M - middle; R - rear. each leg has three joints, 1,2,3
pub_LF1 = rospy.Publisher("/LF1/command", Float64, latch=True)
pub_LF2 = rospy.Publisher("/LF2/command", Float64, latch=True)
pub_LF3 = rospy.Publisher("/LF3/command", Float64, latch=True)
pub_LM1 = rospy.Publisher("/LM1/command", Float64, latch=True)
pub_LM2 = rospy.Publisher("/LM2/command", Float64, latch=True)
pub_LM3 = rospy.Publisher("/LM3/command", Float64, latch=True)
pub_LR1 = rospy.Publisher("/LR1/command", Float64, latch=True)
pub_LR2 = rospy.Publisher("/LR2/command", Float64, latch=True)
pub_LR3 = rospy.Publisher("/LR3/command", Float64, latch=True)
pub_RF1 = rospy.Publisher("/RF1/command", Float64, latch=True)
pub_RF2 = rospy.Publisher("/RF2/command", Float64, latch=True)
pub_RF3 = rospy.Publisher("/RF3/command", Float64, latch=True)
pub_RM1 = rospy.Publisher("/RM1/command", Float64, latch=True)
pub_RM2 = rospy.Publisher("/RM2/command", Float64, latch=True)
pub_RM3 = rospy.Publisher("/RM3/command", Float64, latch=True)
pub_RR1 = rospy.Publisher("/RR1/command", Float64, latch=True)
pub_RR2 = rospy.Publisher("/RR2/command", Float64, latch=True)
pub_RR3 = rospy.Publisher("/RR3/command", Float64, latch=True)

States.Update_var(globals()) #updates and assignes all global variables

global LM_state
global RM_state

#set initial states
LM_state = 1
RM_state = 3
States.LM_State1(globals())
States.RM_State3(globals())

leway = 4.0

There is no easy way to say it so I'll say it the hard way : on a few aspects, I liked the original version of your code more than I like the current one.

Using a dictionary (or any kind of appropriate data structure) to store the information is a good idea and might be the key to remove all the global variable all over the place. However, feeding globals() to functions so that it updates values make things kind of worse that the issue you are initially trying to deal with.

This being said, most of the comments of my previous answer are still relevant here so feel free to have another look.

Anyway, here are a few comments that haven't been said yet.

Names

Python has a style guide called PEP 8. It covers many aspects and is definitely worth a read. Among other things, it covers naming convention and especially constants are better written in CAPITAL_WITH_UNDERSCORES.

rad is a conversion factor and has no reason to change. RAD would me much more explicit.

The name for leway could probably be improved too. The corresponding comment says error allowance which is not even remotely closed to leway. ERROR_ALLOWANCE might be better. I'd usually call such a variable like EPSILON_DELTA_POS because it corresponds to the fact that any position difference smaller than this will be considered to be 0.

Type

One of the best way to solve issues is to solve the right tool for the right job. In computer science/programming, the right tool might be the right data type or data structure. For instance, your flag_[LL]M[123] flags should probably be booleans rather than int. Even if it can be considered as slightly similar, it conveys a lot more meaning to anyone who reads the code.

Then, this code :

def LM1_callback(msg):
    global flag_LM1
    flag_LM1 = 0  
    if abs(msg.current_pos-p_LM1)<leway:
            flag_LM1 = 1

can be written :

def LM1_callback(msg):
    global flag_LM1
    flag_LM1 = False
    if abs(msg.current_pos-p_LM1)<leway:
            flag_LM1 = True

but this is only slightly different so far, the real improvement it that can be written:

def LM1_callback(msg):
    global flag_LM1
    flag_LM1 = abs(msg.current_pos-p_LM1)<leway

Then, other parts of your code can be improved : you can replace the if flag_LM1 == 1 by a much clearer if flag_LM1 (even though the == 1 would still work).
Also, flag_LM = flag_LM1flag_LM2flag_LM3 can be rewritten flag_LM = flag_LM1 and flag_LM2 and flag_LM3. It looks pretty similar and does the same thing but it conveys a lot more meaning to anyone reading your code.

Reordering conditions

It might take a while to anyone to understand what's happening under the ## here are the controls for the states of middle legs comment.

It took me a while to see that the conditions were not disjoints, we might have 2 of the 4 conditions to be true in the same time : the first and the last (and only these one).

Also, it takes some thinking to realise that the conditions are independent enough to be reordered with changing anything.

For these reasons, it might be worth grouping conditions working in a similar way and trying to remove duplicating logic.

I ended with the following code :

if flag_LM and LM_state==1:
    LM_state = 2
    flag_LM = False
if flag_RM and RM_state==1:
    RM_state = 2
    flag_RM = False

if flag_LM and flag_RM:
    if LM_state==2 and RM_state==3:
        LM_state, RM_state = 3, 1
        flag_LM, flag_RM = False, False
    elif RM_state==2 and LM_state==3:
        RM_state, LM_state = 3, 1
        flag_RM, flag_LM = False, False

Now, I can see that we start with a similar check in the 2 first conditions, one is for left only, one for right only.

Then, we have things that will happen only if both flags are True and here again we have a symmetry between left and right.

Documentation

Having magic numbers all over the place makes the code hard to understand. It might be worth adding documentation about what the different variables are supposed to hold and the meaning for the different values. In particular LM_state being 0, 1, 2 or 3 is still very cryptic to me. Maybe you could try to use something like enumerations to give these values a cool name.

StackExchange Code Review Q#117196, answer score: 4