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

Sysex-related fixes #3

Closed
wants to merge 1 commit into from
Closed

Sysex-related fixes #3

wants to merge 1 commit into from

Conversation

ptarabbia
Copy link

Hi,

Here is the pull request for the sysex changes I did.

…to be completed before sending a new one.

On Windows: fix lockups when shutting down while sending/receiving sysex
@SpotlightKid
Copy link
Contributor

Shouldn't this be separated into two pull requests (OS X and Windows)?

@ptarabbia
Copy link
Author

Yes. My bad. I got everything committed together.

On Wed, Jan 15, 2014 at 8:39 AM, Christopher Arndt <notifications@github.com

wrote:

Shouldn't this be separated into two pull requests (OS X an Windows)?


Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-32361718
.

@garyscavone
Copy link
Contributor

Sorry about the delay on this one. At first, I was a bit skeptical of the OS-X hack but after some more thought, it's a better solution than what we had. That said, I'm incorporating your changes into a local prerelease branch and thus, I'm closing this pull request.

@ptarabbia
Copy link
Author

No problem. I agree it is a really ugly hack, forced by the clumsy way
CoreMidi manages the buffers lifetime....

Patrice

On Fri, Mar 14, 2014 at 1:06 PM, garyscavone notifications@github.comwrote:

Closed #3 #3.

Reply to this email directly or view it on GitHubhttps://github.com//pull/3
.

@ptarabbia
Copy link
Author

Hi,

There are a couple more things that I think, could be improved in the API -
things like const-correctness, and using references when relevant. For
example:
virtual RtMidi::Api getCurrentApi( void ) = 0;
virtual void openPort( unsigned int portNumber, const std::string
portName ) = 0;

could be:
virtual RtMidi::Api getCurrentApi( void ) const = 0;
virtual void openPort( unsigned int portNumber, const std::string&
portName ) = 0;

I can definitely do the changes and submit a pull request if you think this
is worth doing.

Thanks,

Patrice

On Fri, Mar 14, 2014 at 1:10 PM, Patrice Tarabbia <
patrice.tarabbia@gmail.com> wrote:

No problem. I agree it is a really ugly hack, forced by the clumsy way
CoreMidi manages the buffers lifetime....

Patrice

On Fri, Mar 14, 2014 at 1:06 PM, garyscavone notifications@github.comwrote:

Closed #3 #3.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3
.

@garyscavone
Copy link
Contributor

Hi Patrice,

Sure, you can submit a pull request with that.

—gary

On Apr 21, 2014, at 7:22 PM, ptarabbia notifications@github.com wrote:

Hi,

There are a couple more things that I think, could be improved in the API -
things like const-correctness, and using references when relevant. For
example:
virtual RtMidi::Api getCurrentApi( void ) = 0;
virtual void openPort( unsigned int portNumber, const std::string
portName ) = 0;

could be:
virtual RtMidi::Api getCurrentApi( void ) const = 0;
virtual void openPort( unsigned int portNumber, const std::string&
portName ) = 0;

I can definitely do the changes and submit a pull request if you think this
is worth doing.

Thanks,

Patrice

On Fri, Mar 14, 2014 at 1:10 PM, Patrice Tarabbia <
patrice.tarabbia@gmail.com> wrote:

No problem. I agree it is a really ugly hack, forced by the clumsy way
CoreMidi manages the buffers lifetime....

Patrice

On Fri, Mar 14, 2014 at 1:06 PM, garyscavone notifications@github.comwrote:

Closed #3 #3.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3
.


Reply to this email directly or view it on GitHub.

@ptarabbia
Copy link
Author

ok. There is another change I would like to do, but it might be quite a
major refactoring - basically I'd like to be able to plug in new
implementations without having to modify the main RtMidi.cpp source. I need
that in order to support custom midi drivers, for a project I'm working on
for M-Audio.

Patrice

On Sat, Apr 26, 2014 at 6:51 PM, garyscavone notifications@github.comwrote:

Hi Patrice,

Sure, you can submit a pull request with that.

—gary

On Apr 21, 2014, at 7:22 PM, ptarabbia notifications@github.com wrote:

Hi,

There are a couple more things that I think, could be improved in the
API -
things like const-correctness, and using references when relevant. For
example:
virtual RtMidi::Api getCurrentApi( void ) = 0;
virtual void openPort( unsigned int portNumber, const std::string
portName ) = 0;

could be:
virtual RtMidi::Api getCurrentApi( void ) const = 0;
virtual void openPort( unsigned int portNumber, const std::string&
portName ) = 0;

I can definitely do the changes and submit a pull request if you think
this
is worth doing.

Thanks,

Patrice

On Fri, Mar 14, 2014 at 1:10 PM, Patrice Tarabbia <
patrice.tarabbia@gmail.com> wrote:

No problem. I agree it is a really ugly hack, forced by the clumsy way
CoreMidi manages the buffers lifetime....

Patrice

On Fri, Mar 14, 2014 at 1:06 PM, garyscavone notifications@github.comwrote:

Closed #3 #3.


Reply to this email directly or view it on GitHub<
https://github.com/thestk/rtmidi/pull/3>
.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-41483372
.

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.

3 participants