CoreMIDI4J doesn't like repeatedly disconnecting devices #19
Comments
|
Forgot to include the err log. |
|
An additional bug, but one that I cannot produce a log for. It looks like you can't dynamically allocate receivers.
Due to this and the previous bug, at present I have no choice but to create devices initially, then create hand-coded pipes for the receivers and transmitters that manage opening and closing. This works but means that I cannot dynamically handle new devices coming online if the user plugs a synth into his computer after the software has been launched. |
|
Hi, Sorry for the delay in replying. I am away on business and working manic hours, so not sure when I will get to look at this. It would be helpful if you can provide some sample code that guarantees both of the crashes you are talking about to give us something to work with in addition to the descriptions that you have provided. |
|
Yes, sorry, I am incredibly buried with my other projects as well, and don’t know when I might have time to look at issues like this, but sample code that reproduces it reliably will definitely be key. Derek, if you do happen to find and fix it, I will of course make time to publish a new release. |
|
Just a thought. When you close all the transmitters and receivers, make sure that the devices are open before opening the new transmitters and receivers. (MidiDevice.isOpen()) I saw something that indicated that the devices may close if the receivers and transmitters are closed. I'm curious and will try running your project. |
|
I'll try to get you guys a simple piece of example code [I've since modified my program heavily to work around the issue, so it won't be of much use to you except in early versions on github].
Sean
…On Jun 6, 2017, at 7:22 AM, James Elliott ***@***.***> wrote:
Yes, sorry, I am incredibly buried with my other projects as well, and don’t know when I might have time to look at issues like this, but sample code that reproduces it reliably will definitely be key. Derek, if you do happen to find and fix it, I will of course make time to publish a new release.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
On Jun 6, 2017, at 7:22 AM, James Elliott ***@***.***> wrote:
Yes, sorry, I am incredibly buried with my other projects as well, and don’t know when I might have time to look at issues like this, but sample code that reproduces it reliably will definitely be key. Derek, if you do happen to find and fix it, I will of course make time to publish a new release.
I am attaching an example stripped out of older code I had. It's not minimal but should be enough to demonstrate what's going on. The code gathers all the available MIDI devices for a receiver and two transmitters, then attaches receivers to the transmitters. It does this three times. It then closes everything cleanly, at which point it crashes hard. The code *should* be closing the existing receivers/transmitters and devices when it opens new ones. I could try stripping it down further if need be.
|
|
To eclab I don't see the attached code |
|
Sorry, it looks like github silently removes files with "unsupported extensions" from email messages. :-( And for some reason I cannot fathom, .java is an unsupported extension. Try now. |
|
For one thing, you are testing if the various objects are not null, but I think you also need to check if the devices, transmitters, and receivers are open. The synchronized lock doesn't appear right. See http://winterbe.com/posts/2015/04/30/java8-concurrency-tutorial-synchronized-locks-examples/ and the java.util.concurrent.locks package. It looks like it should be synchronized(this) according to the winterbe.com blog post. I'm going to have to look at this in more detail, but that is the result of my first look. See https://stackoverflow.com/questions/10963205/how-to-attach-file-to-a-github-issue It appears that the acceptable file types are PNG, GIF, JPG, DOCX, PPTX, XLSX, TXT, or PDF |
|
On Jun 20, 2017, at 3:08 AM, Bradley Ross ***@***.***> wrote:
For one thing, you are testing if the various objects are not null, but I think you also need to check if the devices, transmitters, and receivers are open.
They are null because I set them to be null immediately after closing them to make it clear that they're closed. I think the code's right.
The synchronized lock doesn't appear right.
You can comment out all the synchronized statements in the code because it's only single-threaded (its a remnant of my main code) but it won't make any difference.
I'm pretty sure the synchronization code is correct. Locking on a lock object other than [this] is good practice for many purposes, particularly if you need to have independent locks for different methods. Among the best choices for lock objects is an empty array because it is serializable (many other objects, including new Object(), are not).
|
|
Hi, Apologies for lack of response. I am am away on business and currently rammed. Any free time I have I have on programming is currently spent resolving an issue I have with one of my Java Librarians that use CoreMIDI4J. Will look at this as soon as I can, but if you can find the issue and generate a pull request then I am sure that James and I will be able to respond to that much more quickly. I am happy to consider extending who can actively contribute as well if you have the interest. CoreMIDI4J was put here so it could be supported unlike the older providers.. |
|
I should have some time to play with this over the weekend too, I hope. My massive updates to dysentery, beat-link, and beat-link-trigger are out in preview form, and feedback is settling down into a happy place. |
|
So I have been poking a bit at this, and do not have a definitive answer yet. I discovered that the example was crashing trying to free a bit of memory that was not allocated, in Now, I am not going to be nearly as efficient at debugging this as Derek might because I did not create these classes and I don’t understand the details of their relationships yet, but I did notice a few things that made me go “hmm.” The Indeed, I just tried adding protection against this, and the crash has gone away, the example program has been running with no problems for twenty minutes now. I will want to go through the other classes and add similar protections, but this seems a step in the right direction. In the mean time I will make a preview release with this fix in it and see if it addresses Sean’s ( @eclab ) issue at all. @DerekCook I also notice that the |
Make sure CoreMidiSource and CoreMidiDestination are protected against being opened or closed multiple times. Also improves conformance with the Java MIDI SPI spec by returning empty lists when there are no receivers/transmitters rather than NULL.
|
OK, @eclab here is a preview version with this fix present, can you see if it behaves better for you? I have to zip it up, as we noted earlier in the discussion.
|
|
@DerekCook I fixed a couple of places where we were returning |
|
Everyone, here is a newer snapshot in which I have gone ahead and implemented the rest of the things we are supposed to do in terms of tracking transmitters and receivers that we create, as well as removing them when they get closed, and closing them when their parent MIDI device closes. This makes us much more correctly implement the MIDI Service Provider Interface contract, and will hopefully fix the problems @eclab was seeing. @DerekCook , this is enough of a change that you probably want to test it with some of your synths as well. But if it looks good to everyone, we should probably make another release with it soon.
|
|
And here is one more (last, I hope?) snapshot version to try. I further noticed that we were not closing our devices when we notice them disappearing from the MIDI environment. This cleans that up, so that if anyone is holding on to a device reference for something that has been detached from the MIDI environment (or any receivers or transmitters associated with such a device), they will properly be marked as closed, and give you appropriate exceptions when you try to use them at that point. Again, you will have to unzip this in order to test it. |
|
All, I'm going to have exceptionally poor internet access for about a month due to vacation location. I'll try to do a bit of testing but the turnaround could be weeks. Thanks for getting on this.
Sean
|
|
No worries! Our turnaround was not particularly fast in responding to your report, so that is only fair. 😁 And I suspect it may be a while before Derek has time to do his testing too. |
|
James, Thanks very much for getting here first. I of course trust your judgement in what the correct behaviour should be, and the focus in the early days was getting something that worked that undoubtedly would need improvement! I will be back home soon, and will be doing a new release of my librarians as a have quite a few fixes batched up (have been able to work on them whilst away on a laptop (but no synths and MIDI devices), so will need to do some synth testing, and I will do that with then new release of CoreMIDI4J. |
|
That sounds perfect. And of course, I understand well the challenge of getting this library working at all, and the plan of polishing rough edges as we find them! I was so daunted by the prospect of the initial implementation that I waited for you to come along and do it. I’m glad to be able to contribute a bit from time to time. |
|
Have tried the new release with all of my librarian and synth combinations, and all looks good. But suggest we keep this open until we hear from Sean. |
|
Thanks, that’s great (and fast)! And I agree. |
|
@eclab I hope you had a lovely vacation, and will have a chance to test this fix. |
|
Not back yet! Back in a week. So you have to give me a few. :-)
…On Aug 5, 2017, at 9:53 PM, James Elliott ***@***.***> wrote:
@eclab I hope you had a lovely vacation, and will have a chance to test this fix.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Hi, when you get time, please give this a go and let us know how it works for you. |
|
I think that your proposal is basically the same as mine. I had one other gizmo: a variety of USB MIDI hardware just calls itself the generic "USB MIDI Device". We'd like to be able to rename that. Since we can rename the device name, I had added:
|
|
I saw that, but I did not think it made sense to try to interpret the meaning of non- |
|
That one's not a big deal, I agree. You could always rename the device and then you'd have "USB MIDI Device (My Device Name)" or whatnot.
I had included it because I noted that several of my hardware devices all called themselves the same "USB MIDI Device". Either that's a generic name standard, or perhaps Apple's system was naming them this? At any rate, it didn't seem arbitrary.
|
|
@eclab before I consider all of the above (where in a lot of places we all seem to be in danger of agreeing in most places), can you please:
I don't have much time for a detailed response this morning, as I need to high tail it into work early, but, unless I am mistaken, I think there is some confusion on what is going to be returned as the name under the new scheme - but I would like it verified on what is happening on your system. Just taking one example end point from your log for your microsampler This is end point data for a device with multiple entities - (2) The name that CoreMIDI4J should return is Can you please verify that this is what happens when you run the new snapsot? |
|
Hi I had some time tonight to compare suggestions. @eclab suggested It's not dissimilar to what I have done but I think it is over complicated for the examples I have seen on my own system and what the debug info provided. @brunchboy 's suggestion is simpler I agree with this apart from the final point. I see no reason to return the endpoint name for a device with a single or no entity if you have a device description. If the user has not edited that description then you get the default device name set by the manufacturer (usually the same as the endpoint name), otherwise you get what the user wants. The time when it makes sense to combine the Device Name and Endpoint name is when the entity count is more than one. In this case append the endpoint name to the device name. My code is as follows What this means As an example, under the old code that only used the end point name, my Motif Rack ES would return From those end point descriptions, I don't really know (other than by surmising) that these ports are from my Motif. What if I had another synth using those end point names. How do I tell them apart? That is why when there are multiple entities, adding the end point name to the device name gives a meaningful description. For my Motif Rack with Device Name For @eclab's microSampler you should get I think these are far more meaningful names than just using the end point name when there are multiple end points, and the implementation is simpler than suggested. As mentioned, I would be grateful if you could run the snapshot on your system (it still has the native debug info in) and report bck what you get Latest snapshot reattached here for convenience. |
|
I agree, Derek, I am eager to see what Sean gets running that snapshot on his system. It may well be that your current approach is perfectly good. The situation I was trying to protect against, which may be a non-issue, is if there are manufacturers of single-entity devices whose device names are utterly generic and indistinguishable, especially if you have multiple such devices connected. In such cases, yes, you can edit the device name if you know how to do that, but you may have a hard time figuring out which is which, or may not know you can do that. But if we don’t have any such devices today, there is a good argument for waiting until someone reports an issue about one. |
|
Hi, James Agreed, but at that point you should be able to edit the descriptions to distinguish them, which is covered by my code in response to #21 . I just need to know if the code is working now. Looks good from here. |
|
Yes, understood! Looks good to me too. And we can add some instructions on the project page telling people how to rename their devices if they need to. |
|
I'm posting a new log of attached devices. There's good news and bad news. The good news is that all but one of my devices has a rational name now. The bad news is that the microSAMPLER names aren't right. I'm adding a new USB device to the log: an Eventide ModFactor pedal. The names I get displayed are: microSAMPLER Port 1 The bad ones here are microSAMPLER Port 1 and microSAMPLER Port 2. There are four java MIDI Devices presented from the microSAMPLER: two transmitters and two receivers. The transmitters are called "KBD/KNOB" and "MIDI IN", and the two receivers are called "SOUND" and "MIDI OUT" (I may have mixed some of these up). But when presented in Java, now they're both called Port 1 and Port 2. I believe the reason for this is that the current snapshot may be using the entity name, not the endpoint name. On the microSAMPLER (and I have no doubt also the MicroKorg etc.) the entity name is not descriptive. Would it be possible to use the endpoint name instead? As to only using the device name if there's a single entity, that seems reasonable to me, though what happens if you have one device, one entity, but two endpoints with different names? For example, imagine the microSAMPLER with only one port, named SOUND and KBD/KNOB at its in and out. They'd both be just called microSAMPLER. I guess that's not a big deal, but... |
|
Thanks. Yes, I've picked up the entity name by mistake whilst doing some clean up. It should indeed be the endpoint name. My bad. I didn't spot that as my main rack with my Motif Rack ES is in the keyboard room and weighs a ton, so I didn't haul it back in after the change. Should have checked with it as well. I'll fix that tonight (and check with the Motif) and we should be there. Regarding your final point: entity end points usually do have the same name, it's the microsampler that seems to do things differently compared to all the other interfaces I have seen. But end points are in/out pairs, so you usually present them in different lists. But I can do a name compare and add the end point name if they are different. |
No, just thinking out loud. I've checked the MicroKorg XL manual and it has the same four endpoints as the Microsampler. But I wonder if the original Microkorg has only two endpoints and one device. I know someone who owns one: I might be able to check. It seems an obvious candidate. |
|
OK. I have changed my text above, so what you quoted has disappeared. But if you can check this original MicroKorg it would be interesting |
|
It sounds like we’re getting very close! If you want me to test tonight’s version with some other random MIDI hardware I have lying around I can do so as well. This is going to be a good release, my output menu in Beat Link Trigger is already much more meaningful, though I will definitely have to warn my users to go through and reconfigure all their existing triggers, as the names of many outputs will have changed. |
|
Looks like I can't get ahold of the MicroKorg for a few days. I'll get back with you on its results.
|
|
Hi, yes the more devices we can test with between us the better. Mght as well crack any issues now! I've checked all of my interfaces/devices here, including dragging my Rack with the Motif In to be beside my Mac. Looks like I missed the entity instead of endpoint name issue because the Motif Entity and Endpoint names are the same. Silly mistake, mind! Also caught another small little bug with the extended info I now put into CoreMidiDeviceInfo, so that is fixed as well. Anyhow, try this one. It still has the debug info uncommented. If it looks good, I will remove the debug info and push the latest code back to Github ready for a release. I did not have time to do anything other than correct the end point name, as it has been a very long day over here. But I think that will do for now. coremidi4j-1.1-SNAPSHOT.jar.zip As James says, we will need to make our users aware of name changes (my x.factory librarians warn when they can't find the port name on startup), but I think it is for the better. I certainly find the names more meaningful esp with my Motif and its multiple entities. My Kronos and Montage are the same, but I have not written librarians for them yet. It will certainly avoid future confusion in my setup when they are added! |
|
The default names of all of my MIDI devices look good. In many cases, far better than they used to. As far as I am concerned, this release is ready to be shared with the world. |
|
Thanks, James. Good to hear. I'll finish the code off tonight and push to the main branch in preparation. |
|
Hi. Have commented out the debug code and pushed the changes to the main branch. |
|
Thanks, Derek. I will wait a bit in the hopes @eclab has a chance to test the latest snapshot, but plan on deploying a release this weekend barring any surprises. |
|
On Sep 15, 2017, at 3:00 PM, James Elliott ***@***.***> wrote:
Thanks, Derek. I will wait a bit in the hopes @eclab has a chance to test the latest snapshot, but plan on deploying a release this weekend barring any surprises.
I can test the snapshot on my current machines tonight. However I have obtained the MicroKorg and although the manual discusses USB, the unit does not have a USB port! Probably too old a model. So that's out for testing.
|
|
Looking good. My devices display as: |
|
Great. Thanks for reporting back. Looks like a wrap then, for now, and I need to get back to other things. James, when I get five minutes, I will think of some words to explain the name changes that can go into the release notes. |
|
Thanks again for all this work, Derek! Let me know if you do, otherwise I was going to write something up myself during the release process. We’ll see who gets there first. |
|
I am working on a fairly detailed explanation of this change, with sections aimed both at developers using our library, and end-users of their applications, since I will then be able to point my Beat Link Trigger and Afterglow users at the right heading, so if you haven’t started yet, @DerekCook, you may want to wait and see if my effort meets your needs as well. |
|
No, I haven't started yet. Had to be in work today :-( So I'll wait and see what you come up with, if you've started. |
|
That’s fine! It saves us wasting or duplicating work. :) You can take a look at my draft and see what you think, I need to do other things for a bit. https://github.com/DerekCook/CoreMidi4J#device-names |
|
I’m guessing you didn’t have time to take a look, but I am going to go ahead and do the release. We can change the README whenever and as often as we want if you find things you want added or changed. |
|
Hi, no I did not get to look at it. It was a long day, and then chilling out and much needed sleep! I've had a quick look at the description this morning, and it is great. Covers most of what I wanted to say, so will just embellish it a little to add in my deltas. Thanks |
|
Thanks for those improvements! |
I'm getting a lot of crashes after disconnecting devices and reconnecting again (perhaps 3 or 4 times -- not many!). This is in Edisyn (https://github.com/eclab/edisyn/), from selecting the "MIDI:Change MIDI" menu 3 or 4 times. Try it out! I'm running Java 1.7.0_51 on OS X 10.9.5. I'll usually get a hang or a hard VM dump. Example VM dump stack trace:
The text was updated successfully, but these errors were encountered: