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

Jbflow/feature/tcp socket #119

Open
wants to merge 19 commits into
base: websockets
Choose a base branch
from

Conversation

jbflow
Copy link

@jbflow jbflow commented May 21, 2024

I think I have removed all breaking changes so this should work as before. I have set it back to using the UDP socket with an optional variable you can change in the init file to have it run the TCP socket, and also implemented a socket interface and a base class for the main script so we can test and compare the two sockets and this should help to optimise the code without breaking anything else.

Confirmation that this method does work from postman below.

Screenshot 2024-05-21 at 18 30 36

What:

  • Created a socket interface for handling both UDP and TCP sockets
  • Created a AbletonJSBase class to handle both socket versions
  • Set a socket_type flag in init.py to determine which AbletonJS script to load
  • Created a Threaded TCP socket with framing and handshake handling
  • Set up message propagation up from TCP socket thread to main thread and placing in message queue.Queue via a callback
  • Used Live.Base.Timer to periodically check the queue for messages and sends them to the handler (it was this step that I was missing in my previous attmempt that caused it to hang so thanks to this project for making me aware that we could do this.)

To test TCP socket

  • Update socket_type to "TCP" in midi-script/init.py
  • copy midi-script to AbletonJS in Remote Scripts folder
  • Start Live
  • Connect to the socket from postman, wscat or some other testing service
  • Send a payload
  • Binary data should be sent back as shown in the above screenshot

To do:

  • handle port assignment as I am currently just hardcoding the ports - the tempfile method won't work with the browser (I don't think?)
  • handle decompressing incoming messages, messages out are compressed using zlib but incoming ones aren't decompressed, this is an oversight on my part and mainly because in my application messages out of live are quite large but messages in are smaller.
  • Update readme
  • Add React App example code
  • Integrate TCP socket with TypeScript code (or maybe it's easier to just have it separate for browser?)

Copy link

what-the-diff bot commented May 21, 2024

PR Summary

  • Websocket Connections Established for React
    A new file hooks/useWebsocket.js was added. This file contains a custom React hook that simplifies connecting to a WebSocket and handling incoming messages.

  • Enhancements in MIDI Script Handling
    The base MIDI script (AbletonJS.py) was renamed to AbletonJSBase.py and its respective class and method names were updated. Two new subclasses of AbletonJSBase were introduced: AbletonJSTCP.py and AbletonJSUDP.py. They are responsible for managing TCP and UDP socket connections, respectively.

  • Refined Method Portfolio within Internal.py
    In the midi-script/Internal.py file, a new method set_client_port was inserted to facilitate setting up client port usage.

  • Socket Communication Improved
    New files SocketInterface.py, TCPSocket.py and UDPSocket.py were established. SocketInterface.py offers a framework for socket subclasses, whereas TCPSocket.py and UDPSocket.py cater to TCP and UDP socket connections. The previous Socket.py file was renamed to UDPSocket.py to offer a clearer name distinction.

  • Modification of Initialization Script for Socket Usage
    The script midi-script/__init__.py was adjusted, enabling dynamic TCP and UDP socket class deployment depending on the user-specific socket_type flag.

  • Package.json Updated with New Dependencies
    The package.json file was modified to include two new dependencies: pako and react, to support new languages and frameworks.

  • Streamlined File Transfers and Live Log Checking
    Two new scripts scripts/copy_remote_script.sh and scripts/read_live_logs.py were integrated. The former assists with efficiently copying files from a specific source directory to a targeted destination directory. The latter aids in the reading of Live logs from a specified version of Live provided as a command line argument.

@leolabs
Copy link
Owner

leolabs commented May 28, 2024

That's awesome, thank you so much for this PR and the work you put into making this work with WebSockets! Let's check if it would be possible to make this into a package that can be used on NodeJS, Deno, and in the browser.

There's a build tool called DNT that allows us to write TypeScript focused on the browser and that takes care of creating versions that can be used on NodeJS by, for example, shimming the WebSocket implementation automatically.

For the client side, something like PartySocket might be interesting to look into as it nicely automatically handles reconnects when the connection drops for some reason.

If you're happy with the Python script's side of the PR, we could merge it into a separate branch and I can take care of the TypeScript side. What do you think?

I'm also curious about the latency of WebSockets running in a separate thread compared to the current UDP approach of polling for data every few milliseconds. Have you done any tests to compare the two in that regard?

@jbflow
Copy link
Author

jbflow commented May 28, 2024

No problem, it's great to find other devs working with the Live API. Thanks for sharing theses resources. 🎆

Not heard of Deno or DNT before so will have a read.

👀 PartySocket - as I am currently handling that functionality manually.

I have a few tweaks to make to the Python script that I have made in my own project so I will reflect those here, so yeah merging to a new branch sounds like a good plan, will push my changes over the next 24 hours for you to review.

Haven't looked at testing yet but it's something that needs doing. when I update this PR I will roll back some of the breaking changes so we can look at running both implementations side by side to compare.

@jbflow jbflow marked this pull request as ready for review May 29, 2024 01:41
@jbflow
Copy link
Author

jbflow commented May 29, 2024

@leolabs PR ready for review, breaking changes and clutter removed and working from node as before or you can switch it to TCP to connect from a websocket. :)

@leolabs leolabs changed the base branch from master to websockets May 29, 2024 11:14
@leolabs
Copy link
Owner

leolabs commented May 29, 2024

Thank you, I'll have a look at this now! I think once we get the WebSockets implementation working properly on the JS side, we can safely remove the UDP implementation and just release this as a major version with potential breaking changes to keep the code simple.

Regarding the port situation, I think you asked about it in one of the other threads, we could use an approach where the Python plugin chooses a free port out of a list of, say, 8 predefined ones (e.g. 39031, 39032, 39033, 39034, 39035, 39036, 39037, 39038). This should avoid conflicts where the OS is already using a port, and the client library can just try to connect to each port and if that isn't available, use the next one.

Usually, the first port in the list is free and can be used so this approach shouldn't have a huge performance impact for establishing a connection. What do you think?

@jbflow
Copy link
Author

jbflow commented May 29, 2024

Awesome, the approach I've taken should allow for both to run side by side until we're confident in this approach, the limited testing I did last night it doesn't seem to have broken anything for running in Node.

With regards to ports, that was the approach I was thinking of taking, id agree that it's unlikely we're going to encounter another bound port, the most likely scenario is if live crashes or our own socket fails to close and unbind properly and then it fails to reconnect after starting because it's still open and bound to the previous instance, in which case we can just move to one of the next predefined ports.

Another note on this PR which I forgot leave in my comments, I started looking at chunking for large message sizes but had to remove it because there was an issue with framing, I'm handling this in my code by batching the messages, combined with the queue it seems to work ok, might also be possible to limit the queue size and have it send once it hits a certain size to keep the package size below the limit. What are your thoughts on this?

@jbflow
Copy link
Author

jbflow commented May 29, 2024

I removed the chunking approach in commit dee9906

@jbflow
Copy link
Author

jbflow commented Jun 2, 2024

Hey @leolabs

Just wondering if you've had a chance to look at this yet? I only ask as I'm now making some further additional changes to my other projects Python scripts in the form of an automatic update mechanism handled in the disconnect method, so that updates to the script can be pushed and the script can fetch a raw zip file from GitHub and replace itself when live shuts down. I haven't tested it out yet but that is my next mission, there are security implications to address but I think I have them figured out. I would be interested in contributing that functionality to this project as well if that is of interest to you, but I don't want make too many pull requests and start getting tripped up with the code. I hope this makes sense, apologies for the digression away from the original topic of this PR 🪀 💡 🤯 💥

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.

None yet

2 participants