thestk / rtmidi

A set of C++ classes that provide a common API for realtime MIDI input/output across Linux (ALSA & JACK), Macintosh OS X (CoreMIDI) and Windows (Multimedia)
Other
987 stars 274 forks source link

New release wished for #165

Closed alexmyczko closed 5 years ago

alexmyczko commented 6 years ago

I would like to see a new version released, so it can get into Debian early, to eventually/maybe get Fasttracker II packaged properly, without a local unreleased actual copy of rtmidi.

Would that be do-able during the next 3 months?

radarsat1 commented 6 years ago

I think it should be possible. But you've got me curious as an avid FT2 user from 20 years ago, I didn't know FastTracker II was even available for Linux let alone being packaged for Debian. Where can I find out more about this?

alexmyczko commented 6 years ago

https://16-bits.org/ft2.php The being packaged for Debian, is only valid if the license (not out yet) will be compatible.

8bitbubsy commented 6 years ago

The problem is that the latest version of rtmidi (3.0.0) is useless if you use the MIDI callback with the C wrapper. It just doesn't work, because the callback doesn't have a length parameter, and strlen()'ing the bytes is obviously going to give the wrong length in many cases (zeroes in the middle). I have even hinted about considering releasing a new version (it has been fixed already) several months ago. This is a critical bug! So what I have done is to bundle the code from github, instead of letting people compile it with rtmidi from their package repositories. Anyways, @alexmyczko I'd prefer to not see a packaged distribution of my clone, as it's in beta phase.

radarsat1 commented 6 years ago

Cool stuff!

Anyways, on topic: After reviewing I think there have been sufficient changes that a release is in order. Ideally we could fix the long-standing issues with port naming, but it doesn't warrant blocking a release imho. I'd like to integrate the fixes to the automake build system I've proposed, and then decide what to do regarding version numbers.

A quick glance shows that technically apart from some bug fixes, we've only added a couple of new functions so I'm not sure a major version bump is necessary, but on the other hand the visibility of some symbols may have changed. I'll have to take a deeper look.

garyscavone commented 6 years ago

What are the specifics of the previously mentioned c-wrapper callback bug? Can someone provide a fix for this before a new release is made?

On Jul 15, 2018, at 1:14 AM, Stephen Sinclair notifications@github.com wrote:

Cool stuff!

Anyways, on topic: After reviewing I think there have been sufficient changes that a release is in order. Ideally we could fix the long-standing issues with port naming, but it doesn't warrant blocking a release imho. I'd like to integrate the fixes to the automake build system I've proposed, and then decide what to do regarding version numbers.

A quick glance shows that technically apart from some bug fixes, we've only added a couple of new functions so I'm not sure a major version bump is necessary, but on the other hand the visibility of some symbols may have changed. I'll have to take a deeper look.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/thestk/rtmidi/issues/165#issuecomment-405055751, or mute the thread https://github.com/notifications/unsubscribe-auth/AFOBpatQSSqDdMX0AP_OmM0Kf962Lnrnks5uGntwgaJpZM4VMmjQ.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/thestk/rtmidi","title":"thestk/rtmidi","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/thestk/rtmidi"}},"updates":{"snippets":[{"icon":"PERSON","message":"@radarsat1 in #165: Cool stuff!\r\n\r\nAnyways, on topic: After reviewing I think there have been sufficient changes that a release is in order. Ideally we could fix the long-standing issues with port naming, but it doesn't warrant blocking a release imho. I'd like to integrate the fixes to the automake build system I've proposed, and then decide what to do regarding version numbers.\r\n\r\nA quick glance shows that technically apart from some bug fixes, we've only added a couple of new functions so I'm not sure a major version bump is necessary, but on the other hand the visibility of some symbols may have changed. I'll have to take a deeper look."}],"action":{"name":"View Issue","url":"https://github.com/thestk/rtmidi/issues/165#issuecomment-405055751"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/thestk/rtmidi/issues/165#issuecomment-405055751", "url": "https://github.com/thestk/rtmidi/issues/165#issuecomment-405055751", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [thestk/rtmidi] New release wished for (#165)", "sections": [ { "text": "", "activityTitle": "Stephen Sinclair", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@radarsat1", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"thestk/rtmidi\",\n\"issueId\": 165,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close issue", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueClose\",\n\"repositoryFullName\": \"thestk/rtmidi\",\n\"issueId\": 165\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/thestk/rtmidi/issues/165#issuecomment-405055751" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 355625168\n}" } ], "themeColor": "26292E" } ]

8bitbubsy commented 6 years ago

rtmidi_c.h from latest 3.0.0 release (notice the lack of a messageSize parameter): typedef void( RtMidiCCallback) (double timeStamp, const unsigned char message, void *userData);

rtmidi_c.h from latest github non-release code: typedef void( RtMidiCCallback) (double timeStamp, const unsigned char message, size_t messageSize, void *userData);

And yes, this really is a big problem. As I said, you can't just strlen() message because it can (and will) contain zeroes before the actual end.

radarsat1 commented 6 years ago

Yes, so just to be clear, it has been fixed, @8bitbubsy just wants the fix to be in a tagged release.

Fix was in 431354f421855739d57ec07cd132d8ef14484e4e

I will have a hard look in the next days at the C API to see if there are any bugs to be fixed or improvements that can be made.

I'll also give a harder look to the previous efforts to fix the port naming scheme -- I think the reason we haven't fixed/accepted things on this front is mainly the difficulty of verifying a fix. We want something that works reliably across APIs and operating systems, but this means testing with many different computers and MIDI devices, which is a hard effort to organize. I think the best approach would be to come up with something backwards-compatible that can be improved/fixed incrementally, but it is not easy.

garyscavone commented 6 years ago

The port-naming issue is indeed a long-standing problem, with no easy cross-platform solution. There have been previous proposals (from 4 years ago or more) but they often involved a lot of extra “baggage.” From what I remember, the problem is primarily related to “plug and play” devices (reordering) and drivers that don’t necessarily distinguish multiple devices of the same type / model.

On Jul 15, 2018, at 11:42 PM, Stephen Sinclair notifications@github.com wrote:

Yes, so just to be clear, it has been fixed, @8bitbubsy https://github.com/8bitbubsy just wants the fix to be in a tagged release.

Fix was in 431354f https://github.com/thestk/rtmidi/commit/431354f421855739d57ec07cd132d8ef14484e4e I will have a hard look in the next days at the C API to see if there are any bugs to be fixed or improvements that can be made.

I'll also give a harder look to the previous efforts to fix the port naming scheme -- I think the reason we haven't fixed/accepted things on this front is mainly the difficulty of verifying a fix. We want something that works reliably across APIs and operating systems, but this means testing with many different computers and MIDI devices, which is a hard effort to organize. I think the best approach would be to come up with something backwards-compatible that can be improved/fixed incrementally, but it is not easy.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/thestk/rtmidi/issues/165#issuecomment-405120563, or mute the thread https://github.com/notifications/unsubscribe-auth/AFOBpcMRooU4-7LaxsTtF8bQ040Qz3nlks5uG7c0gaJpZM4VMmjQ.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/thestk/rtmidi","title":"thestk/rtmidi","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/thestk/rtmidi"}},"updates":{"snippets":[{"icon":"PERSON","message":"@radarsat1 in #165: Yes, so just to be clear, it has been fixed, @8bitbubsy just wants the fix to be in a tagged release.\r\n\r\nFix was in 431354f421855739d57ec07cd132d8ef14484e4e\r\n\r\nI will have a hard look in the next days at the C API to see if there are any bugs to be fixed or improvements that can be made.\r\n\r\nI'll also give a harder look to the previous efforts to fix the port naming scheme -- I think the reason we haven't fixed/accepted things on this front is mainly the difficulty of verifying a fix. We want something that works reliably across APIs and operating systems, but this means testing with many different computers and MIDI devices, which is a hard effort to organize. I think the best approach would be to come up with something backwards-compatible that can be improved/fixed incrementally, but it is not easy."}],"action":{"name":"View Issue","url":"https://github.com/thestk/rtmidi/issues/165#issuecomment-405120563"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/thestk/rtmidi/issues/165#issuecomment-405120563", "url": "https://github.com/thestk/rtmidi/issues/165#issuecomment-405120563", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [thestk/rtmidi] New release wished for (#165)", "sections": [ { "text": "", "activityTitle": "Stephen Sinclair", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@radarsat1", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"thestk/rtmidi\",\n\"issueId\": 165,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close issue", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueClose\",\n\"repositoryFullName\": \"thestk/rtmidi\",\n\"issueId\": 165\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/thestk/rtmidi/issues/165#issuecomment-405120563" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 355625168\n}" } ], "themeColor": "26292E" } ]

radarsat1 commented 6 years ago

Agreed on the proposed solutions being too heavy. I've been reviewing the port-naming issue and it's still not entirely clear to me, there seem to be multiple issues/complaints that are intertwined and I'd like to sort them out a bit more clearly. I'll summarize my findings and maybe try to come up with a proposal. In the meantime I'd like to just merge the fixes that allow building against unrecognized OSs (PR #166) and I've just pushed a couple of small improvements to the C API.. I can't see more to do on that for now. (One API change for rtmidi_get_compiled_api, hope it doesn't cause problems with existing software but I think it's safer to pass in the array size.)

sagamusix commented 6 years ago

Slightly off-topic: Even without 431354f it is possible to use the old C API though; it can be determined easily from the high nibble of the first message byte how long the message is (apart from SysEx, which the FT2 clone probably doesn't handle anyway). strlen is quite obviously the wrong solution because we are not dealing with strings here but with MIDI messages.

alexmyczko commented 5 years ago

6 months passed, any update?

radarsat1 commented 5 years ago

Release 4.0.0 has been issued.