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 support (take 2) #42

Open
wants to merge 50 commits into
base: master
Choose a base branch
from

Conversation

daveyarwood
Copy link
Contributor

This is almost identical to #37, but I've built it atop a handful of smaller PRs: #39, #40, #41

Review and merge those other PRs first and the diff of this PR will become a lot smaller and just be about adding TCP support.

See #37 for discussion related to TCP support.

…e server.ListenAndServe and server.CloseConnection
When Timetag arguments are read from incoming messages, they are parsed as
*Timetag, so these references to Timetag needed to be adjusted accordingly.
@daveyarwood daveyarwood mentioned this pull request Mar 9, 2020
@njolstech
Copy link

Is it possible that this PR could get merged?

@hypebeast
Copy link
Owner

Hey,

First, sorry for my late reply.

Yes, overall I think it's possible that this PR gets merged. I'll try to have a look at it during the weekend and see if it's possible to merge it.

@daveyarwood
Copy link
Contributor Author

@hypebeast Much appreciated!

As I mentioned in the description of this PR, I've broken the changes up into several smaller PRs. You'll have a much easier time if you review #39, #40 and #41 first. Once those PRs are merged, the diff for this PR will get a lot smaller and it'll just be about adding TCP support.

On my subsequent branch that adds TCP, I added a second argument that is
the protocol to use. That's why these scripts currently expect 2
arguments. However, on _this_ branch, only the first argument is used,
and that first argument is the port. I ended up b0rking this when I
extracted this branch from the rest to make PR review easier.

This commit fixes it so that the scripts expect the correct number of
arguments, which is 1 argument, the port.
Now, these examples expect 2 arguments: the protocol and the port.
@hypebeast
Copy link
Owner

@daveyarwood Thanks a lot for this. I'll start with the PRs you mentioned.

@daveyarwood
Copy link
Contributor Author

I've resolved merge conflicts and made sure tests are still passing.

@daveyarwood
Copy link
Contributor Author

It's been almost two years now since I made this PR. Is there anything I can do to speed along reviewing and merging it? I put a lot of work into this and I have to be honest, it's disappointing that this process is so slow.

@hypebeast
Copy link
Owner

Hey @daveyarwood ,

First, thanks a lot for your effort and sorry for my late replay. I plan is to be more responsive in the future and to spend more time on this project.

I already had a look at this PR and currently I'm trying to figure out what is the best approach to continue and how to merge it with #48 .

@daveyarwood
Copy link
Contributor Author

daveyarwood commented Apr 26, 2022

I see that this branch has conflicts with the master branch again. I can work on resolving those soon if that would be helpful.

Would it be helpful if I rebased this branch on top of #48 ?

EDIT: Probably as a separate branch, so that we don't lose the current state of this branch which is based atop master.

@Solander
Copy link

Solander commented Nov 6, 2022

Is there any updates on this PR? Would love to have TCP support in the library.

@Solander
Copy link

Solander commented Nov 6, 2022

At closer inspection, I see that this solution only sends the OSC message over a TCP connection and then closing it. Why not keep the connection open?
I'm trying to interface with a program called QLab and it uses OSC over TCP and it also sends data back to the client. Any thoughts on implementing a way to recieve messages as well as sending them over TCP?

@daveyarwood
Copy link
Contributor Author

Re: keeping the connection open, I think that could be a good optimization, but I'm hesitant to do any more work on this branch until the maintainers merge it. It's been almost 3 years now, so I'm not hopeful that it will happen anytime soon, unfortunately.

Anecdotally, I use my fork of go-osc in production for Alda using TCP, and it works great. I'm sure that the communication between the client and server could be made even faster if we reused the same connection.

Thinking out loud: it would probably introduce a little bit of extra complexity to reuse the same connection, wouldn't it? Can TCP connections go "stale" and start misbehaving? If so, is there a way that we could detect that and recover by automatically making a new connection? Maybe this functionality could be added in a backwards compatible way by making it opt-in, but still making the current behavior (one connection per send) the default behavior.

Re: receiving messages in response, I haven't thought about that at all, because I'm so used to thinking about OSC being a one-way protocol. I'd be interested to learn more about that.

@Solander
Copy link

Solander commented Nov 8, 2022

I understand what you mean. Hopefully hypebeast accepts this fork so it can be improved even further. :)
Alda seems like a cool project! I might try it when I got the time!

I'm not sure if it's part of the OSC protocol or if it's something QLab just is using. Quite handy and I hope more programs begins to use it. QLab is a program where you can setup actions (single or grouped) that will be executed each time you press GO.
The bi-directional OSC gives QLab the ability to send it's list of cues to the OSC user so it's easier to know the ID of the certain cue he or she want's to trigger.
As a lighting technician I would love to have that information from my lighting desk as well so I really hope it gets more popular!

@Solander
Copy link

Solander commented Nov 8, 2022

I saw that I missed a question.
On the subject if TCP can go stale and needs reconnecting, it feels like something that is taken care of in the standard library. Not sure though so that needs to be researched.
Opt-in maybe is a good idea from the beginning.

@avlapp
Copy link

avlapp commented Nov 28, 2022

'm so used to thinking about OSC being a one-way protocol. I'd be interested to learn more about that.
That's a common misconception indeed, see also https://opensoundcontrol.stanford.edu/files/osc-best-practices-final.pdf

I'm interested in TCP as well, all though I'm currently using and happy with: https://github.com/scgolang/osc
I've chosen that library, cause it allows for replies etc. Worth to take a look at. Not actively maintained, but the author is willing to merge patches.

@avlapp
Copy link

avlapp commented Nov 28, 2022

There are two specs for TCP? 1.0 and 1.1[1]:
https://forum.renoise.com/t/osc-via-tcp-has-no-framing/42459/3 [2]
https://www.spinics.net/lists/linux-audio-users/msg113397.html (this opinion contradicts with the post above about puredata it seems.)

The c/c++ library Liblo supports both they say: https://github.com/radarsat1/liblo

[1] which was never officially published? https://opensoundcontrol.stanford.edu/spec-1_1.html

@hypebeast
Copy link
Owner

Hey. Thanks a lot for your involvement and your contribution. I just want to make it clear that I will definitely open to merge this PR to provide TCP support. What we need to align on is what would be the best way to move forward. I will have a look at #48 to have a better understanding what would be the option to move forward. But actually I tend to first merge this PR and then to continue with #48.

What do you think? What are the open tasks we need to do to be able to merge this PR?

@daveyarwood
Copy link
Contributor Author

As far as I'm aware (insert big asterisk here), my implementation Should Work™ if the merge conflicts are resolved and this PR is merged.

Big asterisk: I haven't been keeping up to date with any updates/changes that have been happening in go-osc over the last 2-1/2 years since I made this PR, so I'm not really sure off-hand how easy the conflicts would be to resolve, or if something may have changed in a way that would cause the TCP implementation not to work.

I'd be happy to take a look at this soon if I have time and see if I can resolve the merge conflicts.

@daveyarwood
Copy link
Contributor Author

@hypebeast I've just updated my branch with the latest commit from master, and all tests are passing for me locally. It looks like the CI tests are passing as well... it does say something about 1 failing check, but I don't see any details about that failing check.

I believe this PR is ready to merge. Let me know your thoughts, or if you need anything else!

@chabad360
Copy link
Contributor

As far as I'm aware (insert big asterisk here), my implementation Should Work™ if the merge conflicts are resolved and this PR is merged.

The big asterisk is that a lot of things change (including some of the server semantics, I experimented with bringing things inline with how net/http looks, and I kinda liked it), especially once I get around to backporting the changes from my master. It might be a good idea to introduce a PR that splits osc.go into a bunch of smaller files (for reference you can see how I've done it), that way we kill two birds with one stone:

  1. It becomes much easier to determine what all the changes are and how they interact with other portions of the code.
  2. It avoids a host of merge conflicts that result just from having everything being in one file.

If that happens it will be a lot easier for me to introduce my changes in a more piece-meal format (which -considering the scope- is probably a good idea) and should make it possible for both PRs to go through without nearly as much effort (although it will require re-submitting this PR).

@daveyarwood
Copy link
Contributor Author

I would really prefer if my changes could be merged as-is! 🙏 I made this PR almost 3 years ago, and I have very limited time to invest in refactoring it and resolving merge conflicts at this point. At the moment, I have this branch all caught up with master and the tests are all passing. Can we please merge it before it goes out of sync again?

@chabad360
Copy link
Contributor

Oh. Then forget it, I vote that this should get merged first. I'll deal with the conflicts and the changes necessary to accommodate it in my patches.

@mhite
Copy link

mhite commented Jul 8, 2023

I gave the @daveyarwood branch a whirl and quickly discovered it is not intended for two-way TCP communication. I do sincerely appreciate the effort on adding the TCP support. I'm definitely still searching for an implementation that provides bi-directional TCP support for OSC.

@avlapp - https://github.com/scgolang/osc looked promising, and bi-directional communication seems to be part of the original design. Too bad no TCP support there, though.

Interestingly enough, in real "in-the-wild" bi-directional TCP OSC that I've seen thus far, there is no adherence to the int32 sizing from the spec:

The underlying network that delivers an OSC packet is responsible for delivering both the contents and the size to the OSC application. An OSC packet can be naturally represented by a datagram by a network protocol such as UDP. In a stream-based protocol such as TCP, the stream should begin with an int32 giving the size of the first packet, followed by the contents of the first packet, followed by the size of the second packet, etc.

https://ccrma.stanford.edu/groups/osc/spec-1_0.html

Curious what others have seen.

@vlappa
Copy link

vlappa commented Jul 8, 2023

@avlapp - https://github.com/scgolang/osc looked promising, and bi-directional communication seems to be part of the original design. Too bad no TCP support there, though.

That library is serving me well atm, no tcp indeed and a few issues

Interestingly enough, in real "in-the-wild" bi-directional TCP OSC that I've seen thus far, there is no adherence to the int32 sizing from the spec:

The underlying network that delivers an OSC packet is responsible for delivering both the contents and the size to the OSC application. An OSC packet can be naturally represented by a datagram by a network protocol such as UDP. In a stream-based protocol such as TCP, the stream should begin with an int32 giving the size of the first packet, followed by the contents of the first packet, followed by the size of the second packet, etc.

https://ccrma.stanford.edu/groups/osc/spec-1_0.html

Curious what others have seen.

I think pure data supports both OSC 1.0 and 1.1, Udp and Tcp. (Pure data is written in C)
https://github.com/pure-data/pure-data

One could take a look at liblo as well: https://liblo.sourceforge.net/

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

Successfully merging this pull request may close these issues.

9 participants