felis / USB_Host_Shield_2.0

Revision 2.0 of USB Host Library for Arduino.
https://chome.nerpa.tech
1.8k stars 779 forks source link

Extra Debug showing by default for for PL2303 module #111

Closed v173k closed 9 years ago

v173k commented 9 years ago

When initializing PL2303 there is some HEX code dumped to Serial Monitor at program start. See the for-loop HexDumper in cdcprolific.cpp. I believe this is debug code. Following the code to: hexdump.h there is this line with comment:

if(UsbDEBUGlvl >= 0x80) { // Fully bypass this block of code if we do not debug.

So the HEX dump happens since by default UsbDEBUGlvl = 0x80. So I can get rid of this by setting UsbDEBUGlvl to 0x7f for example in my "void setup ()" function.

But my Issue: Is this intended behaviour? Shouldn't the intended behaviour be that this debug code is turned off by default as with all other debugging info, using the following line in settings.h:

#define ENABLE_UHS_DEBUGGING 0

Also why are there two ways to turn this on/off i.e. with ENABLE_UHS_DEBUGGING and with UsbDEBUGlvl

Thank you Vivek

xxxajk commented 9 years ago

0x80 is the default debugging level for drivers. Some areas should be set to a higher level to reduce noise, but have not been done yet due to lack of time.

On Tue, Nov 18, 2014 at 9:18 PM, Vivek notifications@github.com wrote:

When initializing PL2303 there is some HEX code dumped to Serial Monitor at program start. See the for-loop HexDumper in cdcprolific.cpp. I believe this is debug code. Following the code to: hexdump.h there is this line with comment:

if(UsbDEBUGlvl >= 0x80) { // Fully bypass this block of code if we do not debug.

So the HEX dump happens since by default UsbDEBUGlvl = 0x80. So I can get rid of this by setting UsbDEBUGlvl to 0x7f for example in my "void setup ()" function.

But my Issue: Is this intended behaviour? Shouldn't the intended behaviour be that this debug code is turned off by default as with all other debugging info, using the following line in settings.h:

define ENABLE_UHS_DEBUGGING 0

Also why are there two ways to turn this on/off i.e. with ENABLE_UHS_DEBUGGING and with UsbDEBUGlvl

Thank you Vivek

— Reply to this email directly or view it on GitHub https://github.com/felis/USB_Host_Shield_2.0/issues/111.

Visit my github for awesome Arduino code @ https://github.com/xxxajk

xxxajk commented 9 years ago

As far as the on/off value, that is to save flash space for smaller mcu...

On Tue, Nov 18, 2014 at 10:01 PM, Andrew Kroll xxxajk@gmail.com wrote:

0x80 is the default debugging level for drivers. Some areas should be set to a higher level to reduce noise, but have not been done yet due to lack of time.

On Tue, Nov 18, 2014 at 9:18 PM, Vivek notifications@github.com wrote:

When initializing PL2303 there is some HEX code dumped to Serial Monitor at program start. See the for-loop HexDumper in cdcprolific.cpp. I believe this is debug code. Following the code to: hexdump.h there is this line with comment:

if(UsbDEBUGlvl >= 0x80) { // Fully bypass this block of code if we do not debug.

So the HEX dump happens since by default UsbDEBUGlvl = 0x80. So I can get rid of this by setting UsbDEBUGlvl to 0x7f for example in my "void setup ()" function.

But my Issue: Is this intended behaviour? Shouldn't the intended behaviour be that this debug code is turned off by default as with all other debugging info, using the following line in settings.h:

define ENABLE_UHS_DEBUGGING 0

Also why are there two ways to turn this on/off i.e. with ENABLE_UHS_DEBUGGING and with UsbDEBUGlvl

Thank you Vivek

— Reply to this email directly or view it on GitHub https://github.com/felis/USB_Host_Shield_2.0/issues/111.

Visit my github for awesome Arduino code @ https://github.com/xxxajk

Visit my github for awesome Arduino code @ https://github.com/xxxajk

v173k commented 9 years ago

Thanks Andrew. I realize that this is more of a question/query then an issue but I will leave this here because I feel that:

if(UsbDEBUGlvl >= 0x80) { // Fully bypass this block of code if we do not debug.

should be:

if(UsbDEBUGlvl > 0x80) { // Fully bypass this block of code if we do not debug.

(i.e. change >= to >) in hexdumper.h so that by default there is no hex dumped to Serial at program start for default debugging level (0x80).

Let me know what you think. Also whats a better forum for future questions of this sort? Where do you all look normally regarding questions/posts about the USB Host Shield?

Thanks Again!

felis commented 9 years ago

In my opinion, this is the best place for discussing the code, even though it's called "issue tracker" :-).

v173k commented 9 years ago

Here the hexdump I get (copied from Serial Monitor):

0000: 09 02 27 00 01 01 00 80 32 09 04 00 00 03 FF 00 
0010: 00 00 07 05 81 03 0A 00 01 07 05 02 02 40 00 00 
0020: 07 05 83 02 40 00 00 07058103000A010705020200400007058302004000

I will make the pull request.

xxxajk commented 9 years ago

The debugging is laid out as intended. Don't second-guess the intentions.

Consider what you would do if you wanted to incorporate debugging in your sketch... You can re-use the debugging I have designed, and use values from 0x00 to 0x7f for yourself. That is the intention, and how I use it. Once done, you can totally turn off all debugging with one define.

Don't worry yourself too much, since the next major version will do all of this in an even better way.

xxxajk commented 9 years ago

Oh, to explain a little better too...

Let's say you have an issue in a specific place in your code, and how it is interacting with the library, and it only happens in one particular place...

Basically, you can mute debugging until you get to the point of interest.

I hope that this additional information helps you in understanding how to use the debugging better.

v173k commented 9 years ago

Ok so I understood that I should indeed use #define to set the level of debugging I need. It also seems that you want there to be some debug output at the default level. Maybe this is good while the code is considered "Beta" so that if errors occur whoever is using it can report the debug output to you without doing anything special. So I think I understood what your trying to say in that respect though I think once you feel the code is mature, the default debug level should not output any debug info.

But I just felt that it seemed that there were double standards for what got outputted at the Default level. Let me see if can explain this better (of course I speak very specifically with regards to cdcprolific because thats all I have been looking through in detail):

So at default debug level (0x80) you do the hex dump as indicated in my previous post but the following lines don't get executed (see cdcprolific.cpp). They do however get executed at a higher debug level of 0x81.

USBTRACE("PL Init\r\n");
USBTRACE2("NC:", num_of_conf);
USBTRACE2("Conf:", bConfNum);

So why print the hex dump and not show the above. I would think it should be shown at a lower debug level then the hexdump if anything.

Of course this may just be a matter of opinion and I trust you guys know what would be best for the community at large. Just wanted to put it out there. I think you satisfied my question so feel free to close this and I will wait for the update you were talking about. Thanks.

xxxajk commented 9 years ago

As stated before, this was not totally completed. Right now it dumps everything, and Oleg uses the hex features in some of his projects... Different debug levels will be used in the next major release.