2

I am new to programming and Arduinos but I've decided to give a go for a project. I'm after a code that will read PWM signals from a RC receiver (Specifically aileron and throttle) and output this as 4 PWM signals to drive two H Bridges (Right motor forward/reverse and left motor forward/reverse).

This is my code that isn't working.

unsigned long timer[3];
byte last_channel[2];
int throttle;
int aileron;

void setup() {
pinMode(8, OUTPUT);//Right motor fowards
pinMode(9, OUTPUT);//Right motor reverse
pinMode(10, OUTPUT);//left motor fowards
pinMode(11, OUTPUT);//left motor reverse

PCICR |= (1 << PCIE0);  //This enables pin change intterupt enabler

PCMSK0 |= (1 << PCINT0); // This tells the mask register 
which pins will cause an intterupt (Pin 2)

PCMSK0 |= (1 << PCINT1); // (Pin 3)

Serial.begin(9600); //Sets the data rate in bits per second
}

void loop() {


}
ISR(PCINT0_vect) { 

timer[0] = micros(); // registers intterupt time

//Throttle_Channel----------------------------------------------------------          

if(last_channel[0] == 0 && PINB & B00000001 ) { // This checks if pin has 
changed from low to high

last_channel[0] = 1;

timer[1] = timer[0];
}

else if(last_channel[0] == 1 && !(PINB & B00000001)){ 

last_channel[0] = 0;

throttle = timer[0] - timer[1]; //calculates the change in time and sets to 

input[0]
//--------------------------------------------------------------------------
}
//Aileron_Channel-----------------------------------------------------------

if(last_channel[1] == 0 && PINB & B00000010 ) { // This checks if pin has 

changed from low to high

last_channel[1] = 1;

timer[2] = timer[0];
}

else if(last_channel[0] == 1 && !(PINB & B00000010)){ //This checks if pin  has 

changed from high to low

last_channel[1] = 0;

aileron = timer[0] - timer[2]; //calculates the change in time and sets to i

input[0]
//--------------------------------------------------------------------------
}

//Calculations to put output in correct form--------------------------------
int throttle_pwm = (throttle - 1500) / 1000;

int aileron_pwm = (aileron - 1500) / 1000;

int v = (0.5 - abs(aileron_pwm)) * (throttle_pwm / 0.5) + throttle_pwm;

int w = (0.5 - abs(throttle_pwm)) * (aileron_pwm / 0.5) + aileron_pwm;

int lm = v + (w/2) * 255;

int rm = v - (w/2) * 255;
//--------------------------------------------------------------------------


if (lm > 0 && rm > 0){

analogWrite(8,rm);

analogWrite(9,0);

analogWrite(10,lm);

analogWrite(11,0);
}

else if (lm > 0 && rm <= 0){

analogWrite(8,0);

analogWrite(9,abs(rm));

analogWrite(10,lm);

analogWrite(11,0);
}

else if (lm <= 0 && rm > 0){

analogWrite(8,rm);

analogWrite(9,0);

analogWrite(10,0);

analogWrite(11,abs(lm));

}

else if (lm <= 0 && rm <= 0){

analogWrite(8,0);

analogWrite(9,abs(rm));

analogWrite(10,0);

analogWrite(11,abs(lm));

//--------------------------------------------------------------------------
}
}

It compiles fine but I've tried measure PWM at the four output and it doesn't work. If anyone could offer advice or guidance it would be greatly appreciated.

  • 1
    It'd be really helpful if you could reformat your title ("What's wrong with my code" doesn't really tell us anything about the problem) and reformat the post so the code is easier to read. I believe four spaces on each line will format it in the "coding" format. – Hari Ganti Mar 2 '17 at 19:32
  • Thanks for the advice I've made the suggested layout edit. :) – sacred230 Mar 2 '17 at 20:08
  • If this is an RC controller, have you considered using a serial data protocol, like SPI over ZigBee? PWM signals are great for power control to analog devices, but when you have two microcontrollers communicating, a serial protocol would probably be better. – Hari Ganti Mar 2 '17 at 20:26
1

It looks like the code in the ISR is slow and it may compute incorrect results.

It is slow because of using floating point arithmetic in throttle_pwm and aileron_pwm calculations. It may be so slow that the trailing edge of a PWM pulse occurs while the leading edge still is being processed. This will delay trailing edge processing, and mis-measurement of pulse lengths.

The main thing to do to reduce the amount of code executed in the ISR is separate the input measuring phase from the output actuating phase. You could compute pulse widths and store them in volatile variables for a loop in main to pick up and use in analogWrite() statements there.


Note, in 16-bit int arithmetic, the value of expression (t-1500)/1000 is 0 for 500 < t < 2500. Because throttle and aileron are supposed to be RC pulse lengths in the range 1000 to 2000 μs, your throttle_pwm and aileron_pwm will be zero. That means you can replace the following code,

int throttle_pwm = (throttle - 1500) / 1000;
int aileron_pwm = (aileron - 1500) / 1000;
int v = (0.5 - abs(aileron_pwm)) * (throttle_pwm / 0.5) + throttle_pwm;
int w = (0.5 - abs(throttle_pwm)) * (aileron_pwm / 0.5) + aileron_pwm;
int lm = v + (w/2) * 255;
int rm = v - (w/2) * 255;

with this code:

int throttle_pwm=0, aileron_pwm=0, v=0, w=0, lm=0, rm=0;

which should save you several milliseconds of computation! Also, having lm and rm as constant zeroes means that you don't need all those lengthy and laborious if and else blocks. You can replace those 24 lines of stuff with just the following:

analogWrite(8,0);
analogWrite(9,0);
analogWrite(10,0);
analogWrite(11,0);

Slightly more seriously, it looks like you intended for your arithmetic like (throttle - 1500) / 1000, etc. to determine whether to raise or lower the throttle, etc. Instead of doing that time-consuming division, just say:

int throttle_pwm = throttle > 1500 ? 1 : -1;  // or whatever values you meant to have
int aileron_pwm = aileron > 1500 ? 1 : -1;    // ditto

Instead of all those 24 lines of stuff, just say:

analogWrite(8,  (rm>0) * Level);
analogWrite(9,  (rm<=0)* Level);
analogWrite(10, (lm>0) * Level);
analogWrite(11, (lm<=0)* Level);

where Level is some level you want to set the control to. Note, an expression like (rm>0) has a value 1 when it is true and and 0 when it is false. So when rm>0, you have, in effect, analogWrite(8,Level); and analogWrite(9,0);.

If instead you end up with a calculation where lm and rm are actually usable PWM levels, ie numbers from 0 to 255, you could instead say:

analogWrite(8,   (rm>0) * rm);
analogWrite(9,  -(rm<=0)* rm);
analogWrite(10,  (lm>0) * lm);
analogWrite(11, -(lm<=0)* lm);

or could say:

analogWrite(8,  rm>0  ?  rm : 0);
analogWrite(9,  rm<=0 ? -rm : 0);
analogWrite(10, lm>0  ?  lm : 0);
analogWrite(11, lm<=0 ? -lm : 0);

Also see my answer to the question Read RC receiver channels using Interrupt instead of PulseIn and see my answer to the question Arduino read PWM duty cycle value with input (analog / digital).

0

See my page about interrupts. Data to be shared between an ISR and the main code should be declared volatile which yours isn't.


Also see:

https://www.arduino.cc/en/Reference/softwareSerial

Not all pins on the Mega and Mega 2560 support change interrupts, so only the following can be used for RX: 10, 11, 12, 13, 14, 15, 50, 51, 52, 53, A8 (62), A9 (63), A10 (64), A11 (65), A12 (66), A13 (67), A14 (68), A15 (69).

It's not really clear from your post which pins you are using for the encoder, but you mention pins 2 and 3:

PCMSK0 |= (1 << PCINT0); // (Pin 2)
PCMSK0 |= (1 << PCINT1); // (Pin 3)

Pins 2 and 3 on the board are not PCINT0 and PCINT1. Also they are not in the list of supported pin-change interrupt pins.

You might be better off using external interrupts (see my page above). External interrupts are supported on pin 2 and 3. However your code needs to change a bit.

0

If you are trying to read a PWM signal, you should try using the pulseIn function, which reads the length of a pulse signal.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.