ksksue / PhysicaloidLibrary

Android Library for communicating with physical-computing boards (e.g.Arduino, mbed)
http://www.physicaloid.com/
357 stars 151 forks source link

Asynchronous reading from the Arduino #7

Open iumez opened 10 years ago

iumez commented 10 years ago

I'd noticed that while my writes to an arduino seemed to be pretty quick, my reads are not. It seems that I needed to somewhere around 100ms before I consistently have good read results. Since I'm working on an app that needs bidirectional communication with the arduino at around 20Hz (or faster), this is an issue.

Looking at the cdc driver, it seems like you implement reading in a thread which does the usb bulk transfer every 50ms. Is there a reason 50ms was chosen? Would I run into issues if I modified it to run faster? Where is the com.ftdi.j2xx driver referenced in physicaloid? Why have reads by synchronous (though hidden from the user) while writes are truly asynchronous?

Having a 50ms wait time for a read seems at odds with the idea of providing an interface to physical computing boards.

iumez commented 10 years ago

Don't worry about the ftdi driver issue. I found it at ftdi's website then realized that I forgot to look in your libs folder!

elahera commented 10 years ago

I've been having the same problem than iumez with the libray.

My app sends a command to the Arduino One rev. 3 and is waiting for the Arduino's answer. I am tryng to adjust some delay to adjust but it is just trial/error. I've been working with the previous version of the library but even when I've updated to the latest one, in this last version is even worse, I don't receive the status (RX/TX) comm lights blinking. Would you mind to give me some tip?

iumez commented 10 years ago

Hi,

So, I dug into the library and saw that it is written so that reads are synchronous and occur in a thread that runs every 50ms. So you can imagine, depending on the timing, it can take up to two read cycles to get a full message back (100ms). I modified the library so that reads are asynchronous (like the writes). In my testing, this reduced the read latency down to about 3ms which is significantly better.

elahera commented 10 years ago

Hi,

Would you mind to give me more details about how to get the library reads asynchronously?

iumez commented 10 years ago

I modified the read() method in UartCdcAcm.java (at com/physicaloid/lib/usb/driver/uart/UardCdcAcm.java). I've attached my version so you can diff against the original source. Here are the key changes:

private void startRead() {
    if(mReadThreadStop) {
        mReadThreadStop = false;
        //ICU commented out for async read
        //new Thread(mLoop).start();
    }
}

Above, I commented out the creation of the read thread that the library originally used to do reads in the background (every 50ms).

//ICU for async read
private static byte[] rbuf = new byte[USB_READ_BUFFER_SIZE];

@Override
public int read(byte[] buf, int size) {
//ICU added following code for async read
int len = 0;
try {
        len = mConnection.bulkTransfer(mEndpointIn,
                rbuf, rbuf.length, 2);
    } catch(Exception e) {
        Log.e(TAG, e.toString());
    }

    if (len > 0) {
        mBuffer.add(rbuf, len);
        onRead(len);
    }
//ICU end added code
    return mBuffer.get(buf, size);
}

Above, I modified the read() method to perform a read immediately when called rather than simply reading from the buffer. Originally, this would only return what's the in buffer and the buffer would only get filled when the read thread ran (which was every 50ms). So the fastest you could theoretically read would be every 50ms. With my changes, you're just limited to how fast it takes for data to become available over usb.

Below is the code for the read thread (which now doesn't run since I commented out the call in startRead()):

private Runnable mLoop = new Runnable() {
    @Override
    public void run() {
        int len=0;
        byte[] rbuf = new byte[USB_READ_BUFFER_SIZE];
        for (;;) {// this is the main loop for transferring

            try {
                len = mConnection.bulkTransfer(mEndpointIn,
                        rbuf, rbuf.length, 2);
            } catch(Exception e) {
                Log.e(TAG, e.toString());
            }

            if (len > 0) {
                mBuffer.add(rbuf, len);
                onRead(len);
            }

            if (mReadThreadStop) {
                return;
            }

            try {
                Thread.sleep(50);
            } catch (InterruptedException e) {
            }

        }
    } // end of run()
}; // end of runnable

I'd initially experimented with reducing the sleep interval then decided to bypass the thread altogether. Note, I did not modify the ftdi or other drivers (arduino uno uses cdcAcm). However, you can modify them in exactly the same way if necessary.

-Iheanyi

On Tue, May 6, 2014 at 2:29 PM, elahera notifications@github.com wrote:

Hi,

Would you mind to give me more details about how to get the library reads asynchronously?

— Reply to this email directly or view it on GitHubhttps://github.com/ksksue/PhysicaloidLibrary/issues/7#issuecomment-42354500 .

iumez commented 10 years ago

I just realized that replying via email probably discards my attachment. If you still need a copy of my modified UartCdcAcm.java, send me an email: iheanyi (dot) umez (dot) eronini (at) gmail (dot) com

cody82 commented 10 years ago

How about a pull request?:)

iumez commented 10 years ago

Sure :). I'll be able to get on that tonight. I'm still a git and github infant...

pablichjenkov commented 10 years ago

@iumez @cody82 @elahera @Lauszus @ksksue Hi guys I found that a delay is necessary for the read method to work properly. If you eliminate the delay between each poll to the internal usb buffer, you will notice that after a certain amount of read times it will return -1 randomly. At least it happened to me. The root causes of this issue are unknown to me. I just would like to know if some of you noticed this problem or just happened to me.

iumez commented 10 years ago

Umm, that is the read method working properly. The code as originally written buffers reads for you, so application code doesn't notice that sometimes, there is no data available. If you bypass that and remove all delay, then you have to write your code to handle the case where no data is available for read to return.

Essentially, by removing code to buffer the read data, you're taking full responsibility for read timing and handling no data available cases. I would not suggest this if you don't have control (or know ahead of time) what the timing of serial data transmissions looks like on both ends.

-Iheanyi

On 8/23/14, pablichjenkov notifications@github.com wrote:

Hi guys I was working on building a usb library in android too. I found that a delay is necessary for the read method to work properly. If you eliminate the delay between each poll to the internal usb buffer, you will notice that after a certain amount of read times it will return -1 randomly. At least it happened to me. The root causes of this issue are unknown to me. I just would like to know if some of you noticed this problem or just happened to me.


Reply to this email directly or view it on GitHub: https://github.com/ksksue/PhysicaloidLibrary/issues/7#issuecomment-53154506

pablichjenkov commented 10 years ago

@iumez . Thanks for the response, I perfectly understand what you said. The read method return -1 when input buffer is empty. The problem is that it is returning -1 even when there is data in the buffer. I can assure there is data in the buffer because I have a USB annalizer Beagle 480, and it shows data pass from my board to android. However android does not pass it to me. It happens very random, once every 80 reads.

iumez commented 10 years ago

If you are constantly calling read, then it can happen at a period of time when a bulk transfer has not completed between the Beagle and the android phone. Data transfer is not instantaneous. It takes time to happen. The phone is doing more than just running your code and handling USB. There is no guarantee, even if you were constantly sending data to the phone, that at any one point in time, data will be available to be read if you are constantly reading.

On 8/23/14, pablichjenkov notifications@github.com wrote:

@iumez . Thanks for the response, I perfectly understand what you said. The read method return -1 when input buffer is empty. The problem is that it is returning -1 even when there is data in the buffer. I can assure there is data in the buffer because I have a USB annalizer Beagle 480, and it shows data pass from my board to android. However android does not pass it to me. It happens very random, once every 80 reads.


Reply to this email directly or view it on GitHub: https://github.com/ksksue/PhysicaloidLibrary/issues/7#issuecomment-53157827

pablichjenkov commented 10 years ago

@iumez Yeah I get the point where I could call the read method while bulktransfer is in process and then receive -1. However the next times when I read the Android internal buffer, I should get the data that was previously transmitted. In my case I don't see the data at all, seems like it were lost by the system. I was thinking that probably calling bulktransfer(...) without a delay could overhead the system or something like that. The truth is that when I add the delay I don't miss any packet. Even the Google example MissileLauncher has a delay of 50 ms between each usb request. See my post on stackoverflow: http://stackoverflow.com/questions/25464531/android-usb-host-input-bulktransfer-fails-to-read-randomly-when-data-available . I would like to add that I am communicating over bulktransfer directly with no cdc-acm interface.

iumez commented 10 years ago

Not necessarily. Depending on how fast you're reading, it might be a while before data is available. According to the stackoverflow question, you're sending data once a second. So, if you're reading faster than that, you may have to wait until the next second for data.

On Sat, Aug 23, 2014 at 2:26 PM, pablichjenkov notifications@github.com wrote:

@iumez https://github.com/iumez Yeah I get the point where I could call the read method while bulktransfer is in process and then receive -1. However the next times when I read the Android internal buffer, I should get the data that was previously transmitted. In my case I don't see the data at all, seems like it were lost by the system. I was thinking that probably calling bulktransfer(...) without a delay could overhead the system or something like that. The truth is that when I add the delay I don't miss any packet. Even the Google example MissileLauncher has a delay of 50 ms between each usb request. See my post on stackoverflow: http://stackoverflow.com/questions/25464531/android-usb-host-input-bulktransfer-fails-to-read-randomly-when-data-available . I would like to add that I am communicating over bulktransfer directly with no cdc-acm interface.

— Reply to this email directly or view it on GitHub https://github.com/ksksue/PhysicaloidLibrary/issues/7#issuecomment-53164507 .

pablichjenkov commented 10 years ago

I do, I wait untill the next read call and nothing happens. Seems like android does not keep it for me. Sent via phone

iumez notifications@github.com wrote:

Not necessarily. Depending on how fast you're reading, it might be a while before data is available. According to the stackoverflow question, you're sending data once a second. So, if you're reading faster than that, you may have to wait until the next second for data.

On Sat, Aug 23, 2014 at 2:26 PM, pablichjenkov notifications@github.com wrote:

@iumez https://github.com/iumez Yeah I get the point where I could call the read method while bulktransfer is in process and then receive -1. However the next times when I read the Android internal buffer, I should get the data that was previously transmitted. In my case I don't see the data at all, seems like it were lost by the system. I was thinking that probably calling bulktransfer(...) without a delay could overhead the system or something like that. The truth is that when I add the delay I don't miss any packet. Even the Google example MissileLauncher has a delay of 50 ms between each usb request. See my post on stackoverflow: http://stackoverflow.com/questions/25464531/android-usb-host-input-bulktransfer-fails-to-read-randomly-when-data-available . I would like to add that I am communicating over bulktransfer directly with no cdc-acm interface.

— Reply to this email directly or view it on GitHub https://github.com/ksksue/PhysicaloidLibrary/issues/7#issuecomment-53164507 .


Reply to this email directly or view it on GitHub: https://github.com/ksksue/PhysicaloidLibrary/issues/7#issuecomment-53165787

fredcooke commented 10 years ago

Can you please push these changes to github? For my purposes I have near continuous 115200 data stream and need high efficiency and low latency. This sounds like a good direction to move in.