[Solved] UDP Design Question

[Yendor]

I have a problem, but I think its more a design flaw in the way I’m thinking/Implementing and I would like some suggestions or examples in how to do it the correct way.

my full code is over on github but below is a cut down version to see how I did what I did:

What I’m trying to do here is write UDP packets from a subclass. i.e. I have a vector of objects that I have pass a reference to a UDP object with which they can use to write their UDP commands. but when I get more than 1 object in the vector it start throwing error (works with one object).

I’m thinking I need to build some kind of UDP class with a stack that I add to then it can start sending messages if there is something on the stack. but I’m at a loss of where to start this one.

anyways, the cut down code:

#include "lifx.h"

uint16_t remotePort = 56700;

lifx LIFX;
UDP _udp;

void setup() {
	
	IPAddress myIP = WiFi.localIP();
	broadcastIP = IPAddress(myIP[0], myIP[1], myIP[2], 255);

	_udp.begin(remotePort);
	_udp.joinMulticast(broadcastIP);

	LIFX = lifx();
	LIFX.setUDP(_udp);
	LIFX.setBroadcastIP(broadcastIP);
	LIFX.setRemotePort(remotePort);

	LIFX.togglePower();

}

---------------------------------------------------------------

#include "light.h"
class lifx{

	std::vector<light> Lights;

	void lifx::setUDP(UDP &udpRef)
	{
		_lifxUdp = udpRef;
	}

	void lifx::addLight(uint8_t mac[6], uint32_t port)
	{
		light _light = light();
		Lights.push_back (_light);
		Lights.back().setMAC(mac);
		Lights.back().setPort(port);
		Lights.back().setUDP(_lifxUdp);
		Lights.back().setBroadcastIP(_broadcastIP);
		Lights.back().setRemotePort(_remotePort);
	}

	void lifx::togglePower()
	{
		for(auto &Light : Lights)
		{
			uint16_t level = Light.getPowerLevel();
			level = level > 0 ? 0 : 65535;
			Light.setPower(level, 0);
		}
	}
}

---------------------------------------------------------------

class light{

	void light::setUDP(UDP &udpRef)
	{
		_lightUdp = udpRef;
	}

	void light::setPower(uint16_t level, uint32_t duration)
	{
		if (WiFi.ready()) {
			if(_lightUdp.sendPacket(udpPacket, sizeof(udpPacket), _broadcastIP, _lamp.port) < 0)
			{
				Serial.printlnf(Time.timeStr() + ":" + millis() + " - Light setPower - Error in sending UDP");
			}
		}
	}

	uint16_t light::getPowerLevel()
	{
		return _lamp.level;
	}
}

Hopefully the above gave you and idea of how I got the UDP reference down to the light object. once again the full code can be found on github.

It would be greatly appreciated if anyone could give or point my too and example of multiplexing the send of UDP packest.

Thanks.

[lami]

Seems you are working on same thing i tried a while ago
Not sure what exactly you need, but in the set of bulbs, there is single one acting as gateway, so all commands go to a single address/port and target is defined in payload.

[ScruffR]

Just a side note:
If you actually want to use multicast (as indicated with this line _udp.joinMulticast(broadcastIP);) you’d rather use an address falling into the range of multicast addresses (224.0.0.0 ~ 239.255.255.255).

See https://en.wikipedia.org/wiki/Multicast_address

Otherwise just stick with broadcasting.

[Yendor]

Thanks @ScruffR,
Didn’t really know what I was doing there, I was just trying everything to strat with just to get my UDP packets out and once it was working (with one object) didn’t go back and cut out what wasn’t needed. but I’ll cut out the ‘joinMulticast’ command and see what happens.

And Thanks @lami,
I’ve set up targeting as part of building the header, so I don’t think that’s my problem here:

[code]void light::setPower(uint16_t level, uint32_t duration)
{
/* header */
Header header = Header();
int16_t headerSize = sizeof(header);

/* payload */
uint8_t payload[6];
int16_t payloadSize = sizeof(payload);
_lamp.level = level;

/* UDP Packet */
uint8_t udpPacket[headerSize + payloadSize];

/* build header */
header.size = headerSize + payloadSize;
//header.origin = 0;
header.tagged = 0;
header.addressable = 1;
header.protocol = 1024;
header.source = _myID;
header.target[0] = _lamp.mac[0];
header.target[1] = _lamp.mac[1];
header.target[2] = _lamp.mac[2];
header.target[3] = _lamp.mac[3];
header.target[4] = _lamp.mac[4];
header.target[5] = _lamp.mac[5];
//header.target[6] = 0;
//header.target[7] = 0;
//header.reservedA[0] = 0;
//header.reservedA[1] = 0;
//header.reservedA[2] = 0;
//header.reservedA[3] = 0;
//header.reservedA[4] = 0;
//header.reservedA[5] = 0;
//header.reservedB = 0;
//header.ack_required = 0;
header.res_required = 1;
//header.sequence = 0;
//header.reservedC = 0;
header.type = _lightSetPower;
//header.reservedD = 0;

/* build payload */
/* Level, 0 or 65535 uint16_t */
payload[0] = (_lamp.level) & 0xff;
payload[1] = (_lamp.level >> 8) & 0xff;
/* Duration, transition time in milliseconds uint32_t */
payload[2] = (duration) & 0xff;
payload[3] = (duration >> 8) & 0xff;
payload[4] = (duration >> 16) & 0xff;
payload[5] = (duration >> 24) & 0xff;

/* build udp packet */
/* header */
memcpy(&udpPacket, &header, headerSize);
/* payload */
for (uint8_t i = 0; i < payloadSize; i++)
{
    udpPacket[headerSize + i] = payload[i];
}

/* Send UDP Packet */
// TODO
if (WiFi.ready()) {
    // Serial.printlnf("Light setPower - Sending UDP to 192.168.1.255:%d", _lamp.port);
    _lightUdp.beginPacket(_broadcastIP, _lamp.port);
    _lightUdp.write(udpPacket, sizeof(udpPacket));
    _lightUdp.endPacket();
    _msgSentTime = millis();
    _msgSent = true;
}

#if _DEBUG > 2
    Serial.printf("Light setPower - UDP: 0x");
    for(uint8_t i = 0; i < sizeof(udpPacket); i++)
    {
        Serial.printf("%02X ", udpPacket[i]);
    }
    Serial.println("");
#endif

}[/code]

after having a sleep I think I should be able to do something like (warning untested brain dump code):[code]class myUDP{

std::vector<byte> _udpPacket;
std::vector<_udpPacket> _udpPackets;
UDP _udp;

void myUDP::initialise(IPAddress broadcastIP, uint16_t remotePort)
{
	_udp.begin(remotePort);
	_broadcastIP = broadcastIP;
}

void myUDP::add(std::vector<byte> udpPacket)
{
	_udpPackets.push_back(udpPacket);
}

void myUDP::send()
{
	if (WiFi.ready()) {
		for(auto &_packet : _udpPackets)
	    {
			if(_Udp.sendPacket(_packet, sizeof(_packet), _broadcastIP, _lamp.port) < 0)
			{
				Serial.printlnf(Time.timeStr() + ":" + millis() + " - myUDP send - Error in sending UDP");
			}
		}
		_udpPackets.clear();
	}		
}

bool myUDP::available()
{
	if(_udpPackets.size() > 0)
	{
		return true;
	} else {
		return false;
	}
}

}[/code] and google has lead me to believe that you can call myUDP::send from a child (or grandchild) class but I still don’t see how it would know what instance if any that the packet was added, maybe I just pass down a reference to the instance as I did with UDP to start with.

I’ll give feedback on how it goes.

[Yendor]

I could be doing this all wrong so feel free to point out anything I’m missing.

I built a UDP class (lifxUDP) that I can add udp messages to for send later, but when I check the vector size from the main loop its always reporting a size of 0, but if I keep adding messages from a grandchild class the vector increases in size as reported from that class.

So I’m thinking I have somehow got two or more instants of the lifxUDP class object, but I’d be stuff if I can see what I’ve done wrong.

heres the new lifxUDP class: (lifxUDP.h)[code]/*

• lifxUDP.h - library for multitasking sending of UDP packest.
  */
  #ifndef _lifxUDP_h
  #define _lifxUDP_h
  // includes
  #include “common.h”

class lifxUDP{
public:
/* Members Functions /
lifxUDP();
void initialise(IPAddress broadcastIP, uint32_t remotePort);
void add(byte udpPacket);
void send();
bool available();
void begin(int port);
int parsePacket();
int read(byte* packetBuffer, int maxSize);
IPAddress remoteIP();
int remotePort();
void flush();

  /* Members */

private:
  /* Members Functions */

  /* Members */
  uint32_t _remotePort;
  IPAddress _broadcastIP;
	std::vector<std::vector<byte>> _udpPackets;
	UDP _myUdp;

};
#endif
[/code] lifxUDP.cpp [code]/*

• lifxUDP.cpp - library for multitasking sending of UDP packest.
  */

#include “lifxUDP.h”

lifxUDP::lifxUDP()
{

}

void lifxUDP::initialise(IPAddress broadcastIP, uint32_t remotePort)
{
_remotePort = remotePort;
_broadcastIP = broadcastIP;
_myUdp.begin(remotePort);
}

void lifxUDP::add(byte* udpPacket)
{
std::vector data(udpPacket, udpPacket + sizeof(udpPacket));
_udpPackets.push_back(data);
Serial.printlnf(Time.timeStr() + “:” + millis() + " - lifxUDP add - Message added, vector size: %d", _udpPackets.size());
}

void lifxUDP::send()
{
Serial.printlnf(Time.timeStr() + “:” + millis() + " - lifxUDP send - Sending…");
if (WiFi.ready())
{
for(auto &_vPacket : _udpPackets)
{
byte* _packet = &_vPacket[0];
if(_myUdp.sendPacket(_packet, sizeof(_packet), _broadcastIP, _remotePort) < 0)
{
Serial.printlnf(Time.timeStr() + “:” + millis() + " - lifxUDP send - Error: Can’t send UDP");
}
}
_udpPackets.clear();
} else {
Serial.printlnf(Time.timeStr() + “:” + millis() + " - lifxUDP send - Error: WiFi is down…");
}
}

void lifxUDP::begin(int port)
{
_myUdp.begin(port);
}

int lifxUDP::parsePacket()
{
return _myUdp.parsePacket();
}

int lifxUDP::read(byte* packetBuffer, int maxSize)
{
return _myUdp.read(packetBuffer, maxSize);
}

IPAddress lifxUDP::remoteIP()
{
return _myUdp.remoteIP();
}

int lifxUDP::remotePort()
{
return _myUdp.remotePort();
}

void lifxUDP::flush()
{
_myUdp.flush();
}

bool lifxUDP::available()
{
//Serial.printlnf(Time.timeStr() + “:” + millis() + " - lifxUDP available - Available: %d", _udpPackets.size());
if(_udpPackets.size() > 0)
{
return true;
}
return false;
}
[/code]

and then I pass the reference down and call like:(cut down code) [code]ino

lifx LIFX = lifx();
lifxUDP _lifxUDP = lifxUDP();

void setup() {

_lifxUDP.initialise(broadcastIP, remotePort);

// Setup the LIFX object
LIFX.setUDP(_lifxUDP);

LIFX.getStatus();

}

void loop() {

if(_lifxUDP.available() == true)
{
  Serial.printlnf(Time.timeStr() + ":" + millis() + " - UDP message available for sending...");
  _lifxUDP.send();
}

}

---

lifx.cpp

void lifx::setUDP(lifxUDP &udpRef)
{
_lifxUdp = udpRef;
_device.setUDP(_lifxUdp);
}

---

device.cpp

void device::setUDP(lifxUDP &udpRef)
{
// Serial.printlnf(“device setUDP…”);
_deviceUdp = udpRef;
}

void device::getPower()
{

/* --cut-- build udp packet --cut-- */


/* Send UDP Packet */
_deviceUdp.add(udpPacket);

}[/code]

I get a debug log (cut down) like:Sun Jun 19 10:43:54 2016:8670 - State: 2100 Sun Jun 19 10:43:54 2016:8671 - lifxUDP available - Available: 0 Sun Jun 19 10:43:54 2016:8671 - lifx discover... Sun Jun 19 10:43:54 2016:8671 - device getPower... Sun Jun 19 10:43:54 2016:8672 - lifxUDP add - Message added, vector size: 2 Sun Jun 19 10:43:54 2016:8673 - Deivce getPower - UDP: 0x24 00 00 34 16 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 14 00 00 00 Sun Jun 19 10:43:54 2016:8676 - lifxUDP available - Available: 0 Sun Jun 19 10:43:54 2016:8689 - State: 2100 - Claim.... Sun Jun 19 10:43:54 2016:8690 - lifxUDP available - Available: 0 Sun Jun 19 10:43:54 2016:8767 - State: 2100 - Claim....

Any Ideas?

[lami]

I may be a bit off at this one, but I think the culprit is this part:

void lifx::setUDP(lifxUDP &udpRef)
{
    _lifxUdp = udpRef;
    _device.setUDP(_lifxUdp);
}

You pass the reference thanks to &udpRef, but it is not a pointer. So assigning it should assign it by value, thus copying the object. But I would need to try, since our company uses C# now, my C++ is rusty.

Anyway, I would use pointer for _lifxUdp, something like:

void lifx::setUDP(lifxUDP *pUDP)
{
    _lifxUdp = pUDP;
    _device.setUDP(_lifxUdp);
}

Now it is pointer which I know it is safe to pass around, although you need to rewrite all to x->method().

[Yendor]

Thanks @lami
I’ll give that a go. still leaning C++ as I hack this project together.

[Yendor]

@lami, just letting you know that did the trick, I can now send all my UDPcommands with out failure.

Thanks again for your help.