USB IN endpoint buffersize

When opening an IN endpoint if the buffersize < endpoint_max_packetsize+2 then the endpoint doesn’t work.

 2554:     _kernel_swi_regs r;
 2555:     r.r[0] = BM_FreeSpace;
 2556:     r.r[1] = str->buffer_id;
 2557:     CallBufMan(&r);
 2558: #endif
 2559:     int actlen = r.r[2];
 2560:     int maxpacket = UGETW(str->pipe->endpoint->edesc->wMaxPacketSize);
 2561:     if (actlen > maxpacket)
 2562:     {

where actlen=free space in the buffer. So at this point the buffer free space needs to be > maxpacket.

The other byte comes from a fault with the buffermanager.

DeviceFS.s.FSystem makes its own buffer in make_buffer, using buffermanager, using the requested size and the threshold value returned from usbmodule.

In USBmodule create_buffer usbd creates its own buffer with the size requested and returns a threshold value

 2771:     }
 2772:     *thresh = *size - UGETW(str->pipe->endpoint->edesc->wMaxPacketSize);
 2773:     return e;

This loss of 1 byte together with the >maxpacket required in usbmodule means that you need to specify at least maxpacket+2 bytes.

I’d like to fix this so that the buffermodule makes a buffer the requested size and usbmodule will work with maxpacket buffer size.

I’m posting this here because there’s no point in me doing it if the change won’t be accepted.

I’d like to submit these changes. These fixes fix a couple of problems with setting options in the USB special field string. There are 2 modified files

castle.RiscOS.Sources.HWSupport.Buffers.s.Buffers

This changes the Buffer module used by DeviceFS so that it uses all of the buffer size specified when registering or creating a buffer. The existing version creates a buffer 1 byte less that that requested. All I’ve done us use a separate variable for freespace size instead of using the difference between the insert and remove pointers.

mixed.RiscOS.Sources.HWSupport.USB.NetBSD.build.c.usbmodule

This fixes start_read so that it will fetch if freespace in buffer is >= an endpoint maxpacketsize. At the moment it won’t fetch if endpoint maxpacketsize = freespace in buffer.

The changes are tagged COLIN_BUFFER_SIZE_FIX in both files. I’ve kept the original next to the changes so you can see the change.

This change will mean that a special field entry for an endpoint of size will work as expected ie usb will see a maxpacketsize buffer.

Also in usbmodule tagged COLIN_ALTERNATE_FIX I’ve fixed the following.

At present if endpoint and any or both of interface and alternate is specified in the special field then interface and alternate are ignored in favour of the interface used by the found endpoint in the configuration descriptor and alternate is set to 0.

Additionally if the endpoint_type is not specified it is set to bulk which may not be correct for the endpoint and can be determined from the endpoint descriptor.

So I’ve changed things so that if just endpoint is specified endpoint_type, interface and alternate are set from the configuration descriptor.

The change also results in an error if a specified endpoint/interface/ alternate combination isn’t found. At the moment if endpoint exists the other options are ignored but it may be that the endpoint number is in error.

With these changes you can

OPENIN “devicefs#endpoint1;size8:USB1”

and get the correct endpoint_type and an 8 byte buffersize – behaviour
if you don’t specify the endpoint is as it was before ie its set to the
first bulk IN endpoint.

I’ve included a the changed files in USBBufferFix.zip. It includes softload of RISC OS for an Iyonix if you want to try it. For those who haven’t got the latest riscos and are playing with USB, all special field strings work correctly in this version eg nopad and noblock and order doesn’t matter. I’d be interested if anyone tried the changes.

I’ve only tested the changes on an Iyonix.

This changes the Buffer module used by DeviceFS so that it uses all of the buffer size specified when registering or creating a buffer. The existing version creates a buffer 1 byte less that that requested.

I don’t think that’s the right place to make the change – it’s supposed to do that. See PRM4-88 which says “This call claims an area of memory from the RMA and registers it as a buffer. If you
register a buffer n bytes long, it can hold at most n – 1 bytes.”

So it’s the caller’s responsibility to add 1.

I’m not particularly a USB expert, so I’m not sure I can say whether the COLIN_ALTERNATE_FIX is sensible or not – do I remember right that someone had a USB analyser on their desk? Perhaps they can peer review that change for goodness.

I should add that in my view you deserve some of the USB bounty money. With the stated goal of “Update and debug USB stack” you’re taking a methodical approach to analysing the various sticking points and making incremental patches which solve them. This is exactly how I read “Update and debug USB stack” – the update bit being adopting the latest NetBSD sources and the debug bit being what you’re doing, rather than reimplementing the whole thing in Pascal, or whatever.

Re buffers. OK I can move it to usbmodule. The manual isn’t right for the Buffer module if the wordAligned flag is set (not documented in the copy of the manual I have) as in that case you lose 4 bytes – I vaguely remember the aligned buffer not being right somewhere – didn’t take too much notice as I was replacing the code. I’ll look into it. I think buffer size is only going to be critical in USB and only then for setting the buffersize the same as maxpacketsize. I may have fixed everyones buffers if they didn’t read the small print :-)

OK I admit it I found it annoying that the Buffer module lost 1/4 bytes :-) especially as it was easier to use it all.

Re ALTERNATE_FIX. I have a logitech webcam here and it has 3 interfaces 1 interface has 7 alternate interfaces (presumably the video interface) each alternate interface has endpoints 1 and 2 – one isochronous and one interrupt.

If I OPENIN "devices#endpoint1;interface0;alternate7:USB3 the current system will set endpoint=1 endpoint_type=bulk interface=0 alternate=0. There’s nothing you need to know about USB for the fix it just ensures that you get the setup you specify in the special field.

re bounty. Not bothered about the money. Just feel the NETBSD code is getting a bad press and if the RISC OS end is sorted then at least we can be more confident where the problem lies. Besides the USB sources are a good bedtime read – certainly send me to sleep.

DeviceFS_Register PRM2-436
The objects in the device list has entries for default sizes of rx and tx buffers – no mention of the -1 adjustment

DeviceDriver_Entry 6 and 7 (Create buffer for tx/rx) PRM3-612 has suggested size on entry and actual size on exit and a buffer the returned size is created. There is no mention that you will only get size-1 bytes.

So the bug is in devicefs not allocating the requested size

I would also point out that for the buffer swis the PRM’S description of the register
entry states buffer size. It’s only the footnote which contradicts this.

Regarding the Buffer manager the b_WordAligned flag is only half heartedly implemented.
For example insertblock wraps insertindex when it reaches size-4 but removeblock wraps
removindex on size-1.

My version will handle Aligned buffers trivially ie you don’t have to do anything.

If you still want buffers to use<the requested size my version would just need size adjusted in buffer_register to be size-1 or size-4.

Any preferences?

DeviceFS_Register […] DeviceDriver_Entry no mention of the -1 adjustment […] So the bug is in devicefs not allocating the requested size

I think I agree having looked at the PRM, the DeviceFS API is buffer-size-as-expected. DeviceFS currently uses BufferManager as its underlying memory handler (but doesn’t have to, I suppose) so it should be making the adjustment of 1 extra byte (or 1 extra word for the word aligned case).

Regarding the Buffer manager the b_WordAligned flag is only half heartedly implemented

It does look like it was a rushed in, presumably to fix some project crisis, the CVS check in message doesn’t explain why it was added and several subsequent engineers have tried to shore it up.

If there are fixes to the word aligned case, might as well have those too, but I don’t think they affect your main area of concentration on USB since it’s not setting the word aligned flag.

Ok. I’ve changed it so that the buffer size change occurs in DeviceFS instead of Buffers This version bufszDevfs.zip includes the usbmodule and FSystem fixes – see ReadMe file.

I’ve also included a version of the Buffer file you already have which adjusts the buffer size in Buffer_Register so that it is compatible with existing documentation.

In the absence of someone coming forward about the ALTERNATE FIX I’ll give you a crash USB course.

When programming a normal serial port your interface to it is fully described by the list of registers that are memory mapped into the ARM memory space. So you read and write memory to program the device.

USB is not like that, USB’s equivalent of registers for a device are the leaves in a descriptor tree – I’ll come to what the terms mean later.

Each leaf is fully described in ADFS terms as

Device.Configuration.Interface.leaf

Each node in the tree has properties and this list of properties is known as a Descriptor. So a Device Descriptor defines the device level properties, Configuration Descriptor the Configuration level properties and so on.

An endpoint is one type of leaf. It can be thought of as a file except that you can only read from it if it has an IN property or write to it if it has an OUT property.

When a device is plugged in the Configuration is read from the device and this is returned as a block of memory containing all of the Descriptors in the tree from the Configuration Descriptors down to and including the leaves. A typical tree may be

Configuration 
   interface0
     endpoint1
     endpoint2
   interface1
     endpoint3
     endpoint4

Configuration
   interface0 Alternate 0
     endpoint1
     endpoint2
   interface0 Alternate 1
     endpoint1
     endpoint2
   interface1
     endpoint3
     endpoint4

Note 1. The descriptors are not word aligned and in C terms are packed. C structures for the descriptors can be found here (build.h.USBDevFS).
Note 2. Although the returned block is enumerated as a linked list it does describe a tree.
Note 3. The unusual – for RISC OS – naming of the C structure variables is so that the names used correspond to the names used in the USB Standard Specification.

So if we goto “1987: static void find_interface_and_endpoint” in usbmodule on entry alternate_return has been initialised to 0. The function traverses the configuration tree and is basically validating the special string entries (valid→).

If the special string set for the above device includes:

“endpoint1;interface0;alternate1” then because valid→endpoint != &deaddead ie it was specified then only the true clause of the if/else structure at line 2029 is ever used and so when found my specified alternate1 is ignored so I don’t open the endpoint I want.

Also if the special string doesn’t include the endpoint type (bulk,interrupt,isochronous,control) then it defaults to bulk when the endpoint specifies its own type in the endpoint descriptor.

Hope this helps – I tried :-)

In the absence of someone coming forward about the ALTERNATE FIX I’ll give you a crash USB course…Hope this helps

As it was 11pm last night (only one eye open, back half of brain asleep) I just did the buffer changes yesterday, but hope to come back to the alternate stuff soon. I do/did know a fair bit about USB, at least at the hardware level.

While you might be currently well versed (in topic X) you’re effectively asking the reviewer to empty something else out of their head to make space for (topic X) something else, in the context of other things they’re trying to concentrate on (topic Y).

There seem to be quite a few people doing application level work on USB (midi and cameras and things) who could share the joy of review over in the other thread.

Sorry. I inferred from what you wrote earlier that you didn’t know much.