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

Minor changes and additions #29

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Minor changes and additions #29

wants to merge 17 commits into from

Conversation

Brettles
Copy link

Some things that I found handy while using the code: Added log output for client latency (other RTP MIDI clients do this so I figured this was a good thing); a little bit of extra decoding for pitch blend commands; and passed the journal through to the handler because it was good to have for what I needed.

Great work on the code...

Copy link
Owner

@mik3y mik3y left a comment

Choose a reason for hiding this comment

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

@Brettles cool! thanks for the patch! Couple small requests but this will be good to merge.

examples/example_server.py Outdated Show resolved Hide resolved
pymidi/client.py Outdated Show resolved Hide resolved
@mik3y
Copy link
Owner

mik3y commented Aug 26, 2023

PS: I just added some github actions to the repo. When you push this branch again you should see them run.

It's OK if the lint check fails - I will fix that subsequently (it's going to be a big diff, best not to mix it with your functional changes).

@Brettles
Copy link
Author

No problems. Thanks for your patience - mostly a git and Python n00b. New PR inbound because I'm not sure if (how!) to update this one.

@Brettles
Copy link
Author

...or it might have already done it. Sorry. Unsure. Looks like the commits are in there. Let me know if that isn't the case.

@Brettles
Copy link
Author

Again apologies for my n00b-ness (because I think my next change got rolled into this PR).

Made a change to the client code - I'm still learning this RTP MIDI stuff but in all the other examples I've seen the client uses two source ports (one for control and one for MIDI) and they are generally allocated sequentially. This is now reflected in the client module by having two sockets rather than one. So far it seems to be working better with the other components I'm using; but more testing to come.

@mik3y
Copy link
Owner

mik3y commented Aug 29, 2023

Hey @Brettles, there's no need to make any apology for noobness! I think open source should be fun first and foremost; and good bit of that fun comes from learning as we go. I'm just happy you're diving in!

Just a quick heads up, I'm just headed out on a short vacation so I'm probably not going to be able to merge this until next week. So feel free to revise/tinker if you like until then.

Poke me again a week from now if I haven't jumped back in by the , thanks!

@Brettles
Copy link
Author

No problems! Have made a bunch of other small changes. Will submit them... No rush. Enjoy your vacation!

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.

2 participants