Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCP packets not parsed because of frame length prefix #82

Open
arlomedia opened this issue Dec 12, 2024 · 11 comments
Open

TCP packets not parsed because of frame length prefix #82

arlomedia opened this issue Dec 12, 2024 · 11 comments

Comments

@arlomedia
Copy link

I'm trying to use the TCP support added by @daveyarwood, but my server listener was only calling the handleBadData() function, with the error "No '44' present after the address, but there is still more data left in the message."

I dug into the OSCParser class and realized that it's not handling the first four bytes of the message, which is a prefix that provides the message size. If JavaOSC doesn't need that, I guess it should ignore or discard it somehow.

In my local copy of OSCParser, I edited the convertMessage function to rename the rawInput argument to rawInputWithLength, then added these lines at the start of the function:

ByteBuffer rawInput = null;
if (rawInputWithLength.limit() > 4) {
	rawInput = ByteBuffer.allocate(rawInputWithLength.limit() - 4);
	for(int i = 4; i < rawInputWithLength.limit(); i++) {
		rawInput.put(i - 4, rawInputWithLength.get(i));
	}
} else {
	rawInput = rawInputWithLength;
}

This discards the length prefix and then the parsing works correctly.

However, this should only happen with TCP packets, and I plan to use UDP for other functions in the future. So I also had to edit OSCPortInBuilder and OSCSerializerAndParserBuilder so the OSCParser would know what network protocol it's used with. In OSCPortInBuilder.build(), after

parserBuilder = new OSCSerializerAndParserBuilder();

I added

parserBuilder.setNetworkProtocol(networkProtocol);

And in OSCSerializerAndParserBuilder, I added

public void setNetworkProtocol(NetworkProtocol networkProtocol) {
	this.properties.put("networkProtocol", networkProtocol);
}

Then the new code in OSCParser.convertMessage() can check the protocol:

if ((properties.get("networkProtocol") == NetworkProtocol.TCP)&&(rawInputWithLength.limit() > 4)) {

Let me know if I have misdiagnosed the problem or if there's a better way to solve it.

@daveyarwood
Copy link
Collaborator

It's entirely possible that I missed something when I implemented TCP support. I don't know off-hand what the expectation is with TCP OSC clients -- is there never, sometimes, or always a length prefix? We may want to refer to the OSC spec as the authority on this. Or if there is some way we can detect when the first 4 bytes are a length prefix and act accordingly, gracefully handling either the presence or absence of a length prefix, that could be another option.

@arlomedia
Copy link
Author

arlomedia commented Dec 12, 2024

I couldn't find an official statement on this; I learned about the packet length header from this page:

https://community.etcconnect.com/control_consoles/eos-family-consoles/f/eos-family-show-control-support-midi-smpte-osc-rtc-etc/53331/eos-unresponsive-to-self-made-osc-messages/171720

With my current short messages, the first three bytes are null and the fourth byte is the size, but I'm guessing all four bytes could be used to show larger sizes, so we can't just look for a starting null byte. Maybe we could look for the first slash in the message, which would be the start of the address pattern ... but I'm very new to OSC and don't really know the answer.

Also, I assumed JavaOSC doesn't support SLIP as in OSC 1.1 because of this open issue. I did try sending data from my iOS app in SLIP format, and in that case the data byes were all the same except instead of an initial four bytes, there was a byte with a value of -64 at the start and end of the data. I could have removed those and continued the parsing, but I don't know what other differences might come up with the SLIP format so I figured it's best to stay with what JavaOSC was designed for.

Fortunately my app will only exchange TCP data with other instances of the same app, so I can use either OSC 1.0 or 1.1 -- as long as my iOS and Android app use OSC the same way. That said, it sounds like SLIP is the default for new applications now and I would prefer to use that if JavaOSC supported it.

@hoijui
Copy link
Owner

hoijui commented Dec 30, 2024

ok... I have no time to work on this, and I did not use it myself in over 15 years now, and I will not use it in the future either.
If you can self-organize and tell me what to do (e.g. make person X a moderator).. that would be best for me.
For what I care, both of you could be moderators.

@daveyarwood
Copy link
Collaborator

daveyarwood commented Dec 30, 2024

I'm in a similar boat, unfortunately - I don't really have much bandwidth these days to be a good maintainer.

@arlomedia
Copy link
Author

For what I care, both of you could be moderators.

Thanks for the offer, but I don't think I have enough experience with Java or Github to do that.

I did find that in addition to not handling packet length headers for incoming data, the TCPTransport also doesn't add packet length headers for outgoing individual messages. It does add them for outgoing bundles. Building on my additions above:

In OSCPortOutBuilder, after:

serializerBuilder = new OSCSerializerAndParserBuilder();

add:

serializerBuilder.setNetworkProtocol(networkProtocol);

In OSCSerializer, after:

writeArguments(message);

add:

if (properties.get("networkProtocol") == NetworkProtocol.TCP) {
	writePLH(message);
}

In OSCSerializer, add:

/**
 * Serializes a packet length header. Call this after writing the message and arguments.
 * @throws OSCSerializeException if the message failed to serialize
 */
private void writePLH() throws OSCSerializeException {
	byte[] rawOutput = output.toByteArray();
	int length = rawOutput.length;
	byte[] newOutput = new byte[4 + length];
	ByteBuffer.wrap(newOutput).putInt(length); // add the length
	ByteBuffer.wrap(newOutput, 4, length).put(rawOutput); // add the message content

	output.clear();
	output.put(newOutput);

	if (false) { // what to check here?
		throw new OSCSerializeException("Error adding packet length header");
	}
}

I see that the existing writeSizeAndData function does that differently, and could be adapted to handle individual messages as well as bundles. For now, the changes above are working for me. OSCKit in the iOS version of my app was rejecting messages until I added this.

@daveyarwood
Copy link
Collaborator

@arlomedia If you want to make a PR with this change, I'll be happy to test it locally with my project, Alda and see if the inter-process communication still works. The IPC for that project is between Java and Go processes using OSC over TCP, but the funny thing is that I'm the one who wrote TCP support for both the Java and the Go OSC libraries... so if it doesn't work, that might just be because the Go library also needs to be updated to send the header bytes 😂

@daveyarwood
Copy link
Collaborator

daveyarwood commented Jan 22, 2025

@arlomedia Apologies if you already covered this, but what's your setup on the sending and receiving side? Is it an iOS application (presumably Swift or something?) sending OSC messages, and your own application receiving them using JavaOSC?

@arlomedia
Copy link
Author

arlomedia commented Jan 22, 2025

Thanks, I've never forked a project or done PRs but I'll try it.

I develop an iOS (Swift) and Android (Kotlin) version of the same app. Instances of the app on different devices can communicate with each other. Each device can be a client or a server, but it's a closed system so I can determine how they interact. I'm using https://github.com/sammysmallman/OSCKit on iOS and am trying to use JavaOSC on Android.

I've already implemented this functionality using a few other approaches, but want to try OSC and see if that will work better. The functionality is described here FWIW: https://www.bandhelper.com/tutorials/live_sharing.html#broadcast_actions

@daveyarwood
Copy link
Collaborator

Looks like a cool project! It seems like the kind of thing OSC was designed for. It sounds like the issue is compatibility between OSCKit and JavaOSC, which could easily be on the JavaOSC side from not handling the header bytes.

Anyway, if you're able to push your code to a branch or make a PR with the proposed changes, I'll be happy to test it out with my Java + Go project and see if it breaks anything, sometime soonish when I have time.

@arlomedia
Copy link
Author

Thanks for your help with this. I hope I set this up correctly:

#83

If you were sending to and from JavaOSC in your project, I expect you wouldn't see the problem because the packet length header is missing from both the sending and receiving functions. But this patch would be needed to exchange data with another OSC library.

@daveyarwood
Copy link
Collaborator

Thanks @arlomedia. I'll give your branch a try soon (hopefully this weekend) and let you know how it goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants