Skip to content

sparkfun quadstep driver refactor

i bought a quadstep from sparkfun to run stepper motors.

the supplied arduino driver

  1. did a good job of showing the quadsteps functionality
  2. is free
  3. was limited to the arduino mega
  4. had hardcoded pins
  5. could use some cleanup

 

i have also been reading clean code: a handbook of agile software craftsmanship by robert martin and thought this would be a good opportunity to apply some of the concepts.

When re-factoring it is critical to have tests. I want to make incremental code changes and test after each one. I want to improve the code, not break it. The supplied drive only supports the the Arduino Mega or Mega 2560. This is because of hardcoded pins in the driver. I will need to fix the hardcoded pins before I am able to test the driver with my hardware.

soft-coding the step pin

The step pin is hardcoded for all motors. I believe the author did this because the stepping speed was a concern and the author was considering using analogWrite to do pulse width modulation (pwm). PWM was never used so there is no longer a need for hardcoded pins.

In this library the pins for each connection are stored in a private variable. an additional private variable for each channel is needed to hold each step pin number. another argument is added to the motor_pins function so the variable can be initialised with the desired pin.

The cbi / sbi functions can all be replaced with digital writes. The newer arduino libraries use cbi/sbi in the background when the pin can be determined at compile time. there is A LOT of duplicated code that i really want to take out but I will resist until i get a test working.

https://github.com/johnjamesmiller/quadstep/commit/699e94e261a5fd950ca38ca5c2ecc28945b91b2c

Repeated code

repeated code is a place for bugs to hide. if you have to make the same change multiple places what are the chances you will do it exactly the same each time? the quadstep.cpp file is 630 lines long now and most of it is repeat code. lets start condensing some code.

motor_pins


	if(motnum == 1)
	{
	        repeatedPinModeFunctions();
       	        uniqueVariableInitilizations = parameter;
	}
	else if(motnum == 2)
	{
	        repeatedPinModeFunctions();
       	        uniqueVariableInitilizations = parameter;
	}
... continued for motors 3 and 4

In the motor_pins code there are 8 lines of pin mode functions repeated for each motor. extract the duplicated code out of the if/else blocks and execute it before.


       	
	pinMode(motor_enable, OUTPUT);
	pinMode(motor_dir, OUTPUT);
	pinMode(motor_step, OUTPUT);
	pinMode(motor_ms1, OUTPUT);
	pinMode(motor_ms2, OUTPUT);
	pinMode(motor_ms3, OUTPUT);
	digitalWrite(motor_enable, HIGH);
	digitalWrite(motor_dir, LOW);
	
	if(motnum == 1)
	{
		_motor_enable_1 = motor_enable;
		...
	}
	else if(motnum == 2)
	{
		_motor_enable_2 = motor_enable;
		...
	}
... continued for motors 3 and 4

motor_go

Each motor has the same stepping code repeated 5 times for the 5 different stepping values. The hierarchy is:


motor1
 if(step_size == 1)
                //sets speed
                current_control(1);

                //sets step_size
                digitalWrite(_motor_ms_11, LOW);    
                digitalWrite(_motor_ms_12, LOW);    
                digitalWrite(_motor_ms_13, LOW);

		digitalWrite(_motor_enable_1, LOW);
		for(int i=1;i<=number_of_steps;i++)
		{
			//low to high transition moves one step
			digitalWrite(_motor_step_1, HIGH);
			delayMicroseconds(step1);
			digitalWrite(_motor_step_1, LOW);
			delayMicroseconds(step1);
		}
		digitalWrite(_motor_step_1, LOW);
		digitalWrite(_motor_enable_1, HIGH);
 else if(step_size == 2)
                //sets speed
                current_control(2);

                //sets step_size
                digitalWrite(_motor_ms_11, HIGH);    
                digitalWrite(_motor_ms_12, LOW);    
                digitalWrite(_motor_ms_13, LOW);


		digitalWrite(_motor_enable_1, LOW);
		for(int i=1;i<=number_of_steps;i++)
		{
			//low to high transition moves one step
			digitalWrite(_motor_step_1, HIGH);
			delayMicroseconds(step2);
			digitalWrite(_motor_step_1, LOW);
			delayMicroseconds(step2);
		}
		digitalWrite(_motor_step_1, LOW);
		digitalWrite(_motor_enable_1, HIGH);
 step4
 step8
 step16
motor2
 step1
 step2
 step4
 step8
 step16
motor3
 step1
 step2
 step4
 step8
 step16
motor4
 step1
 step2
 step4
 step8
 step16

each stepN is a block of 18 very similar lines of code. the current control is called at the beginning of each block to set one of the step size variables (step1, step2, step4, step8, step16). I am going to condense the 5 variables to 1. Now the current_control function is a lot smaller but more importantly each block is more similar and does not have to be repeated. Each block is left with 3 unique lines. the other 15 lines only need to be repeated once per motor.

Wow I am down from 630 lines of code to 323. What did I lose in those 300 lines of code? previously you were forced to pass 1,2,4,8,16 or it wouldnt step. now I can pass any number, probably not a good idea.

enums

I will add an enum and pass that instead of an integer. This will force the user of the library to select one of our values. This also adds clarity when reading the code. now instead of seeing the counter-intuitive number 16 you see SIXTEENTH and you know it is a smaller step then FULL.

A reminder that it is important to make small changes and test after each change, insuring that the code still works.

https://github.com/johnjamesmiller/quadstep/commit/b2ddf7dd29cae197ae17d06dd6b71f21242df0ad

comments are a failure to express yourself in code.

What is worse then no information at all? wrong information. wrong information is worse. Most comments are a duplication of information in the code. As the code is changed comments do not get updated. There is real world examples of this in the original code (quadstep.cpp line 369, 379, quadstep.pde line 61). This code has only been around for a year and maintained by one person. Imagine what happens to large enterprise projects after 10 years and 20 people have roughed up the comments. eventually maintainers ignore all of the comments. some comments are good but before you use one think long and hard if it is the best thing to do. or just be lazy and don’t put any comments.

This is the single easiest take away from clean code. Whenever you see a comment above a block of code turn that comment into a function name and put the code block in the function. If the comment is above a function consider renaming it.


	//sets speed
	current_control(step_size);

In eclipse use alt+shift+r to rename a function and eclipse will automatically update all the places the function is used.


	set_speed(step_size);

There are quite a few more examples in this code but I will let them be for now.

classes

A class holds variables and the functions that act on them. The variables in the quadstep class are the pins for each of the 4 motors and the functions that act on them. The functions are exactly the same except for the variables they act on. The class should be condensed to a unique set of functions. It should handle one motor. Each motor should be an instance of that class.

Now using the code looks like this:


quadstep motor1;
quadstep motor2;

motor1.motor_pins(0,1,2,3,4,9);
motor2.motor_pins(7,6,A0,A1,A2,5);

motor1.motor_go(FULL,-200,2);
motor2.motor_go(FULL,200,2);

motor1.stall();
motor2.stall();

There are many other “smells” that say this should happen. The functions first argument is a flag, the motor number. Flags indicate that the function does more then one thing. The “Single Responsibility Principle” says that functions should do only one thing. Switches and if else chains are another indicator that the function has more then one responsibility.

The cpp file is now down to 115 lines from the original 630 lines.

https://github.com/johnjamesmiller/quadstep/commit/a3fac1927270252f4ef979f75a69117dd2be6199

comments, round 2

Now that I don’t have to make 4 direction functions, 4 enable functions, 4 ect I can extract each of the commented blocks of code out. The idea here is to separate levels of abstraction. The motor ‘go’ function needs to know that you must set the direction, enable the motor, step, and then disable the motor. The motor ‘go’ function does NOT need to know how you enable the motor, that is a lower level of abstraction.

The motor go function is now much more readable


void quadstep::go(step_modes_t step_size, int number_of_steps, int torque)
{
	_torque = torque;
	set_direction(number_of_steps);
	set_speed(step_size);
	set_microstep_format(step_size);
	enable();
	for(int i=1;i<=abs(number_of_steps);i++)
	{
		step();
	}
	disable();
}

https://github.com/johnjamesmiller/quadstep/commit/af13236e47700707083e0f2459a844d7a2218ccd

hidden temporal couplings

The _torque member is the only variable directly set here. Interesting, what does it do? It is used by the set_speed function. If I were to call the set_speed function first it would not behave correctly. The torque parameter should be passed to the set_speed function so the temporal coupling is exposed. It turns out that is the only place torque is used so I can delete the member variable.

https://github.com/johnjamesmiller/quadstep/commit/2ae188f3d39a598d189f4be56ae0611912b9a76f

avoid mental mappings

Setting the motor pins is annoying. I have to make look back and forth between


  motor_pins(y,z,l,m,n,o)
  y: enable pin assignment
  z: direction pin assignment
  l: MS1 pin assignment
  m: MS2 pin assignment
  n: MS3 pin assignment
  o: step pin

and this


  motor1.motor_pins(0,1,2,3,4,9); //ch 1
  motor2.motor_pins(7,6,A0,A1,A2,5);      //ch 2
  motor3.motor_pins(12,13,A0,A1,A2,10);  //ch 3
  motor4.motor_pins(4,27,A0,A1,A2,11);  //ch 4

several times to make sure I put the correct pin in the correct location. Your brain cycles are too valuable for this. Any time you find yourself mentally mapping between one thing and the other you should consider changing the code.

Setting the pins has now become self explanatory.


  motor1.set_enable_pin(0);
  motor1.set_direction_pin(1);
  motor1.set_step_pin(9);
  motor1.set_microstep_select_pins(2,3,4);

This has expanded the code a bit but I am ok with it because the code is now more readable and self explanatory.

https://github.com/johnjamesmiller/quadstep/commit/04aafea841ba8923951801aa4027c070e9562f42

conclusion

Try to make your intentions as clear as possible. Make small changes. Test after every change. Take pride in your code. That reminds me I should add my name to the headers.

git://github.com/johnjamesmiller/quadstep.git

I am sure there are more ways this code can be improved but I am tired of rambling in this excessively long post.

Post a Comment

Your email is never published nor shared. Required fields are marked *
*
*