Skip to content

Conversation

@StrongBearCeo
Copy link
Contributor

Fix #88 , message too long will be split into smaller chunks

@StrongBearCeo StrongBearCeo changed the title Issue #88 Issue #88 - Fix error: Message too long Jun 10, 2023
… so that client can assemble the chunks with more confidence
Copy link
Owner

@ideoforms ideoforms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like great work!

It's a significant and complex new piece of functionality, so would you also be able to add a unit test to verify that this is working as expected? Something like /live/clip/add/notes / /live/clip/get/notes, creating a large number of notes (exceeding the UDP size limit) and verifying that they are added successfully. I would then expect the unit test to fail before your fix, and pass after it. This could go in a new test file tests/test_api.py.

Thanks again, this will be a great fix for operating on complex clips.

@ideoforms
Copy link
Owner

I've had a more detailed look at this, and have also added a unit test that can be used to test bidiretional long messages. It populates a clip with a large number of notes (~384), and then queries the notes to verify that they are all intact. It is easy to extend this to test a significantly larger number of notes by increasing the number of time shifts applied.

It has shed light on a couple of limitations with this PR that need addressing:

  1. The unit test currently passes with master but fails with the implementations of osc_server/client in this PR, so I think the packet reassembly is failing somewhere.
  2. The packet re-assembly works great on client.query(), but also needs adding to the await_message handler. Actually, both of these would benefit from a minor refactor to share the same receive code...
  3. It also needs implementing in AbletonOSCClient's send_message method and osc_handler', so that outbound client → server messages (e.g. /live/clip/add/notes`)

Potentially (2) and (3) could become a separate Issue, but (1) should be fixed before this can be merged.

@StrongBearCeo I realise it has been a while since this original PR - so if you're not still working on this, no worries, at some point I (or somebody else) will be able to get this across the line.

@StrongBearCeo
Copy link
Contributor Author

@ideoforms that's great. I'll look into it when I have time, too.

@esaruoho
Copy link
Contributor

@StrongBearCeo have you had the time to look at it? :)

@StrongBearCeo
Copy link
Contributor Author

@StrongBearCeo have you had the time to look at it? :)

@esaruoho I haven't. I've been using my own fork for this, actually

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

/live/clip/get/notes Error "Message too long"

4 participants