Dimming LED with udp and bushbutton

Hi Im looking for a little help.

I am trying to dim an LED via either udp (from iphone) or by using a push button by holding it down.
I can do these individually but im having trouble putting them both together giving me the 2 switching options.
i have managed to dim the LED with the push button but as soon as i let go of the button the LED will go back to the udp state.
im hoping its something obvious so please forgive me if it is as i am quite new to all this.
here is the code(bit messy)

#include <SPI.h>         // for Arduino later than ver 0018
#include <EthernetUdp.h>   // UDP library from bjoern@cs.stanford.edu
#include <Ethernet.h>
         
                         // source:  http://code.google.com/p/arduino/source/browse/trunk/libraries/?r=1094#libraries%2FEthernet 
                         
//////////  NETWORK INFO  ////////////////

byte mac[] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED };  //Set your Ethernet Shield's MAC address here - make sure you replace the ZZs with your shield's values!
byte ip[] = { 192,168,0, 177 };    // Set your shield's desired IP address here - check your network for configuration details
//byte gateway[] = { 192,168,0,1 };   //if you need to set a gateway IP
//byte subnet[] = { 255,255,255,0 };    // Change this to your subnet address
  
unsigned int localPort = 7777;      // local port to listen on (set this the same as Port # on Ardumote Params Screen)

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

///////// Pin Assignments /////////////////
  
int LEDPin2 = 2;  //Set LEDPin to Pin 2 - Place an LED on Pin 2 of your ethernet shield for testing this code  (pwm)

int Button43 = 43;

int buttonState2;
int ledBrightness2 = 0;
int fadeAmount2 = 5;
int reading2;

long lastDebounceTime2 = 0;
long debounceDelay2 = 50;

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

// buffers for receiving and sending data
char packBuff[UDP_TX_PACKET_MAX_SIZE]; //buffer to hold incoming packet,

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

EthernetUDP Udp;


void setup() {
  
  // More info on Ethernet on Arduino's website:  http://arduino.cc/en/Reference/EthernetBegin
  // start the Ethernet and UDP:
  Ethernet.begin(mac,ip);   // If you don't need to set your default gateway or subnet manually, use this
//  Ethernet.begin(mac,ip,gateway,subnet);  // Use this line instead if you've manually set all the parameters

  Udp.begin(localPort);    //Setup UDP socket on port defined earlier
  Serial.begin(9600);    //Start Serial Communications with PC  
  pinMode(LEDPin2,OUTPUT);    //Designate pin 2 as Output Pin  
  pinMode(Button43,INPUT);
}

void loop()   
  {
       
  int pwmVal;    // Integer that will hold our PWM values for later use 
  // if there's data available, read a packet
  int packetSize = Udp.parsePacket(); // note that this includes the UDP header}
  
 if(packetSize)
  
    packetSize = packetSize - 8;      // subtract the 8 byte header
    Serial.print("Packet size: ");
    Serial.println(packetSize);

    // read the packet into packetBuffer and get the senders IP addr and port number
    Udp.read(packBuff,UDP_TX_PACKET_MAX_SIZE);
    Serial.println("Message: ");
    Serial.println(packBuff);
  
     pwmVal = (packBuff[3] - '0')*100 + (packBuff[4] - '0')*10 + (packBuff[5] - '0');    //Get PWMXXX message, and use XXX to set an int between 0 and 255.
 
 //////////////////////// Pin 2 (LED_Pin) /////////////////////////////////////       
  
     if (packBuff[0] = 'R' && packBuff[1]=='E' && packBuff[2]=='D')  // Wait for "REDXXX" and use XXX as value for PWM 
    {    
      analogWrite(LEDPin2,pwmVal);    //Set LED_Pin to PWM Value  
      
      Serial.println("RED on Pin 2");    //Write notification  
    }

    else if (packBuff[0] = 'P' && packBuff[1]=='2' && packBuff[2]=='H')  // If we get the message "P2H", then set LED_Pin (2) HIGH
    {     
      digitalWrite(LEDPin2,HIGH);    //Turn on LED_Pin
      
      Serial.println("LED ON");    //Write notification 
  
    }
    else if (packBuff[0] = 'P' && packBuff[1]=='2' && packBuff[2]=='L')  // If we get the message "P2L", then set LED_Pin (6) LOW
     {
       digitalWrite(LEDPin2,LOW);    //Turn off LED_Pin
  
      Serial.println("LED OFF");    //Write notification}     
  } 
  else if (reading2 != HIGH) {
    lastDebounceTime2 = millis();    
  }
  reading2 = digitalRead(Button43);
  
  if ((millis() - lastDebounceTime2) > debounceDelay2) {
    buttonState2 = reading2;
  }
  
  if (reading2 != HIGH) {
    ledBrightness2 = ledBrightness2 + fadeAmount2;
    if (ledBrightness2 <= 0 || ledBrightness2 >= 255) {
      fadeAmount2 = -fadeAmount2;
    }
    analogWrite(LEDPin2, ledBrightness2);
    
      delay(100);
  }
   
  delay(20);
  
 }
 if(packetSize)

If packetSize what? Turns green before Christmas?

  else if (reading2 != HIGH) {
    lastDebounceTime2 = millis();    
  }
  reading2 = digitalRead(Button43);

Test the variable, then assign it a value. Which end of the horse does the cart go on?

  reading2 = digitalRead(Button43);
  
  if ((millis() - lastDebounceTime2) > debounceDelay2) {
    buttonState2 = reading2;

I fail to see a relationship between buttonsState2 and reading2. Some meaningful names, like currState and prevState would make the relationship clear. You appear to have one switch. What's with the 2s in the names?

I gave up on trying to follow the code. The random indenting makes it to hard. Put each { on a new line, and use Tools + Auto format to greatly improve the layout of the code. Perhaps the problem (whatever it is) will become clearer.

You probably want to think about detecting the transition of the switch from released to pressed, rather than being concerned about just the current state.

Using different variables to set the PWM value to when the switch is pressed and when the UDP packet arrives is the root of your problem, though.

Thanks for ripping me a new hole :slight_smile:

Sorry about the mess as i say im very new to this.
i had taken bits form other another project that had more then one led hence the 2 after everything.

ill try neaten it up a bit

Thanks for ripping me a new hole

That certainly wasn't my intent. You begged, borrowed, or stole that code with the intent to make it yours. That's fair. That's why people post code.

But, when you do borrow someone else's code as your own, there is a certain level of expectation that you will understand what you have borrowed, and, more importantly, that you make the code you borrow fit with the code that you are adding it to. That includes proper indenting and renaming variables so that they make sense in your context.

I think that if you do that, and can see the structure of the program, that the issues will become apparent.

One of the things I do when reviewing code I haven't written, or haven't read in a while, is to print it out, and tape all the pages together. Then, I draw parallel lines to line up each { and }, so that I can see the indented blocks better.

Often, I'll see a block of code that makes more sense in a function than inline. Oh, goody, a chance to isolate some code and reduce the complexity of the main function.

For instance, you have code to get data from a UDP packet or from a switch. What if loop() looked like this?

void loop()
{
   if(!getUdpData())
   {
      getSwitchData();
   }
   digitalWrite(somePin, someVal);
}

Anyone can look at that and see the structure and flow, without even knowing what getUdpData() and getSwitchData() do.

So, if your loop() method looked like this, it would real clear, would it not, where to look for an issue? It's either got to be in getUdpData() or getSwitchData(), right?

Then, you'd be able to skip looking at half the code.

You'd also be able to test, in another sketch, that getUdpData() works properly, and, in another sketch, that getSwitchData() works properly.

The only possible problem, then, is that one of them is stepping on the other one's data. That is easy enough to solve, by having functions return values, instead of using global variables for everything.

Thanks for the advice
i have finally had a chance to play with this again but i am still struggling.

I have changed the way to dim the led with a button which works fine but it still returns to the udp state after i release the button weather it is the anologwrite or digitalwrite that the iphone previously sent.

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

byte mac[] = { 
  0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED }; 
byte ip[] = { 
  192,168,0, 177 };    
unsigned int localPort = 7777;     
int LEDPin = 2;  
int Button = 43;
int dir = 1;
int lum = 0;
boolean toggle = false;

char packBuff[UDP_TX_PACKET_MAX_SIZE]; 
EthernetUDP Udp;
void setup() {
  Ethernet.begin(mac,ip);   
  Udp.begin(localPort);   
  Serial.begin(9600);     
  pinMode(LEDPin,OUTPUT);      
  pinMode(Button,INPUT);
}
void loop()   
{
  int pwmVal;     
  int packetSize = Udp.parsePacket();
  packetSize = packetSize - 8;    
  Serial.print("Packet size: ");
  Serial.println(packetSize);
  Udp.read(packBuff,UDP_TX_PACKET_MAX_SIZE);
  Serial.println("Message: ");
  Serial.println(packBuff);

  pwmVal = (packBuff[3] - '0')*100 + (packBuff[4] - '0')*10 + (packBuff[5] - '0');   

  if (packBuff[0] = 'R' && packBuff[1]=='E' && packBuff[2]=='D')   
  {  
    analogWrite(LEDPin,pwmVal);           
  }
  else if (packBuff[0] = 'P' && packBuff[1]=='2' && packBuff[2]=='H')  
  {     
    digitalWrite(LEDPin,HIGH);   
  }
  else if (packBuff[0] = 'P' && packBuff[1]=='2' && packBuff[2]=='L') 
  {
    digitalWrite(LEDPin,LOW);          
  } 
  if (digitalRead(Button) == LOW) {
    toggle = true;
    lum += dir;
    if ((lum >= 0) && (lum < 256))
      analogWrite(LEDPin, lum);
    delay(20);
  } 
  else { // toggle direction
    if (toggle) {
      dir = -1 * dir;
      toggle = false;
    }
  }
  delay(20);

}

if i just use button or udp on its own they work fine but to combine the two im just getting stuck.
i had a look at function return values you mentioned but just got a little more confused as i say i am really new to code and arduino.
is it actualy possible to have two different anologwrites for one pin? anymore more advise would be great!

  int pwmVal;     
  int packetSize = Udp.parsePacket();
  packetSize = packetSize - 8;    
  Serial.print("Packet size: ");
  Serial.println(packetSize);
  Udp.read(packBuff,UDP_TX_PACKET_MAX_SIZE);
  Serial.println("Message: ");
  Serial.println(packBuff);

  pwmVal = (packBuff[3] - '0')*100 + (packBuff[4] - '0')*10 + (packBuff[5] - '0');

So, regardless of whether a packet arrived, or not, you calculate a PWM value based on the packet (that may not have arrived).

  if (packBuff[0] = 'R' && packBuff[1]=='E' && packBuff[2]=='D')   
  {  
    analogWrite(LEDPin,pwmVal);           
  }
  else if (packBuff[0] = 'P' && packBuff[1]=='2' && packBuff[2]=='H')  
  {     
    digitalWrite(LEDPin,HIGH);   
  }
  else if (packBuff[0] = 'P' && packBuff[1]=='2' && packBuff[2]=='L') 
  {
    digitalWrite(LEDPin,LOW);          
  }

Then, you set the value of the pins based on that packet (that may not have arrived).

  if (digitalRead(Button) == LOW) {
    toggle = true;
    lum += dir;
    if ((lum >= 0) && (lum < 256))
      analogWrite(LEDPin, lum);
    delay(20);
  } 
  else { // toggle direction
    if (toggle) {
      dir = -1 * dir;
      toggle = false;
    }
  }

Then, if the switch is being pressed, you set the LED to a the PWM value in a different variable.

Of course the LED is going to revert to the packet value if the switch is not pressed.

You need to calculate THE pwm value based on the packet only if there IS a packet.

You need to calculate THE pwm value if the switch is pressed.

Then, you need to assign THE pwm value to the pin, regardless of where it came from.