Side note: It also appears, from SC_CoreMIDI.cpp, that the dataless real-time messages could be simplified by eliminating the data argument.

CoreMIDI:

    case 6: // tunerequest
    case 8: // clock
    case 9: // tick
    case 10: // start
    case 11: // continue
    case 12: // stop
    case 14: // activeSense
    case 15: // reset
        gRunningStatus = 0; // clear running status
        runInterpreter(g, s_midiSysrtAction, 3);
        return 1;

PortMIDI:

        case 0xF6:
        case 0xF8:
        case 0xF9:
        case 0xFA:
        case 0xFB:
        case 0xFC:
        case 0xFE:
            gRunningStatus = 0; // clear running status
            ++g->sp;
            SetInt(g->sp, p[i] & 0xF);
            ++g->sp;  // could delete this line
            SetInt(g->sp, 0);  // could delete this line
            runInterpreter(g, s_midiSysrtAction, 4);  // change to '3' here
            i++;
            break;

But perhaps not in this PR.

I'd like to ask what is the path forward on this PR.

I believe it will fix a crashing bug. I assumed that because a crash is involved, it would be treated with higher priority... but the "crash" label was removed so there is now no way to tell from the PR list that this is about a crash... so it's been lost in the shuffle.

Should I push the branch directly to this repository for it to be built automatically? I can reproduce the crash but I don't have a Windows build environment.

but the "crash" label was removed so there is now no way to tell from the PR list that this is about a crash... so it's been lost in the shuffle.

@jamshark70 are you asking why i removed the label? it's because we typically don't use "bug" or "crash" for PRs, only issues. i regularly make changes like this to labels on PRs and issues as part of triageing. we can definitely change this practice, if you'd like.

I'd like to ask what is the path forward on this PR.

someone will review it when they have time, like any other PR.

it's because we typically don't use "bug" or "crash" for PRs, only issues

I thought it might be something like that.

"Bug," I suppose, is not really necessary for a PR. As for the crash label, my lens is tinted by my time in a software company, where all crashes automatically received top priority to be fixed in the next release. So I was surprised that this issue dropped off the radar. (Admittedly, I didn't follow it carefully either, so I take my share of the responsibility for that.) Labeling crashes might help bring them to the top.

True, this issue affects seldom used MIDI messages on one OS only -- but it was reported by a user, who is unable to proceed with syncing SC to hardware. There's no workaround.

By "path forward," I specifically meant generating a test build. The code looks correct but shouldn't be merged until I fire the offending messages at it and it responds correctly.

As for the crash label, my lens is tinted by my time in a software company, where all crashes automatically received top priority to be fixed in the next release. So I was surprised that this issue dropped off the radar. (Admittedly, I didn't follow it carefully either, so I take my share of the responsibility for that.) Labeling crashes might help bring them to the top.

prioritizing based only on crash/noncrash is not so productive, because many crashes happen under uncommon circumstances and shouldn't receive attention just for that. if your gripe is that we should be doing a better job of prioritizing issues and PRs, i agree, and we've been discussing this on the dev calls. i'll open a ticket after the 3.11.2 release to explain more.

By "path forward," I specifically meant generating a test build. The code looks correct but shouldn't be merged until I fire the offending messages at it and it responds correctly.

oh, that's much clearer thanks. the wiki has instructions on how to do that: https://github.com/supercollider/supercollider/wiki/Continuous-Integration---Travis-&-Appveyor#storage-and-deployment-of-build-artifacts

"Gripe" is a strong word, and not my intent at all.

I do think this PR in particular is an ideal candidate for a patch release: small scope, doesn't break anything that was working (because it was fundamentally not working), and the symptom is severe. Given that, when I saw 3.11.2 being discussed, I should have spoken up, and I didn't -- so I don't have any grounds to complain about other people's handling. It surprised me that it didn't happen, but the lesson learned is that I shouldn't assume, not that other people should read my mind.

I still hold the opinion that it's useful in the PR list to be able to see at a glance the difference between bug fixes and enhancements (bug fixes are often more urgent than new features), and whether it's related to a crash or not. When evaluating PRs for inclusion in a release, you're looking at the PR list, not the issue list. I don't see a compelling reason to keep this information at one remove. But I haven't been maintaining the list myself, so I'll only offer it as an opinion and not press the point.

"Gripe" is a strong word, and not my intent at all.

i didn't realize it would read as strong, my apologies.

we'll have a more appropriate ticket to discuss this soon. i'll try to make sure your concerns are addressed.

Picking up again:

https://github.com/supercollider/supercollider/wiki/Continuous-Integration---Travis-&-Appveyor#storage-and-deployment-of-build-artifacts: "on every build of an internal branch, a ZIP macOS archive and ZIP Windows archives are deployed to S3, and (normally) the Windows archives are stored on AppVeyor too"

Just to doublecheck/confirm: Is it OK if I push this branch directly to this repo, to generate a test build? Not normal practice, so I wanted to ask before going ahead with that.

Thanks!

Is it OK if I push this branch directly to this repo, to generate a test build? Not normal practice, so I wanted to ask before going ahead with that.

yes, definitely ok!

Using the test build from s3, the fix seems OK.

• No crash!
• Sysrt messages are routed to the proper receiving functions.
• Active sensing appeared not to be received at all. However, the sysrt function wasn't called either -- so it looks to me like loopMIDI simply doesn't transmit active sensing messages.

In that case, we could probably go ahead and merge.

Actually, the arguments passed through to the rt responder functions might not make sense. I'd like to doublecheck that first.

Actually, the arguments passed through to the rt responder functions might not make sense. I'd like to doublecheck that first.

The argument does make sense: It's the device ID (which is relevant).

So I'm pretty well satisfied. I think it's OK to review.