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

Add a method to reset the ok flag and clear the error message #322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stfufane
Copy link

Hi,

I don't know if you might be interested or not by this pull request, but I'm making a wrapper around the library with Deno, something similar to what was made here for Node JS : https://github.com/justinlatimer/node-midi

And while trying to handle errors, I noticed two things :

  • The msg member of the device struct was set from a reference to an exception, and was then incorrect when the exception got out of scope (dangling pointer)
  • Once an error was met, the ok flag was set to false forever

It's kinda specific to that type of usage though, because I call the library with FFI bindings and then read the methods' results, so it's probably not required for a "classic" usage linking to a native C/C++ application.

Anyway, I'm open to your opinion on that :)

Cheers

@jbarrett
Copy link
Contributor

jbarrett commented Jan 25, 2025

This would be great for my own use case (to the point that I've included this patch in my own bindings). I have a couple of questions:

If I'm not mistaken, the API consumer is now responsible for free()ing and resetting msg after an error, is this correct?

Might it be a good idea to have an errno struct member set to the appropriate RTMIDI_ERROR constant as well? I think retaining the msg pointer is good for those needing ABI stability (i.e. me), but knowing the nature of the error without having to inspect the string could be useful.

Cheers!

@stfufane
Copy link
Author

Hey, sorry for the late reply. Didn't work much on it recenlty so I don't really have the context anymore, I'd need to check. But tbh this repository seems kinda dead and outdated, if I had to rewrite web bindings for midi, I'd probably use this library instead : https://github.com/celtera/libremidi

It uses modern C++ and offers C bindings, it includes midi 2.0 spec, and is actively maintained.

Good luck :)

@jbarrett
Copy link
Contributor

jbarrett commented Feb 1, 2025

I wouldn't call this project dead -- MIDI 1 is not exactly a moving target so I don't think a release is required that frequently :)

Cheers for the heads up on libremidi - I had ignored that in the past due to the lack of a C interface. Thanks!

@stfufane
Copy link
Author

stfufane commented Feb 1, 2025

Yeah of course it works fine and has not many reasons to evolve, but what I meant is that this PR, for example, got absolutely no answer or comment, not even to say it's bad or anything, so I don't know what should be expected of it...

I'm trying to see if I can generate an FFI binding to libremidi for my Deno midi library, there's a bit of effort but it should be doable.

@garyscavone
Copy link
Contributor

Given the potential memory leak issue noted, as well as the fact that I don't use the C functionality myself, I will wait to see if others have opinions on 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.

3 participants