Hi,
I'm just implementing the Keil HID sample code, but I found some strange things even the compiler complains about:
In usbcore.c there are some variables used without being set before. The variables are "alt" in USB_SetConfiguration() and "msk", "old", "alt" and "ifn" in USB_SetInterface().
__inline BOOL USB_SetConfiguration(void) { USB_COMMON_DESCRIPTOR *pD; DWORD alt, n, m; : case USB_INTERFACE_DESCRIPTOR_TYPE: alt = ((USB_INTERFACE_DESCRIPTOR *) pD)->bAlternateSetting; break; case USB_ENDPOINT_DESCRIPTOR_TYPE: if (alt == 0) { : } : }
The code at USB_ENDPOINT_DESCRIPTOR_TYPE is executed when "alt" is 0 by chance only and setting "alt" in USB_INTERFACE_DESCRIPTOR_TYPE has no effect as "alt" is an auto-variable loosing it's contents after USB_SetConfiguration() is finished.
USB_SetInterface() shows similar things.
The source code does work in my environment, but I'm anxious that this is by chance only.
Could anybody explain me how to come to a more reliable solution?
BTW: Code parts like
(BYTE *) pD += pD->bLength;
in a sample code scare me even after more than 20 years C programming (Realview complains as well).
Regards Herbert
"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.
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!
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.
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 ?
Hello Herbert,
"Regarding Keil's HID sample code I would like to know how difficult it is to make it functional for Linux as well."
USB is a standard protocol. As long as you follow the USB spec, your device works on any OS (, in principle :-) ). Surely, Windows give us some (many?) exceptions, but Linux doesn't have so much exception. I don't see any problem on the HID example which disturbs working on Linux.
"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."
The example declares no feature report. Then, even if you see STALL for GET_REPORT(Feature), it is the expected result.
- Declare a feature report in the report descriptor - Modify HID_GetReport() handler
hiduser.c /* * HID Get Report Request Callback * Called automatically on HID Get Report Request * Parameters: None (global SetupPacket and EP0Buf) * Return Value: TRUE - Success, FALSE - Error */ BOOL HID_GetReport (void) { /* ReportID = SetupPacket.wValue.WB.L; */ switch (SetupPacket.wValue.WB.H) { case HID_REPORT_INPUT: GetInReport(); EP0Buf[0] = InReport; break; case HID_REPORT_OUTPUT: return (FALSE); /* Not Supported */ case HID_REPORT_FEATURE: /* EP0Buf[] = ...; */ /* break; */ return (FALSE); /* Not Supported */ } return (TRUE); }