Is Keil USB sample code weird?

It seems that variables are used without being set before but this is not the case.

For example let's look at the "SetConfiguration" function and the "alt" variable.

__inline BOOL USB_SetConfiguration(void)
{
  USB_COMMON_DESCRIPTOR *pD;
  DWORD alt, n, m;

        pD = (USB_COMMON_DESCRIPTOR *)USB_ConfigDescriptor;
        while (pD->bLength) {
          switch (pD->bDescriptorType) {
            case USB_CONFIGURATION_DESCRIPTOR_TYPE:
            :
            case USB_INTERFACE_DESCRIPTOR_TYPE:
              alt = ((USB_INTERFACE_DESCRIPTOR *)pD)->bAlternateSetting;
              break;
            case USB_ENDPOINT_DESCRIPTOR_TYPE:
              if (alt == 0) {
              :
              }
              break;
         :
}

The function processes a Configuration descriptor where there are always descriptors in a defined order starting from Configuration, Interface, Endpoint. This means that case "USB_INTERFACE_DESCRIPTOR_TYPE" will be always executed and sets "alt" before case "USB_ENDPOINT_DESCRIPTOR_TYPE" is executed. This is always true when a descriptor is correctly designed (errors in the descriptor which is in the usbdescr.c are not handled).

Similar is true for "USB_SetInterface" function.

So I disagree that this is not a reliable solution.

Also I don't see the problem with using:

  (BYTE *) pD += pD->bLength;

It is an effective way of implementing it in C. You can ignore the compiler warning if you know exactly want you want to achieve with it.

BTW: These functions are tested and work as expected and also pass the USB compliance test.

It seems that variables are used without being set before but this is not the case.

For example let's look at the "SetConfiguration" function and the "alt" variable.

__inline BOOL USB_SetConfiguration(void)
{
  USB_COMMON_DESCRIPTOR *pD;
  DWORD alt, n, m;

        pD = (USB_COMMON_DESCRIPTOR *)USB_ConfigDescriptor;
        while (pD->bLength) {
          switch (pD->bDescriptorType) {
            case USB_CONFIGURATION_DESCRIPTOR_TYPE:
            :
            case USB_INTERFACE_DESCRIPTOR_TYPE:
              alt = ((USB_INTERFACE_DESCRIPTOR *)pD)->bAlternateSetting;
              break;
            case USB_ENDPOINT_DESCRIPTOR_TYPE:
              if (alt == 0) {
              :
              }
              break;
         :
}

The function processes a Configuration descriptor where there are always descriptors in a defined order starting from Configuration, Interface, Endpoint. This means that case "USB_INTERFACE_DESCRIPTOR_TYPE" will be always executed and sets "alt" before case "USB_ENDPOINT_DESCRIPTOR_TYPE" is executed. This is always true when a descriptor is correctly designed (errors in the descriptor which is in the usbdescr.c are not handled).

Similar is true for "USB_SetInterface" function.

So I disagree that this is not a reliable solution.

Also I don't see the problem with using:

  (BYTE *) pD += pD->bLength;

It is an effective way of implementing it in C. You can ignore the compiler warning if you know exactly want you want to achieve with it.

BTW: These functions are tested and work as expected and also pass the USB compliance test.

I think this programming style is #*#*#*#*#*
(no admissible word at hand)

what - for example - about this single stmt taken from usbcore?

      m = (n & 0x80) ? ((1 << 16) << (n & 0x0F)) : (1 << n);

I've been using 'C' since early eighties, but in my opinion this is simply crap.
I know some find that fast, super cool and on top of smartness ... but I prefer REAL code to such ##### puzzles.

The code does look weird. But that is hardly surprising considering that they managed to cram that much of USB functionality into a little over 700 lines of code.
In my opinion, there are too few comments (only 3 of them inside function bodies.) Some references to relevent USB docs would help too. Some functions are huge, they should have been split into smaller ones. Too many magic numbers. Too much weird bit-wise arithmetic.
If the code is working, great. But I wouldn't want to change anything in it, not without buying insurance first :-)

Robert,

The function processes a Configuration descriptor where there are always descriptors in a defined order starting from Configuration, Interface, Endpoint. This means that case "USB_INTERFACE_DESCRIPTOR_TYPE" will be always executed and sets "alt" before case "USB_ENDPOINT_DESCRIPTOR_TYPE" is executed.

Ok, if case "USB_INTERFACE_DESCRIPTOR_TYPE" will be always executed first, it sets "alt", but when calling case "USB_ENDPOINT_DESCRIPTOR_TYPE" later, "alt" will be unintialized and contains a random value, as "alt" is an uninitialized auto-variable and not a static variable.

So I ask again: Do you have an explanation for this ?

(BYTE *) pD += pD->bLength;

It is an effective way of implementing it in C. You can ignore the compiler warning if you know exactly want you want to achieve with it.

I agree it is "effective", but a) I never ignore compiler warnings (and I never recommend to do so), and b) it's not ANSI C, and c) the GNU Arm-compiler used by Keil as well does not even compile it.

BTW: These functions are tested and work as expected

The do work under Windows, I agree; nevertheless I think, that sample source code spread all over the world should be portable and readable...

also pass the USB compliance test.

.. and do not work with Linux systems, as some functions of the HID driver used by Linux are not implemented :-(

Regards
Herbert

You should analyze the code a little better.

There is a while loop at the top and the whole configuration descriptor is processed in a single function call which means that "alt" is always configured before used.

"I never ignore compiler warnings [...]"

Not ignoring warnings is not the same as always obeying them and rewrite the code.

Some compilers produces warnings that are stupid. An example - a compiler that warns that (1 << 0) is a null operation. But in embedded programming, it is quite common to name processor pins as: RELAY1 = 0, RELAY2 = 1, ... and then do "output = (1 << RELAY1) | (1 << RELAY2)".

A warning is just a warning because the compiler _may_ have detected a problem in the code.

In some situations, it might be meaningful to rewrite code just to silence warnings - for example "if ((a = b) != 0)" but in some situations it is better to kill the warning than to destroy clean readable code with strange work-arounds just to make a specific compiler happy.

However, in this case someone at Keil seems to have missed that the result of a cast is _not_ an lvalue, i.e. a cast should not be used on the target of an assign. Some compilers accepts it silently. Some compilers complains that it is deprecated. Some compilers strictly follows the "not an lvalue rule" and fails to compile the code.

I use :? quite a lot, but I can't say I like this line:

 m = (n & 0x80) ? ((1 << 16) << (n & 0x0F)) : (1 << n);

Not because of any complexity, but because there is no comment describing the full value range of n. If n is an 8-bit variable, then there are 128 combinations with 0x80, but most probably the part after the ':' should only perform a shift between 0 and 15.

This is not a matter of "analyzing", because we are not in a mystery show.

Good code should talk to you ;)

Good code should talk to you ;)

So, Ulrich, you hear voices when you program ? :-)
Just kidding!

sure. you don't?

Per,

I agree on what you've said regarding "output = (1 << RELAY1) | (1 << RELAY2)"; on the other hand ignoring uninitialized variable warnings is a different story, I think.

Ulrich,

I could not say it better.

Robert,

thanks for pointing me in the correct direction, although I have to say, that "analyzing" would not be neccessary (and much easier) if the code is written in a more readable way. But I do understand that you want to defend your company.

Thanks for all of your answers!

Regards
Herbert

State machines implemented with switch often requiers dummy assigns just to keep the compiler happy. Even trivial code gets too complex for the compiler to be able to figure out.

first step for me is always to turn warnings as high as possible. Visual Studio produces some excellent hints.I wonder what Keil is going to show (this is my day 3 with Keil tools)

The examples are provided to ease the understanding of USB coding on the target chips. Then, easy-to-understand code is preferable than efficient or robust or perfect code. For this purpose, I think he has done good job as a whole, though there may be several flaws pointed out above. It's users' responsibility to make it efficient/robust/perfect for their job. The examples are just a starting point.

As of USB understanding, I have to point out this stuff.

In these examples, IN and OUT endpoint handlers are bound to a single ISR for each endpoint address, like USB_EndPoint1(). This coding gives misunderstanding to USB beginners.

On USB, IN and OUT endpoints are independent, even if they share the same address. Just on the default endpoint, IN and OUT are tightly bound by the control transfer protocol.

Tsuneo

If a good lint tool isn't an option, then it sometimes helps to see if the code can be compiled with alternative compilers since different compilers normally finds different things to complain about.

This is an ARM thread, but most code should be possible to compile with a 32-bit PC compiler (gcc, VC++, ...) if you just supply relevant header files. You will obviously not manage to link an ARM binary, but the results from compiling individual source files can be revealing. Note that some compilers requires full optimization for some warnings to be issued, i.e. when the compiler performs maximum flow analysis.

that is hardly surprising considering that they managed to cram that much of USB functionality into a little over 700 lines of code."

That is an absolutely stupid thing to do in example code - unless the purpose of the example is specifically to demonstrate "optimum" coding for minimum size.

The whole point of example code is to provide a clear, detailed, and easy-to-follow illustration of the subject at hand - the use of "clever tricks" is totally out of place!

For the same reason, example code should be extremely well-commented.

Unfortunately, a very large amount of so-called "example code" posted on the internet is extremely poor in these respects!

:-)

(quite a lot of it is also poor in that it doesn't actually work, either!)

Tsuneo,

nice to see the guru dropping by ;-)

Regarding Keil's HID sample code I would like to know how difficult it is to make it functional for Linux as well. As far as I know not answering GET_REPORT(Feature) is one of the problems causing not to be operable under Linux but there may be others as well.

Do you have hints for one being not familiar with 1000+ pages of the USB spec ?

Regards
Herbert