MotorControlLibrary for Arduino MotorShields R3 - opinions&improvements

UPDATE 08.09.2015
Hello guys,

Here is the updated Library with improved comments and functions. (I put it into the Annex)

Header:

#ifndef MotorControl_h
#define MotorControl_h
#include "Arduino.h"

class MotorControl
{
public:
	MotorControl(int pinPWM, int pinBrake, int pinDirection);
	void 	attachCS(int pinCS);
	void 	setPWM(int valuePWM);    
	void 	incrPWM(int steps, int delay);
	void 	decrPWM(int steps, int delay);
	void	brakeOn(); 
	void 	brakeOff();
	float 	readCS();
	int  	readPWM();
	void 	changeDirection();
	char* 	readDirection();
	bool 	readBrake();
	void    setDirection(char* Direction);

private:
	const float volts_per_amp = 1.65;
	const float resolution = 5.00 / 1023.00;
	int 	_valueCS;
	int 	_pinCS;
	int 	_valuePWM;  
	int 	_pinBrake;
	int 	_pinDirection;
	int 	_pinPWM;
	bool 	_stateBrake;
	bool 	_attached;
	bool 	_attachedCS = false;
	char* 	_stateDirection;
};

#endif

Sourcecode:

#include "Arduino.h"
#include "MotorControl.h"

MotorControl::MotorControl(int pinPWM, int pinBrake, int pinDirection):_pinPWM(pinPWM), _pinBrake(pinBrake), _pinDirection(pinDirection){
	pinMode(_pinBrake, OUTPUT);
	pinMode(_pinDirection, OUTPUT);
	digitalWrite(_pinBrake, LOW);
	digitalWrite(_pinDirection, HIGH);
	_valuePWM = 0;
	_stateDirection = "FWD";
	_attached = true;
}
// Generate a instance of the object and hand over the connectet Pins.


void MotorControl::setPWM(int valuePWM){
	if(_attached &&! digitalRead(_pinBrake) ){
		_valuePWM = valuePWM;
		_valuePWM = constrain(_valuePWM, 0, 255);
		analogWrite(_pinPWM, _valuePWM);
	}
}
// Write a PWM-value between 0-255 to _pinPWM.


void MotorControl::incrPWM(int steps, int _delay){
	if(_attached && !_stateBrake ){
		unsigned long previousMillis = millis();
		do{
			unsigned long currentMillis = millis();
			if(currentMillis - previousMillis >= _delay){
				_valuePWM++;
				steps--;
				if(_valuePWM>255){
				_valuePWM = 255;
				return;
				}
			setPWM(_valuePWM);
			previousMillis += _delay;
			}
		}while(steps>0);
	}
}
// Increment the actual PWM-value by 1 per step in a cicle and writes it to the _pinPWM. Each repetition is performed every "delay" - milliSeconds. 
// Returns if all steps are performed or the PWM-value go beyond 255.


void MotorControl::decrPWM(int steps, int _delay){
	if(_attached && !_stateBrake ){
		unsigned long previousMillis = millis();
		do{
			unsigned long currentMillis = millis();
			if(currentMillis - previousMillis >= _delay){
				_valuePWM--;
				steps--;
				if(_valuePWM<0){
					_valuePWM = 0;
					return;
				}
			setPWM(_valuePWM);
			previousMillis += _delay;

			}
		}while(steps>0);
	}
}
// Decrement the actual PWM-value by 1 per step in a cicle and writes it to the _pinPWM. Each repetition is performed every "delay" - milliSeconds. 
// Returns if all steps are performed the PWM-value goes below 0.


void MotorControl::brakeOn(void){
	if(_attached && !_stateBrake){
		digitalWrite(_pinBrake, HIGH);
		_stateBrake = true;
	}
}
// Turn the brake on.


void MotorControl::brakeOff(void){
	if(_attached && _stateBrake){
		digitalWrite(_pinBrake, LOW);
		_stateBrake = false;
	}
}
// Turn the brake off.


void MotorControl::changeDirection(void){
	if(_attached){
		if(_stateDirection == "REV"){
			digitalWrite(_pinDirection, HIGH);
			_stateDirection = "FWD";
		}
		else if(_stateDirection == "FWD"){
			digitalWrite(_pinDirection, LOW);
			_stateDirection = "REV";
		}
	}
}
// Change the actual direction of the motor. 


int MotorControl::readPWM(){
	if(_attached){
		return _valuePWM;
	}
}
// Read the actual PWM of the motor. Return the PWM-value as integer.


char* MotorControl::readDirection(void){
	if(_attached){
		return _stateDirection;
	}
}
// Read the actual direction of the DC-motor. Return the value as char*.


void MotorControl::setDirection(char* Direction){
	if(Direction=="FWD"){
		digitalWrite(_pinDirection, HIGH);
	}
	else if(Direction=="REV"){
		digitalWrite(_pinDirection, LOW);
	}
}
// Set the direction of the DC-motor to forward("FWD") or reverse("REV").


bool MotorControl::readBrake(void){
	if(_attached){
		return _stateBrake;
	}
}
// Read the actual state of the brake. Return the value as bool.


void MotorControl::attachCS(int pinCS){
	if(_attached){
		_attachedCS = true;
		_pinCS = pinCS;
	}		 			
}
// Attach the current-sensing pin to the instance. Activate the readCS()-Function.


float MotorControl::readCS(void){
	if(_attachedCS){
		float _y = 0.00;
		for(int x = 0; x < 200; x++){
			_valueCS = analogRead(_pinCS);
			_y = (1.00-0.99) * _valueCS + (0.99 * _y);    												
		}
		_y = _y * resolution;                         													
		_y = _y / volts_per_amp;                       													
		_y = _y * 1000.00;                             													
		return _y; 																						
	}
}
// Read the current used by the motor. Return the value in milliAmps.

31.08.2015stale Version, for current Version scroll down to my next post

Hello Community,
I startet to teach me programming on my own last week. As an entry I thought it is good to start with Arduino. And yes, it was a good idea:)
Now I have completet my first own library in the last two days with the idea of being able to control motors via the Arduino MotorShield R3 even faster and Code-saving, , but primarily in order to learn even more.
It would be very nice if you could check my library and tell me what you think about it and my codes and give me suggestions for improvements to increase my learning process even more.

Best regards and thanks in advance,
EATEX
I have packed the files with comments in the Annex because they exceed the maximum lenght

Header:

#ifndef MotorControl_h

#define MotorControl_h
#include <inttypes.h>
#include “Arduino.h”

class Motor
{
public:
 Motor();
 void attach(int pinPWM, int pinBrake, int pinDirection);
 void attachCS(int pinCS);
 void setSpeed(int valuePWM);    
 void incrSpeed(int steps, int msBetSteps);
 void decrSpeed(int steps, int msBetSteps);
 void brakeOn();
 void brakeOff();
 void changeDirection();
 float readCS();
 int   readPWM();
 char* readDirection();
 char* readBrake();

private:
 const float volts_per_amp = 1.65;
 const float resolution = 5.00 / 1023.00;
 float _y;
 int _valueCS;
 int _pinCS;
 int _valuePWM;  
 int _pinBrake;
 int _pinDirection;
 int _pinPWM;
 char* _stateDirection;
 char* _stateBrake;
 int _attached = 0;
};

#endif




Source-File:


#ifndef MotorControl_h
#define MotorControl_h
#include <inttypes.h>
#include “Arduino.h”

class Motor
{
public:
 Motor();
 void attach(int pinPWM, int pinBrake, int pinDirection);
 void attachCS(int pinCS);
 void setSpeed(int valuePWM);    
 void incrSpeed(int steps, int msBetSteps);
 void decrSpeed(int steps, int msBetSteps);
 void brakeOn();
 void brakeOff();
 void changeDirection();
 float readCS();
 int   readPWM();
 char* readDirection();
 char* readBrake();

private:
 const float volts_per_amp = 1.65;
 const float resolution = 5.00 / 1023.00;
 float _y;
 int _valueCS;
 int _pinCS;
 int _valuePWM;  
 int _pinBrake;
 int _pinDirection;
 int _pinPWM;
 char* _stateDirection;
 char* _stateBrake;
 int _attached = 0;
};

#endif

MotorControl.h (3.95 KB)

MotorControl.cpp (7.06 KB)

Typically, the include guard name (and the file name) should be the same as the class name.

float Motor::readCS()
{
	if(_attached)
{
	_y= 0.00;
	for(int x=0; x < 1000; x++)                     	
	{
		_valueCS = analogRead(_pinCS);
		_y = (1.00-0.99) * _valueCS + (0.99 * _y);    	// Average of 1k measurements and smoothing
	}
	_y = _y * resolution;                         		// Analog to V
	_y = _y / volts_per_amp;                       		// V to A
	_y = _y * 1000.00;                             		// A to mA
	return _y; 											//return current in mA as float
	}
}

Why is _y a class member? It looks like it should be a local variable. Reading the current level 1000 times seems excessive. Some of the comments, like the last one, are useless.

Brakes are either on or off. That can be conveyed in a one byte boolean, better that a 9 character string.

	float read.CS()
		returns the actual current flow through the Motor as float (in mAmp)

The function name is not read.CS().

Thank you PaulS for your attentive and constructive criticism!
It would be nice if you could give me some examples of the useless comments and how you would improve them, because commenting well is one of the Points i struggle with.

I fixed the issues that you have found, improved the incrSpeed()- and decrSpeed()-functions and scaled down the readCS() to average of 200 times instead of 1k times.

Greetings,
EATEX

Header:

#ifndef MotorControl_h
#define MotorControl_h
#include "Arduino.h"
class MotorControl
{
public:
	MotorControl(int pinPWM, int pinBrake, int pinDirection);
	void 	attachCS(int pinCS);
	void 	setSpeed(int valuePWM);    
	void 	incrSpeed(int steps, int delay);
	void 	decrSpeed(int steps, int delay);
	void	brakeOn(); 
	void 	brakeOff();
	float 	readCS();
	int  	readPWM();
	char* 	changeDirection();
	char* 	readDirection();
	bool 	readBrake();

private:
	const float volts_per_amp = 1.65;
	const float resolution = 5.00 / 1023.00;
	int 	_valueCS;
	int 	_pinCS;
	int 	_valuePWM;  
	int 	_pinBrake;
	int 	_pinDirection;
	int 	_pinPWM;
	bool 	_stateBrake;
	bool 	_attached;
	bool 	_attachedCS = false;
	char* 	_stateDirection;
};

#endif

Sourcecode:

MotorControl::MotorControl(int pinPWM, int pinBrake, int pinDirection):_pinPWM(pinPWM), _pinBrake(pinBrake), _pinDirection(pinDirection)
{
	pinMode(pinBrake, OUTPUT);
	pinMode(pinDirection, OUTPUT);
	digitalWrite(pinBrake, LOW);
	digitalWrite(pinDirection, HIGH);
	_valuePWM = 0;
	_stateDirection = "FWD";
	_attached = true;
}

void MotorControl::setSpeed(int valuePWM)
{
	if(!_attached||_pinBrake==HIGH) return;
	_valuePWM = valuePWM;
	_valuePWM = constrain(_valuePWM, 0, 255);
	analogWrite(_pinPWM, _valuePWM);
}

void MotorControl::incrSpeed(int steps, int _delay)
{
	if(!_attached||_pinBrake==HIGH) return;
	unsigned long previousMillis = millis();
	do
	{
		unsigned long currentMillis = millis();
		if(currentMillis - previousMillis >= _delay) 
		{
			_valuePWM++;
			steps--;
			if(_valuePWM>255)
			{
				_valuePWM = 255;
				return;
			}
			setSpeed(_valuePWM);
			previousMillis += _delay;
		}
	}while(steps>0);
}

void MotorControl::decrSpeed(int steps, int _delay)		
{
	if(!_attached||_pinBrake==HIGH) return;
	unsigned long previousMillis = millis();
	do
	{
		unsigned long currentMillis = millis();
		if(currentMillis - previousMillis >= _delay) 
		{
			_valuePWM--;
			steps--;
			if(_valuePWM<0)
			{
				_valuePWM = 0;
				return;
			}
			setSpeed(_valuePWM);
			previousMillis += _delay;
		}
	}while(steps>0);
}


void MotorControl::brakeOn(void)																				
{
	if(!_attached) return;
	if(!digitalRead(_pinBrake))
	{
		digitalWrite(_pinBrake, HIGH);
		analogWrite(_pinPWM, 0);
	}
}


void MotorControl::brakeOff(void)																			
{
	if(!_attached) return;
	if(digitalRead(_pinBrake))
	{
		digitalWrite(_pinBrake, LOW);
		analogWrite(_pinPWM, _valuePWM);	
	}
}

char* MotorControl::changeDirection(void)																		
{
	if(!_attached) return "not Attached!";
	if(digitalRead(_pinDirection)==LOW)
	{
		digitalWrite(_pinDirection, HIGH);
		_stateDirection = "FWD";
	}
	else if(digitalRead(_pinDirection)==HIGH)
	{
		digitalWrite(_pinDirection, LOW);
		_stateDirection = "REV";
	}
}


int MotorControl::readPWM(void)
{
	if(_attached)
	{
		return _valuePWM;																						}
}


char* MotorControl::readDirection(void)																				
{
	if(_attached)
	{
		return _stateDirection;
	}
}


bool MotorControl::readBrake(void)																			{
	if(_attached) 
	{
		if(digitalRead(_pinBrake)) _stateBrake=true; else _stateBrake=false;
		return _stateBrake;
	}
}


void MotorControl::attachCS(int pinCS)																		
{
	_attachedCS = true;
	_pinCS=pinCS;
	if(!_attached) return;      							
}


float MotorControl::readCS(void)
{
	if(_attachedCS)
	{
		float _y = 0.00;
		for(int x = 0; x < 200; x++)                     	
		{
			_valueCS = analogRead(_pinCS);
			_y = (1.00-0.99) * _valueCS + (0.99 * _y);    												
		}
		_y = _y * resolution;                         													// Analog to V
		_y = _y / volts_per_amp;                       													// V to A
		_y = _y * 1000.00;                             													// A to mA
		return _y; 																}
}

A useless comment is one like this:

	return _y; 											//return current in mA as float

Anyone can see that this is a return statement. Anyone looking at the variable type can see that it is a float. A much better comment would precede the function:

// Read the current used by the motor. Return the value in milliAmps.

That way, one gets an overview of the function and understands why the value is converted to milliAmps.

PaulS:
A useless comment is one like this:

	return _y; 											//return current in mA as float

Anyone can see that this is a return statement. Anyone looking at the variable type can see that it is a float. A much better comment would precede the function:

// Read the current used by the motor. Return the value in milliAmps.

That way, one gets an overview of the function and understands why the value is converted to milliAmps.

Thank you very much for this example, PaulS. It helped me much to understand whats a good comment.
I´m going to improve my comments the next days and update the files as soon as possible.

Greetings,
EATEX