UsbRequest.queue crashing Android 3.1 application

Question

I'm developing an Android 3.1 application that uses USB Host mode to communicate with my keyboard (Korg M3) via MIDI over USB. This is running on my Xoom with Android 4.0.3 installed. I'm able to receive MIDI messages over USB without any problems, but sending note data back to my keyboard is having mixed success, with frequent crashes after a half-second delay.

Here's the error I keep getting as I tap the button on the Action Bar to send a note:

E/dalvikvm(6422): JNI ERROR (app bug): accessed stale global reference 0x1da0020a (index 130 in a table of size 130)

What I've checked/tried to track down the cause:

• Since the code is multithreaded, I have Java synchronized blocks surrounding accesses to the output request pool, input request pool (as per the ADB sample in the Android documentation), and a custom lock object for the current output request & associated ByteBuffer object references. I have structured the code that performs these locks to minimise the likelihood of deadlocks occurring.
• When retrieving an available UsbRequest object from the relevant request pool, I am setting the clientData reference to a new ByteBuffer object, rather than reusing the previously-associated ByteBuffer object and calling clear() on it.

• I have added extensive logging calls (for logCat) at the critical points in the code to try and track down where exactly it's failing. What I've found is that the error eventually occurs at the following point (this code works fine for a few times up until then):

  public void sendMidiData()
  {
      synchronized(_outputLock)
      {
          if(_currentOutputRequest == null || _outputBuffer.position() < 2)
              return;
  
          Log.d(_tag, "Queuing Send request");
          //// ERROR - happens in this next statement:
          _currentOutputRequest.queue(_outputBuffer, _maxPacketSize);
          Log.d(_tag, "Send request queued; resetting references...");
          //Initialise for next packet
          _currentOutputRequest = null; //getAvailableSendRequest();
          _outputBuffer = null; //(ByteBuffer)_currentOutputRequest.getClientData();
          Log.d(_tag, "Output request & buffer references set.");
      }
  }
  

• I have also tried setting the _currentOutputRequest and _outputBuffer references to null so that the available request is retrieved only when the next MIDI event is to be written to the buffer. If null is replaced by the original calls (as shown commented out), the next available request is retrieved immediately. This has made no difference.

Is there any idea as to what would be causing this issue? Could this be a previously-undiscovered bug in Android? A search on the error returned doesn't bring back much; mainly references to NDK programming (which I'm not doing).

Cheers.

Answer 1

Okay, I had a bit of a breakthrough and found two issues:

• The call to _currentOutputRequest.queue(_outputBuffer, _maxPacketSize); was passing the entire buffer capacity _maxPacketSize, which has a constant value of 64 (bytes). Apparently this only works for bulk reads, where up to 64 bytes will be read; bulk Send requests need to specify the exact number of bytes being sent.
• The _currentOutputRequest.queue() and _connection.requestWait() method calls appear not to be thread-safe, particularly in the implementation of UsbDeviceConnection (which is the type of _connection). I suspect the UsbRequest _currentOutputRequest internally uses the UsbConnection object when queuing a Send request.

How I fixed this:

The call to queue() is changed to:

_currentOutputRequest.queue(_outputBuffer, _outputBuffer.position());

For the second issue, the queue() statement already occurs inside a synchronized block, using the _outputLock object. In the Run() method of the reader thread, I have had to wrap the call to requestWait() in a synchronized block also using outputLock:

UsbRequest request = null;
synchronized(_outputLock)
{
    // requestWait() and request.queue() appear not to be thread-safe.
    request = _connection.requestWait();
}

Given that this occurs in a while loop and requestWait blocks the thread, one major issue I found is that the lock causes starvation when trying to queue a Send request. The result is that while the application receives and acts on incoming MIDI data in a timely fashion, outputted MIDI events are significantly delayed.

As a partial fix for this, I inserted a yield statement just before the end of the while loop to keep the UI thread unblocked. (The UI thread is temporarily being used as a note event is triggered by a button press; this will eventually use a separate playback thread.) As a result it's better, but not perfect, since there's still quite a delay before the first output note is sent.

A better solution:

To resolve the demand on a mutual lock for reading and writing asynchronously, the asynchronous queue() and requestWait() methods are only used for read operations, which remain on the separate 'reader' thread. Because of this, the synchronized block is unnecessary, so this segment can be reduced to the following:

UsbRequest request = _connection.requestWait();

As for write/send operations, the core of this is moved to a separate thread for executing synchronous bulkTransfer() statements:

private class MidiSender extends Thread
{
    private boolean _raiseStop = false;
    private Object _sendLock = new Object();

    private LinkedList<ByteBuffer> _outputQueue = new LinkedList<ByteBuffer>();

    public void queue(ByteBuffer buffer)
    {
        synchronized(_sendLock)
        {
            _outputQueue.add(buffer);
            // Thread will most likely be paused (to save CPU); need to wake it
            _sendLock.notify();
        }
    }

    public void raiseStop()
    {
        synchronized (this)
        {
            _raiseStop = true;
        }

        //Thread may be blocked waiting for a send
        synchronized(_sendLock)
        {
            _sendLock.notify();
        }
    }

    public void run()
    {
        while (true)
        {
            synchronized (this)
            {
                if (_raiseStop) 
                    return;
            }

            ByteBuffer currentBuffer = null;
            synchronized(_sendLock)
            {
                if(!_outputQueue.isEmpty())
                    currentBuffer =_outputQueue.removeFirst(); 
            }

            while(currentBuffer != null)
            {
                // Here's the synchronous equivalent (timeout is a reasonable 0.1s):
                int transferred = _connection.bulkTransfer(_outPort, currentBuffer.array(), currentBuffer.position(), 100);

                if(transferred < 0)
                    Log.w(_tag, "Failed to send MIDI packet");

                //Process any remaining packets on the queue
                synchronized(_sendLock)
                {
                    if(!_outputQueue.isEmpty())
                        currentBuffer =_outputQueue.removeFirst();
                    else
                        currentBuffer = null;
                }
            }

            synchronized(_sendLock)
            {
                try
                {
                    //Sleep; save unnecessary processing
                    _sendLock.wait();
                }
                catch(InterruptedException e)
                {
                    //Don't care about being interrupted
                }
            }

        }           
    }
}

At first I was concerned about the asynchronous code conflicting with the above (since they share the same UsbDeviceConnection), but this seems not to be an issue since they're using completely different UsbEndpoint instances.

The good news is that the application runs more smoothly and doesn't crash when sending notes at the same time as I'm playing the keyboard.

So in general (for bi-directional USB communication on separate endpoints), it seems that the asynchronous methods are best suited for read/input operations where we don't need to worry about defining polling behaviour (whether or not this is what happens internally), while output/send operations are better served by the synchronous bulkTransfer() method.

Answer 2

I think there's a bug in android_hardware_UsbRequest.cpp.

In function android_hardware_UsbRequest_queue_direct(), the Global Ref is set after the request is queued, so the waiting thread may get control before the reference is set.

The Global Ref is to the Java UsbRequest object, and UsbDeviceConnection.requestWait uses this to recover the UsbRequest object. If the waiting thread gets in before the ref is set, this will be a stale reference.

The obvious solution is to set the global reference before calling usb_request_queue(), and delete the reference if the call fails.

if (usb_request_queue(request)) {
    request->buffer = NULL;
    return false;
} else {
    // save a reference to ourselves so UsbDeviceConnection.waitRequest() can find us
    // we also need this to make sure our native buffer is not deallocated
    // while IO is active
    request->client_data = (void *)env->NewGlobalRef(thiz);
    return true;
}

Update: I have raised this as AOSP issue# 59467.