Malformed packets when used with Oscuino

[Axagthoth]

Hi,
I've justed started this same issue on the Oscuino end, but since I'm not sure which library is generating the problem, I'm opening it here too. Maybe someone has also encountered this problem?
I'm trying to use Oscuino with the new Arduino MKR1000 board, which uses the WiFi101 library (a modified version of the WiFi library) to communicate.
Receiving messages on the board works like a charm, I've tried it both with TouchOSC and Processing, and have managed to control one of the PWM pins on the board remotely.
The problem appears when trying to send messages from the board to the computer: no matter what I try, I can't seem to create "correct" OSC messages that my receivers can interpret.
Checking with the example codes I think the issue can only lie with one of the three following calls:

Udp.beginPacket(remIP, remPort);
msg.send(Udp);
Udp.endPacket();

I've reached this conclusion because comparing the code of WifiUDP with that of EthernetUDP (with which the same code works) I've seen that the code is quite different, but I'm not savvy enough to identify which pieces of the code might be critical.
The question is: do any of these functions add/require something to/from the UDP packet? The reason I'm asking this is that going deeper and analyzing the packets with Wireshark, I've realized that (at least) precisely 4 bytes are wrong, so maybe something is screwing the 4-padding requirement of OSC up? Here are the images of the capture that make me suspect this (I'm trying to send "layer1/clip1/connect 1"):

Any help and ideas would be greatly appreciated. Thanks a lot in advance!

[sandeepmistry]

@Axagthoth thank you for bringing this to our attention.

Could you please provide a minimal sketch to reproduce the issue? (ideally without OSC lib etc).

[Axagthoth]

@sandeepmistry thanks for answering so fast! Sure, I can provide an example sketch, but I'm not sure I understand what you mean by "without OSC lib". That's precisely what generates the issue. I can send "normal" UDP packets just fine (just like the WiFiUDPSendReceiveString example does). What should I attach, exactly?

[sandeepmistry]

It would be nice to isolate the issue from the Oscuino library. So it would be great if you could reproduce just using the WiFi101 library (by using WiFiUDP to send similar data).

Anyways, how about you start with sharing a sketch that others can run to reproduce the issue, even if it includes Oscuino.

[Axagthoth]

Oh, right! Excuse my denseness, it's getting late on this side of the ocean :)

Here's the sketch WITH OSC. In a while I'll share the one without.

#include <SPI.h>
#include <WiFi101.h>
#include <WiFiUdp.h>
#include <OSCMessage.h>

int status = WL_IDLE_STATUS;
char ssid[] = "yourSsid";
char pass[] = "yourWiFiPass";
int locPort = 12000;
int remPort = 10000;
int ledPin = 6;
int ledState = LOW;
WiFiUDP Udp;

void setup() {
  Serial.begin(9600);
  Serial.println("OSC test");

 // check for the presence of the shield:
  if (WiFi.status() == WL_NO_SHIELD) {
    Serial.println("WiFi shield not present");
    // don't continue:
    while (true);
  }

  // attempt to connect to Wifi network:
  while ( status != WL_CONNECTED) {
    Serial.print("Attempting to connect to SSID: ");
    Serial.println(ssid);
    // Connect to WPA/WPA2 network. Change this line if using open or WEP network:
    status = WiFi.begin(ssid, pass);

    // wait 10 seconds for connection:
    delay(5000);
  }
  Serial.println("Connected to wifi");
  printWifiStatus();

  Udp.begin(locPort);
}

void loop() {
  delay(5000);
  OSCMessage msg("/layer1/clip1/connect");
  msg.add((int32_t)1);
  Udp.beginPacket("192.168.1.6", remPort);
  msg.send(Udp);
  Udp.endPacket();
  msg.empty();

}

void printWifiStatus() {
  // print the SSID of the network you're attached to:
  Serial.print("SSID: ");
  Serial.println(WiFi.SSID());

  // print your WiFi shield's IP address:
  IPAddress ip = WiFi.localIP();
  Serial.print("IP Address: ");
  Serial.println(ip);

  // print the received signal strength:
  long rssi = WiFi.RSSI();
  Serial.print("signal strength (RSSI):");
  Serial.print(rssi);
  Serial.println(" dBm");
}

[Axagthoth]

Ok, this should be the same exact message I'm trying to send with OSC, but translated to raw ASCII chars, as per this specification. Maybe you are right, and it isn't Oscuino after all! I've been checking the packets with Wireshark on my side, and I get the same packet as in my first picture, with 17 bytes of data! Even though now they can be clearly counted as being 32 bytes.

#include <SPI.h>
#include <WiFi101.h>
#include <WiFiUdp.h>

int status = WL_IDLE_STATUS;
char ssid[] = "yourSsid";
char pass[] = "yourWiFiPass";
int locPort = 12000;
int remPort = 10000;
int ledPin = 6;
int ledState = LOW;
WiFiUDP Udp;

void setup() {
  Serial.begin(9600);
  Serial.println("OSC test");

 // check for the presence of the shield:
  if (WiFi.status() == WL_NO_SHIELD) {
    Serial.println("WiFi shield not present");
    // don't continue:
    while (true);
  }

  // attempt to connect to Wifi network:
  while ( status != WL_CONNECTED) {
    Serial.print("Attempting to connect to SSID: ");
    Serial.println(ssid);
    // Connect to WPA/WPA2 network. Change this line if using open or WEP network:
    status = WiFi.begin(ssid, pass);

    // wait 10 seconds for connection:
    delay(5000);
  }
  Serial.println("Connected to wifi");
  printWifiStatus();

  Udp.begin(locPort);
}

void loop() {
  delay(5000);
  // /layer1/clip1/connect 1 int
  char rawOSC[] = { 47, 108,  97, 121,
                   101, 114,  49,  47,
                    99, 108, 105, 112,
                    49,  47,  99, 111,
                   110, 110, 101,  99,
                   116,   0,   0,   0,
                    44, 105,   0,   0,
                     0,   0,   0,   1
                   };
  Udp.beginPacket("192.168.1.6", remPort);
  Udp.write(rawOSC);
  Udp.endPacket();  
}

void printWifiStatus() {
  // print the SSID of the network you're attached to:
  Serial.print("SSID: ");
  Serial.println(WiFi.SSID());

  // print your WiFi shield's IP address:
  IPAddress ip = WiFi.localIP();
  Serial.print("IP Address: ");
  Serial.println(ip);

  // print the received signal strength:
  long rssi = WiFi.RSSI();
  Serial.print("signal strength (RSSI):");
  Serial.print(rssi);
  Serial.println(" dBm");
}

[sandeepmistry]

@Axagthoth nice work!

[Axagthoth]

@sandeepmistry @adrianfreed Ok, looking into the code in WiFiUDP.cpp I decided to change this line in my code:

Udp.write(rawOSC);

for this line (the second definition of the write function, with a pointer and the size):

Udp.write(rawOSC, 32);

And things got a little better. Now the message data doesn't get truncated at the end, thus reaching the ending 28 bytes of correct hexadecimal values, but it somehow gets cut at the head (the first 4 bytes are still somehow considered part of the header and not the data). Maybe it has something to do with the defines in socket_buffer.h? I hope this is helpful and not a bother; like I said earlier I don't know enough about sockets to pinpoint the exact problem.

[sandeepmistry]

Hi @Axagthoth,

I think everything is ok, I tried it out locally. Initially I was seeing issues, but then I disabled the LLC protocol decoding. This can be done by:

1. Right clicking on a packet
2. Selecting "Protocol Preferences"
3. "Disable LLC ..."

Now I see this:

So, Wireshark is just confusing us by auto-enabling LLC decoding.

You should definitely be using Udp.write(rawOSC, 32); instead of Udp.write(rawOSC); because rawOSC is not a null terminated string.

This is why you are seeing: Udp.write(rawOSC) will loop through the char array until a 0x00 character is found.

I'm going to close this now. Thank you for providing a detailed issue report and also investigating.

[Axagthoth]

I disagree with the issue being marked as resolved, as it is not. It's interesting to have found out that Wireshark wrongly interpreted the headers, but the fact remains that something fishy is going on when trying to encode the messages with OSCMessage::send(Print &p) from the Oscuino library (which was the original issue); and other programs than Wireshark aren't receiving the messages either.
If you compare the bytes in my first post with the ones in my fifth post, you will clearly notice that in the Oscuino case they get truncated, even though they should be the same (independently of how Wireshark distributes them):

Oscuino:
2f 6c 61 79 65 72 31 2f 63 6c 69 70 2f 63 6f 6e 6e 65 63 74 00

rawOSC:
2f 6c 61 79 65 72 31 2f 63 6c 69 70 2f 63 6f 6e 6e 65 63 74 00 00 00 2c 69 00 00 00 00 00 01

That's twenty bytes missing at the tail. I'd really rather being able to leverage the power of the Oscuino library (which has worked before) than having to write the raw OSC messages each time.

[sandeepmistry]

Please see my comment about the use of write(const char *str) vs write(const uint8_t *buffer, size_t size).

The OSC library needs to use write(const uint8_t *buffer, size_t size) otherwise only the data before a 0x00 byte will be sent. Which lines up with what you are seeing.

[cmaglie]

@Axagthoth
It would be nice to have a look at the OSCuino library too, can you provide a link to the library?

[sandeepmistry]

I'm re-opening this, WiFiUDP is not buffering UDP TX packets.

[Axagthoth]

Yep, sure: Oscuino

I was gonna post the code of the OSCMessage::send(Print &p) function too:

OSCMessage& OSCMessage::send(Print &p){
    //don't send a message with errors
    if (hasError()){
        return *this;
    }
    uint8_t nullChar = '\0';
    //send the address
    int addrLen = strlen(address) + 1;
    //padding amount
    int addrPad = padSize(addrLen);
    //write it to the stream
    p.write((uint8_t *) address, addrLen);
    //add the padding
    while(addrPad--){
        p.write(nullChar);
    }
    //add the comma seperator
    p.write((uint8_t) ',');
    //add the types
#ifdef PAULSSUGGESTION
    // Paul suggested buffering on the stack
    // to improve performance. The problem is this could exhaust the stack
    // for long complex messages
    {
        uint8_t typstr[dataCount];

        for (int i = 0; i < dataCount; i++){
            typstr[i] =  getType(i);
        }
        p.write(typstr,dataCount);
    }
#else
    for (int i = 0; i < dataCount; i++){
        p.write((uint8_t) getType(i));
    }
#endif
    //pad the types
    int typePad = padSize(dataCount + 1); // 1 is for the comma
    if (typePad == 0){
            typePad = 4;  // This is because the type string has to be null terminated
    }
    while(typePad--){
        p.write(nullChar);
    }
    //write the data
    for (int i = 0; i < dataCount; i++){
        OSCData * datum = getOSCData(i);
        if ((datum->type == 's') || (datum->type == 'b')){
            p.write(datum->data.b, datum->bytes);
            int dataPad = padSize(datum->bytes);
            while(dataPad--){
                p.write(nullChar);
            }
        } else if (datum->type == 'd'){
            double d = BigEndian(datum->data.d);
            uint8_t * ptr = (uint8_t *) &d;
            p.write(ptr, 8);
        } else if (datum->type == 't'){
            osctime_t time =  datum->data.time;
            uint32_t d = BigEndian(time.seconds);
            uint8_t * ptr = (uint8_t *)    &d;
            p.write(ptr, 4);
            d = BigEndian(time.fractionofseconds);
            ptr = (uint8_t *)    &d;
            p.write(ptr, 4);

        } else if (datum->type == 'T' || datum->type == 'F')
                    { }
        else { // float or int
            uint32_t i = BigEndian(datum->data.i);
            uint8_t * ptr = (uint8_t *) &i;
            p.write(ptr, datum->bytes);
        }
    }
    return *this;
}

So, as you can see it use both signatures of the write function, but whenever it uses write(const char *str) its because the data is exactly one byte. Maybe printing '\0' to the stream is a problem? Perhaps a better implementation would be writing to a buffer, and the just using one write(const uint8_t *buffer, size_t size) call?

[sandeepmistry]

@Axagthoth thank for this.

Maybe printing '\0' to the stream is a problem? Perhaps a better implementation would be writing to a buffer, and the just using one write(const uint8_t *buffer, size_t size) call?

This would be more of a work around. WiFiUDP is not correctly buffering writes between beginPacket(addressOrHost, port) and endPacket(), this needs to be fixed. Each write is sent as a separate packet, which is incorrect.

[Axagthoth]

@sandeepmistry Aaah I see. Thanks a lot for all the info! What you say about the buffering certainly sounds coherent with the actual problem. I'm afraid I can't be of further help there, though :( Should I change the title of the issue?

[sandeepmistry]

@Axagthoth

Should I change the title of the issue?

No it's ok, we can leave it as is.

I've submitted #59 to resolve this problem. We've also contacted Atmel to see if buffer UDP data inside the WINC1500, then we can save RAM on the Arduino side. If this is possible I'll open another PR instead of #59.

[Axagthoth]

@sandeepmistry ok, I've tested #59 and the issue seems resolved! Thanks a lot! Gonna make some further tests with beast-sized packets just to be on the safe side...

[sandeepmistry]

@Axagthoth excellent, thanks for trying it out!

I've made some notes on the buffer size here: #59 (comment).

[cmaglie]

Fixed by #59