help understanding feature transfers.

[ulao]

Hi all, I have been working with a usb device for a few years though it is a bit bang implementation (v-usb), thus a bit limited. I do not however feel the firmware only driver is my issue.

I wanted to add a send method for an LCD I have on a game controller. All aspects of the game controller work fine including filling the LCD screen with progmem data. My attempt to do this was with the usb.dll or a "usb-libusb" from a older ddk lib.  The code to send was quite simple.
usbSetReport(dev, USB_HID_REPORT_TYPE_FEATURE,somedata, sizeof(somedata)))
I found I had to send my data in two chucks because there is a 128 byte max. From the best I can tell and from sniffing the USB data, this is a control transfer and it does not seem to be using an interrupt. Here is my descriptor for those IDs.

Code: [Select]

0x06, 0x00, 0xff,              // USAGE_PAGE (Generic Desktop)
    0x09, 0x01,                    // USAGE (Vendor Usage 1)
    0xa1, 0x01,                    // COLLECTION (Application)
    0x15, 0x00,                    //   LOGICAL_MINIMUM (0)
    0x26, 0xff, 0x00,              //   LOGICAL_MAXIMUM (255)
    0x75, 0x08,                    //   REPORT_SIZE (8)

    0x85, 0x11,                    //   REPORT_ID (17)
    0x95, 0x06,                    //   REPORT_COUNT (6)
    0x09, 0x00,                    //   USAGE (Undefined)
    0xb2, 0x02, 0x01,              //   FEATURE (Data,Var,Abs,Buf)

    0x85, 0x12,                    //   REPORT_ID (18)
    0x95, 0x83,                    //   REPORT_COUNT (131)
    0x09, 0x00,                    //   USAGE (Undefined)
    0xb2, 0x02, 0x01,              //   FEATURE (Data,Var,Abs,Buf)
    0xc0                           // END_COLLECTION

Now as far as v-usb goes, I need to use long transfers and it populates the data in 8 byte chucks. The LCD does fill up but never the same and always in a block un-arranged pattern. At times, less then %5 it fills up the entire image. So some of that data is getting out of wack.

this is the screen shot of the usbLyzer data. See the red (stalls), sorry I had the wrong controller highlighted at the time of the snap shot.


From what I know the stall indicate the following.

When a device receives a request that is undefined, is inappropriate given the current setting or state of the device, or uses values that are inappropriate for the particular request, then a Request Error exists. The device handles a Request Error by returning a STALL PID to the next DATA or STATUS stage, preferably at the next DATA stage transaction.

Since it's random and not every send that errors, "a request that is undefined" and "values that are inappropriate for the particular request" must not be the issue, thus my error must be "inappropriate given the current setting or state of the device".  So I believe its how I'm using the v-usb driver. They do have a crc option that gets enabled in their usbconfig but it does not help. I did notice it took much longer to attempt to fill the screen. So I do not think crc is my issue. As far as I can tell, it seem like the packets that get delivered are intact, just not all of them are sent (maybe rejected).

So a bit about my code.
my usbFunctionSetup passes on all USBRQ_HID_SET_REPORT's to my usbFunctionWrite
if(rq->bRequest == USBRQ_HID_SET_REPORT) return USB_NO_MSG;
then I handle my report 18 (set) like this

Code: [Select]

if ( data[0] == 18)
{     
return handleSendData ( data );
 
}

char handleSendData(uchar *data)
{


//0 hold report buffer
//these go in transfers...
if (data[2]== 0x24)
{     
//dc lcd
bytesRemaining = 116;//120 + this byte.
needMoreThen8Data=0x24;//set flag
currentAddress = 4;//can only load 4 in here.
for (char i=0;i<4;i++) lcd_data[i] = data[i+4];
return 0;
}
else if (data[2]== 0x25)
{     
//dc lcd two
bytesRemaining = 68;//71 remaing 
needMoreThen8Data=0x25;//set flag (use same flag)
currentAddress = 123;//can only load 4 in here.
for (char i=119;i<123;i++) lcd_data[i] = data[i+4];
return 0;
}
return 1;
}

As I mentioned I need to do 128 then the remainder. This code is just for the first 4 bytes because of the header info. So the driver passes the remainder payload back to the usbFunctionWrite. so in that function I also have

   

Code: [Select]

    if(needMoreThen8Data)
    {
        if(bytesRemaining == 0) {needMoreThen8Data=0;return 1;}//finished
        if(len > bytesRemaining) len = bytesRemaining;

        if(needMoreThen8Data==0x12)
for (char i=currentAddress;i<currentAddress+8;i++)
{mapper[i] = data[j]; j++;}
if(needMoreThen8Data==0x24 || needMoreThen8Data==0x25)
{
for (unsigned char i=currentAddress;i<currentAddress+8;i++)
{lcd_data[i] = data[j]; j++;}
//if (needMoreThen8Data==0x25)
_lcdNeedsWrite = 1;
}



        currentAddress += len;
        bytesRemaining -= len;
        if(bytesRemaining == 0) {needMoreThen8Data=0;return 1;}//finished
return 0;
    }

One thing worth a mention here is that the entire time this is going on my normal usb poll operation runs. So far I have not attached this to the problem. It does not seem to be the culprit. There is no problem skipping this during a transfer but I can not stop this for more the 8 ms as the USB will die on me. My gamepad data payload is more the 8 bytes so I do it in two pulls as v-usb is only 1.1

Code: [Select]

while (!usbInterruptIsReady()) usbPoll(); usbSetInterrupt( reportBuffer, 8);
cli();
if (!_autoPause) curGamepad->doWork();//always less then 8 ms
sei();
while (!usbInterruptIsReady()) usbPoll();  usbSetInterrupt((void *)&reportBuffer + 8, 8);//2 extra are for ps3 see descriptor notes
I have tried omitting all of this and the problem remains but felt I should mention it.
Also here is how I set up my pipes.

Code: [Select]

     9,          /* sizeof(usbDescriptorConfiguration): length of descriptor in bytes */
    USBDESCR_CONFIG,    /* descriptor type */
    18 + 7 * USB_CFG_HAVE_INTRIN_ENDPOINT + /*7 * USB_CFG_HAVE_INTRIN_ENDPOINT3*/ + 7 + 9, 0,
                /* total length of data returned (including inlined descriptors) */
    1,          /* number of interfaces in this configuration */
    1,          /* index of this configuration */
    0,          /* configuration name string index */
    USB_CFG_IS_SELF_POWERED,  /* attributes */

    USB_CFG_MAX_BUS_POWER/2,            /* max USB current in 2mA units */
/* interface descriptor follows inline: */
    9,          /* sizeof(usbDescrInterface): length of descriptor in bytes */
    USBDESCR_INTERFACE, /* descriptor type */
    0,          /* index of this interface */
    0,          /* alternate setting for this interface */
    1 + USB_CFG_HAVE_INTRIN_ENDPOINT ,//+ USB_CFG_HAVE_INTRIN_ENDPOINT3,   /* endpoints excl 0: number of endpoint descriptors to follow */
    USB_CFG_INTERFACE_CLASS,
    USB_CFG_INTERFACE_SUBCLASS,
    USB_CFG_INTERFACE_PROTOCOL,
    0,          /* string index for interface */

    9,          /* sizeof(usbDescrHID): length of descriptor in bytes */
    USBDESCR_HID,   /* descriptor type: HID */
    0x10, 0x01, /* BCD representation of HID version */
    0x00,       /* target country code */
    0x01,       /* number of HID Report (or other HID class) Descriptor infos to follow */
    0x22,       /* descriptor type: report */
    USB_CFG_HID_REPORT_DESCRIPTOR_LENGTH, 5,  /* total length of report descriptor *///
//#endif


#if USB_CFG_HAVE_INTRIN_ENDPOINT    /* endpoint descriptor for endpoint 1 */
    7,          /* sizeof(usbDescrEndpoint) */
    USBDESCR_ENDPOINT,  /* descriptor type = endpoint */
    0x81,       // bulk IN endpoint number 1
    0x03,       /* attrib: Interrupt endpoint */
    8, 0,       /* maximum packet size */
    0x08, /* in ms*/

//the output.

    7,          // sizeof(usbDescrEndpoint)
    5,  // descriptor type = endpoint
    0x01,        // out endpoint number 2
    0x03,       // attrib: Interrupt endpoint
    8, 0,       // maximum packet size
    0x08, // in ms
#endif     

I have asked for help over at avr-freaks, the v-usb board, and my good old friends at microchip. No one seems to be able to help. I'm sure its the way I'm using v-usb but the project is about 18 files and the main alone is huge. If there is something more I can add let me know. I welcome any ideas at all, I don't play expect, I'm human and I make many mistakes. They are how I learn.

[bpaddock]

I'll leave the USB question to those with the knowledge.
I did want to point out a couple of issues with AVR GCC code.

"Random"

AVC-GCC is particularly critical of missing 'volatile' directives where they are needed.  Are they in all the proper places?

Replace:

Code: [Select]

while (!usbInterruptIsReady()) usbPoll(); usbSetInterrupt( reportBuffer, 8);
cli();
if (!_autoPause) curGamepad->doWork();//always less then 8 ms
sei();
while (!usbInterruptIsReady()) usbPoll();  usbSetInterrupt((void *)&reportBuffer + 8, 8);//2 extra are for ps3 see descriptor notes
with this:

Code: [Select]

  while (!usbInterruptIsReady()) usbPoll(); usbSetInterrupt( reportBuffer, 8);

#include <atomic.h>
  ATOMIC_BLOCK( ATOMIC_RESTORESTATE ) /* Disable interrupts, renable when exit ATOMIC_BLOCK{} scope */
  {

     if (!_autoPause) curGamepad->doWork();//always less then 8 ms

  }
  while (!usbInterruptIsReady()) usbPoll();  usbSetInterrupt((void *)&reportBuffer + 8, 8);//2 extra are for ps3 see descriptor notes
That should fix any problems with 'code motion' where the compiler reorders the cli() to disable interrupts.

[Jan Axelson]

What I'm seeing in the screen capture is:

Set Report - stall
Set Report - stall
Set Report - error is "Tra..." - what is the full text?
Set Report - success

and repeated:

Set Report - stall
Set Report - stall
Set Report - error is "Tra..."
Set Report - success

I agree that "inappropriate given the current setting or state of the device" is the likely reason for the stalls.

Can you determine if the endpoint is stalling the Setup stage (containing the request) or the data stage (with the report data)? That might offer a clue.

[ulao]

wow didnt see that.  I attached the log (not sure if you can open it). It says unsuccessful transfer

[The attachments upload directory is not writable. Your attachment or avatar cannot be saved.]
http://spawnlinux.ddns.net/DoCz/down_m/temp/usbstall.ulz

Can you determine if the endpoint is stalling the Setup stage (containing the request) or the data stage (with the report data)? That might offer a clue.

hmmm, well set up only has that one line for set reports
else if(rq->bRequest == USBRQ_HID_SET_REPORT) return USB_NO_MSG;   
Thou what I did for a test here was put a count in there.
else if(rq->bRequest == USBRQ_HID_SET_REPORT) {reportBuffer[BUTTON_ROW_3]++; return USB_NO_MSG;   }
I then did the send routine 4 times. I got 6, then 6, then 6, then 5. So the issue is at least apparent in the setup.

 bpaddock, that is interesting, I have all kinds of cli's in my code. Can you explain that in more detail, or link to an explanation. I'd like to understand the pros and cons better.

p.s. I just realized I'm talking to the Jan  It's a pleasure.

[bpaddock]

bpaddock, that is interesting, I have all kinds of cli's in my code. Can you explain that in more detail, or link to an explanation. I'd like to understand the pros and cons better.

"Problems with reordering code":
http://www.nongnu.org/avr-libc/user-manual/optimization.html#optim_code_reorder


Explains atomic.h:
http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html

GCC, as any optimizing compiler might, is free to reorder instructions if there is no impact to the C Abstraction Machine (See the C Standard).

A naked CLI has no side effects as far as the compiler is concerned so the compiler may move it to a place where it does not have its intended function of disabling interrupts at the correct time.  The reasons that the compiler might choose to move the instruction are obscure and it only happens in rare cases.  Those hair-pulling caused cases of once in a blue-moon crash. :-(

Turning optimization off is NOT the correct solution, that only hides a design problem.  Also in AVR-LiBC that will actually break the delay functions that are calculated at compiler time.

Depending how the cli() macro is actually defined makes a difference to how often this 'code motion' is a concern.
A naked cli() is prone to code motion.   Marking the instruction as volatile prevents this code motion in most all cases.
The above link explains the use of 'memory barriers' in addition to volatile.

Code: [Select]

static inline void cli( void )
{
 __asm__ __volatile__ ("cli");
}

Things become even more complicated on the higher end ARM parts where ideally we need an 'execution barrier' and no such thing exists.  There are only Data Memory Barrier (DMB), Data Synchronization Barrier (DSB), Instruction Synchronization Barrier (ISB).

'volatile' and the various barriers are never meant to be used in place of proper design where multiple entities may modify a single resource, or read a resource while it is being modified.

[ulao]

bpaddock, thx! Good to know... I did give it a try to see what would happen and that seems to have done the trick... I'm really quite shocked and clearly would have been chasing this rabbet for a long time. To think you offered this suggest for reasons outside my issue... what are the chances? Or maybe you were begin casual? - Anyways, I own you a bear.

Jan, thx for the Candor, I didn't know you frequent the board here. I have always admired reading your documentary. Having the opportunity to here from you was an honer.

I finally feel like I can ask USB related question and get answers. I have had so many dead posts in my 7 years at this that I now feel cheated. I own you both! thank you so much.


-------------------------------------------------
The issue of random data is gone, I always get a full image now. but still the issue of failed attempts remains but with multiple tries it will succeed.

I need to do this
while( usbSetReport(dev, USB_HID_REPORT_TYPE_FEATURE, buffer.bytes, sizeof(buffer.data)))
      {
         fprintf(stderr,".");
      }
The driver reports error 5 quite often (Communication error with device). I though usb had a retry, maybe I need to use the check some code to do this, not sure. Though it is slowing things down a lot. I;d like to still understand why I get the USB errors.

[Jan Axelson]

Glad you got it working. Thanks to bpaddock.

[ulao]

Well not completely 

I still get the occasional error below. The usb driver does not retry, I do that in code.

URB Control Transfer failed
Device Object   USBPDO-10
Driver Object   usbhub

URB Function   URB_FUNCTION_CONTROL_TRANSFER
URB Status   USBD_STATUS_STALL_PID

Endpoint 0   Default Control

Request   Set Report
Report Type   Feature
Report ID   18
Report Length   132

I can live with the errors, I just want the usb to retry if possible.

There is a possibility its this code
   cli();
   WAIT_FOR_DATA(); STOP_WAIT();
   sei();
but the WAIT_FOR_DATA() is an ASM function that uses a pin change interrupt. So using the atomic trick will not work.  The WAIT_FOR_DATA() must stop the usb interrupts and must use 1 interrupt for the pin change.

[bpaddock]

There is a possibility its this code
   cli();
   WAIT_FOR_DATA(); STOP_WAIT();
   sei();
but the WAIT_FOR_DATA() is an ASM function that uses a pin change interrupt. So using the atomic trick will not work.

If all of the code, cli/sei et.al. is moved to ASM then 'code motion' is not an issue as it is then outside the domain of the optimizer.

The WAIT_FOR_DATA() must stop the usb interrupts and must use 1 interrupt for the pin change.

That is not clear to me.  Can the USB IRQ be disabled directly, not via CLI?  CLI will disable the pin change IRQ as well as USB IRQ.

[ulao]

That is not clear to me.  Can the USB IRQ be disabled directly, not via CLI?  CLI will disable the pin change IRQ as well as USB IRQ.

Well right, that's what I was attempting to say. the WAIT_FOR_DATA(); needs the usb IRQ off. So I use cli to do that. Then in the ASM code I use the PCINT1_vect, then shut it off. I have to do this inside 8ms. Though, during this time if the reports come in that send LCD data, the memory is getting messed up (I think).  I guess I could use the equivalent ASM for  the  ATOMIC stuff?

at least I assume that ATOKIC is preventing the PCINT1_vect.

This is my ASM code.

Code: [Select]

#include <avr/io.h>

#define __SREG__ _SFR_IO_ADDR(SREG)
#define PINx _SFR_IO_ADDR(PINC)
#define DEBUG _SFR_IO_ADDR(PORTB)
#define SCKA 5
#define SCKB 4
.text
.extern sega_buffer
.extern rejected


.global PCINT1_vect
PCINT1_vect:

sbic PINx,SCKA//if its high we are not in the lead signal.
reti //tries again after next pin change.

ldi r25,0//stop timer its good now..
sts PCICR,r25;
sts PCMSK1,r25;

sbis PINx,SCKA//find rising edge
rjmp PCINT1_vect

push r0
in r0,__SREG__

push r0 ;SREG
push r18 ;data
push r19 ;count
push r26 ;X
push r27

//set up data
ldi r26,lo8(sega_buffer)
ldi r27,hi8(sega_buffer)
sub r18,r18 ;CLC
ldi r19,4


ldi r25,179; Skip to where the check starts..
loop:
dec r25
brne loop
nop

edge:
sbis PINx,SCKB//find rising edge
rjmp edge

nextloop:
ldi r25,2; Skip to where the data starts..
loop2:
dec r25
brne loop2
nop

nextBit:
nop
nop
nop

sbic PINx,SCKB ;D7
sec
st X+,r18
clr r18
rol r18
nop
nop
nop
sbic PINx,SCKA ;D6
sec
rol r18
ldi r25,2; Skip 5
skip5:
dec r25
brne skip5
sbic PINx,SCKB ;D5
sec
rol r18
ldi r25,2; Skip 4
skip4:
dec r25
brne skip4
sbic PINx,SCKA ;D4
sec
rol r18
ldi r25,2; Skip 3
skip3:
dec r25
brne skip3
sbic PINx,SCKB ;D3
sec
rol r18
ldi r25,2; Skip 2
skip2:
dec r25
brne skip2
sbic PINx,SCKA ;D2
sec
rol r18
ldi r25,2; Skip 1
skip1:
dec r25
brne skip1
sbic PINx,SCKB ;D1
sec
rol r18

ldi r25,2; Skip 0
skip0:
dec r25
brne skip0
sbic PINx,SCKA ;D0
sec
rol r18
dec r19
brne nextBit //its at 5, now start 6,7,8,9 a bit faster. ( seem to be this way for all of my DC's )
    ldi r19,5

//asci....
//nop
//nop

analog_nextBit:
sbic PINx,SCKB ;D7
sec
st X+,r18
clr r18
rol r18
nop
nop
nop

sbic PINx,SCKA ;D6
sec
rol r18
ldi r25,2; Skip 5
analog_skip5:
dec r25
brne analog_skip5
sbic PINx,SCKB ;D5
sec
rol r18
ldi r25,2; Skip 4
analog_skip4:
dec r25
brne analog_skip4
sbic PINx,SCKA ;D4
sec
rol r18
ldi r25,2; Skip 3
analog_skip3:
dec r25
brne analog_skip3
sbic PINx,SCKB ;D3
sec
rol r18
ldi r25,2; Skip 2
analog_skip2:
dec r25
brne analog_skip2

sbic PINx,SCKA ;D2
sec
rol r18
ldi r25,2; Skip 1
analog_skip1:
dec r25
brne analog_skip1
sbic PINx,SCKB ;D1
sec
rol r18
ldi r25,2; Skip 0
analog_skip0:
dec r25
brne analog_skip0
sbic PINx,SCKA ;D0
sec
rol r18
dec r19
breq EXIT// if its a 0 exit..

//the ASCI was a bit off this helped
sbic PINx,SCKA
jmp analog_nextBit
jmp analog_nextBit
//

/* epilogue start */
EXIT:
pop r27
pop r26
pop r19
pop r18
pop r0
out __SREG__,r0
pop r0
reti

EXIT2:
ldi r25,200; empty timer
timer:
dec r25
brne timer
ldi r25,200;
timer2:
dec r25
brne timer2
ldi r25,200;
timer3:
dec r25
brne timer3

//ldi r25, 0
    //sts rejected, r25
reti



[ulao]

I didn't know you frequent the board here.

I just realized the title of the url.   

Ok ignore that last post, I didnt write that well at all. This is what I have now.

ATOMIC_BLOCK( ATOMIC_RESTORESTATE )
{
..
sei();
..// we need interrupts for a pin change so turn them on then back off.
cli();
...
}

So my issues is that the moment I turn on int's till the off I may be getting these stalls.  I need something more like,

ATOMIC_BLOCK( ATOMIC_RESTORESTATE )
{
...
}
...
ATOMIC_BLOCK( ATOMIC_RESTORESTATE )
{
...
}

because of the nested calls in my code structure I can not do that.  Is there way way to engage interrupts in the atomic block?

ATOMIC_BLOCK( ATOMIC_RESTORESTATE )
{
..
**ATOMIC_RESTORESTATE**
..// we need interrupts for a pin change so turn them on then back off.
**ATOMIC_BLOCK**
...
}

[bpaddock]

Enabling IRQs inside of the atomic block defeats its purpose.  No reason to use it.

"nested calls"

Do you mean nested IRQs?
Only advice I can give there is restructure the code so it is not required.
These tend to blow up the stack, especially if the nested IRQs share a resource.

If the code is in a tight loop anyway why not just look for the pin change rather than the IRQ of the pin change?

[ulao]

no more like the code structure.

main
{
ATOMIC_BLOCK( ATOMIC_RESTORESTATE )
{
  device->dostuff();
}
}

I do that because most all device do not need an IRQ. One on particular devices does.

dostuff code

sei(); we need interrupts for a pin change so turn them on
.. //do the code that needs the pin change
cli(); we are done now turn back off.

but I may just add some code in there to exclude that device from cli.

Thx for the help.