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.

Sign up for GitHub

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

Jump to bottom

Issue #88 - Fix error: Message too long #89

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

Issue #88 - Fix error: Message too long #89

wants to merge 3 commits into from

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
@StrongBearCeo
Add chunk index, total chunks, message id and delimiter to each chunk…
2d900d9 
… so that client can assemble the chunks with more confidence
@StrongBearCeo StrongBearCeo mentioned this pull request Jun 23, 2023
Closed
ideoforms
ideoforms requested changes Jul 25, 2023
View reviewed changes
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ideoforms ideoforms ideoforms requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

/live/clip/get/notes Error "Message too long"
3 participants
@StrongBearCeo @ideoforms @esaruoho