Using Interrupts to read RC signal and send it via UDP

Hello
I want to read the PWM signal from RC receiver and write this value to a servo that is connected to the arduino and also I want to send this PWM value as a UDP packet to other Arduino
for the first part I used interrupt and everything works very well.
But when I tried to send the PWM value via UDP nothing was working (both the servo and the UDP)
I use Arduino UNO and Ethernet Shield
and here is the code:

#include<Servo.h>
#include<SPI.h>
#include<Ethernet.h>
#include<EthernetUdp.h>

/////////////////////////////////////////////////////////////////////////////////////////////////////////
// Network Configurations
/////////////////////////////////////////////////////////////////////////////////////////////////////////

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };

IPAddress sourceIp(192, 168, 1, 177);
unsigned int sourcePort = 8888; // local port to listen on

IPAddress destIP(192,168,1,170);
unsigned int destPort = 8889;

EthernetUDP Udp;

byte signalBuffer[2]= {0};

/////////////////////////////////////////////////////////////////////////////////////////////////////////

// channel in pins
#define SIGNAL_IN_ID 0 // interrupt ID of pin 2
#define SIGNAL_IN_PIN 2
// channel output pin
#define SIGNAL_OUT_PIN 5

Servo servo;
// Bit Flag, that is set in bUpdateFlagsShared to indicating the new Signal.
#define SIGNAL_FLAG 1
// shared Flag
volatile uint8_t bUpdateFlagsShared;
// Shared variables
volatile uint16_t unSignalInShared;

// Next variables are only used the the interrupt routine.
uint32_t ulSignalStart;

void setup() {
Serial.begin(9600);
servo.attach(SIGNAL_OUT_PIN);
attachInterrupt(SIGNAL_IN_ID,calculatePWM,CHANGE);

// start the Ethernet and UDP:
Ethernet.begin(mac,sourceIp);
Udp.begin(sourcePort);
}

void loop() {
// local variables to copy the shared variables.
static uint16_t unSignalIn;
static uint8_t bUpdateFlags;
// check if new signal
if(bUpdateFlagsShared)
{
// turn interrupts off quickly and make a copy of the shared variables
noInterrupts();
bUpdateFlags = bUpdateFlagsShared;
if(bUpdateFlags & SIGNAL_FLAG)
{
unSignalIn = unSignalInShared;
}
bUpdateFlagsShared = 0;
interrupts();
}

if(bUpdateFlags & SIGNAL_FLAG)
{
if(servo.readMicroseconds() != unSignalIn)
{
servo.writeMicroseconds(unSignalIn);
Serial.println(unSignalIn);
}
// send the value via UDP... this may be moved inside the if block
signalBuffer[0] = unSignalIn;
signalBuffer[1] = unSignalIn >> 8;

Udp.beginPacket(destIP, destPort);
Udp.write(signalBuffer,2);
Udp.endPacket();
}
bUpdateFlags = 0;
}
// simple interrupt service routine
void calculatePWM()
{
if(digitalRead(SIGNAL_IN_PIN) == HIGH)
{
ulSignalStart = micros();
}
else
{
unSignalInShared = (uint16_t)(micros() - ulSignalStart);

bUpdateFlagsShared |= SIGNAL_FLAG;
}
}

any ideas to solve this problem?
thank you and best regards

Have you tried without blocking interrupts and basically not changing your values in the ISR until the flag has been reset by the main loop? This way you don't need to make the local copy, so No need to suspend interrupts

ulSignalStart isn't declared volatile, that sticks out immediately. Any variable that communicates
between one call of an ISR and another call of any ISR or the main program must be declared
volatile or risk the compiler optimizing incorrectly.

Why are you using a flag to say unSignalInShared has changed? Just read it (safely) since you check
for change later anyway.

As for you actual problem, perhaps your interrupts are interacting - what exact library and shield are
you using for the UDP? Does it use interrupts for transfering data?

Have you had UDP sends working by themselves?

J-M-L:
Have you tried without blocking interrupts and basically not changing your values in the ISR until the flag has been reset by the main loop? This way you don't need to make the local copy, so No need to suspend interrupts

Are you grasping towards saying 'use a semaphore'? That's more complex and unnecessary, interrupts()
and noInterrupts() compile to single instructions each.

And how exactly can an ISR wait for the main program to progress anyway?

MarkT:
Are you grasping towards saying 'use a semaphore'? That's more complex and unnecessary, interrupts()
and noInterrupts() compile to single instructions each.

And how exactly can an ISR wait for the main program to progress anyway?

His ISR calculates unSignalInShared and sets a flag for the main loop to know that a data is avail. Main loop handles the data and resets the flag.

He is blocking interrupts for a short while in main loop to ensure access of 2 bytes long data is not corrupted by an inconveniently timed ISR (murphy's law) that would modify the data while doing operations.

I'm suggesting that his ISR could first check if the flag is set then do nothing because it means the loop is still working on handling that data. This way no need to block interrupts - possibly means missing a rising edge and thus loosing an update of PWM but if that happens then it means there is a risk the ISR calculates anyway 2 PWM before the loops handles it and you loose one anyway for the transmission. A serial.print at 9600 bauds fills the small buffer pretty quickly for example, making then the call blocking. So there is an easy way to test if this can be part of his pb.

Otherwise I disagree with your general comment on

ulSignalStart isn't declared volatile, that sticks out immediately. Any variable that communicates between one call of an ISR and another call of any ISR or the main program must be declared volatile or risk the compiler optimizing incorrectly.

In his case ulSignalStart is only used in the ISR so even if compiler is to optimize data access by keeping it into a register that's fine while the ISR executes and the data is guaranteed to be set in memory at max at the end of the ISR function. Since an ISR Won't be interrupted by itself on arduino, there is no pb there for his usage.

J-M-L:
Have you tried without blocking interrupts and basically not changing your values in the ISR until the flag has been reset by the main loop? This way you don't need to make the local copy, so No need to suspend interrupts

Thank you very much for your suggestion....
I tried what you said as shown below and it works very well.
#include<Servo.h>
#include<Ethernet.h>
#include<EthernetUdp.h>

/////////////////////////////////////////////////////////////////////////////////////////////////////////
// Network Configurations
/////////////////////////////////////////////////////////////////////////////////////////////////////////

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };

IPAddress sourceIp(192, 168, 1, 177);
unsigned int sourcePort = 8888; // local port to listen on

IPAddress destIP(192,168,1,170);
unsigned int destPort = 8889;

EthernetUDP Udp;

byte signalBuffer[2]= {0};

/////////////////////////////////////////////////////////////////////////////////////////////////////////

// Assign your channel in pins
#define SIGNAL_IN_ID 0 // interrupt ID of pin 2
#define SIGNAL_IN_PIN 2
// Assign your channel output pin any pwm pin (3,5,6,9,10,11)
#define SIGNAL_OUT_PIN 5

Servo servo;

volatile byte newSignal = 0x00;
volatile uint16_t unSignalIn;

// Next variables are only used the the routine, so they do not need to be volatile.
uint32_t ulSignalStart;

void setup() {
Serial.begin(9600);
servo.attach(SIGNAL_OUT_PIN);
attachInterrupt(SIGNAL_IN_ID,calculatePWM,CHANGE);

// start the Ethernet and UDP:
Ethernet.begin(mac,sourceIp);
Udp.begin(sourcePort);
}

void loop() {
// check if new signal
if(newSignal)
{
if(servo.readMicroseconds() != unSignalIn)
{
servo.writeMicroseconds(unSignalIn);
Serial.println(unSignalIn);
}
// here we can add udp packet.
Serial.println("No Change");
signalBuffer[0] = unSignalIn;
signalBuffer[1] = unSignalIn >> 8;

Udp.beginPacket(destIP, destPort);
Udp.write(signalBuffer,2);
Udp.endPacket();
newSignal = 0x00;
}
}

// simple interrupt service routine
void calculatePWM()
{
if(!newSignal)
{
// if the pin is high, its a rising edge of the signal pulse, so lets record its value
if(digitalRead(SIGNAL_IN_PIN) == HIGH)
{
ulSignalStart = micros();
}
else
{
unSignalIn = (uint16_t)(micros() - ulSignalStart);
newSignal = 1;
}
}

}

Good to hear!

MarkT:
ulSignalStart isn't declared volatile, that sticks out immediately. Any variable that communicates
between one call of an ISR and another call of any ISR or the main program must be declared
volatile or risk the compiler optimizing incorrectly.

Why are you using a flag to say unSignalInShared has changed? Just read it (safely) since you check
for change later anyway.

As for you actual problem, perhaps your interrupts are interacting - what exact library and shield are
you using for the UDP? Does it use interrupts for transfering data?

Have you had UDP sends working by themselves?

thank you very much for your help
actually my code works very well when the next three lines of code are commented
Udp.beginPacket(destIP, destPort);
Udp.write(signalBuffer,2);
Udp.endPacket();

I took a look to the source code of the library of UDP it seems that it does not use interrupt at least in the source file that I checked.

Those functions do call SPI.beginTransaction(), which does disable some interrupts during the SPI transaction. I'm not certain which one(s).