[3/3] hid.c: Add debug support

Add debug macros to the code for easier debugging.

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
---
 hw/input/hid.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Programmingkid <programmingkidx@gmail.com> writes:

> Add debug macros to the code for easier debugging.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> ---
>  hw/input/hid.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/input/hid.c b/hw/input/hid.c
> index 329a27b..42ca592 100644
> --- a/hw/input/hid.c
> +++ b/hw/input/hid.c
> @@ -37,6 +37,13 @@
>  #define RELEASED -1
>  #define PUSHED -2
>
> +/* #define DEBUG_HID_CODE */
> +#ifdef DEBUG_HID_CODE
> +    #define DEBUG_HID(fmt, ...) printf(fmt, __VA_ARGS__)
> +#else
> +    #define DEBUG_HID(fmt, ...) (void)0
> +#endif
> +

This style of debug setup is discouraged these days as its prone to
bitrot. It's better to define like this:

#define DEBUG_HID(fmt, ...) \
  if (DEBUG_HID_CODE) {     \
     printf(fmt, __VA_ARGS);\
  }

This means you get the benefit of the compiler checking your format
strings even if the code gets optimised away when DEBUG_HID_CODE isn't
defined.

>  /* Translates a QKeyCode to USB HID value */
>  static const uint8_t qcode_to_usb_hid[] = {
>      [Q_KEY_CODE_SHIFT] = USB_HID_LEFT_SHIFT,
> @@ -331,6 +338,7 @@ static void hid_keyboard_event(DeviceState *dev, QemuConsole *src,
>          return;
>      }
>      keycode = qcode_to_usb_hid[qcode];
> +    DEBUG_HID("keycode = 0x%x qcode:%d\n", keycode, qcode);
>
>      count = 2;
>      if (evt->u.key.data->down == false) { /* if key up event */
> @@ -381,6 +389,9 @@ static void hid_keyboard_process_keycode(HIDState *hs)
>      slot = hs->head & QUEUE_MASK; QUEUE_INCR(hs->head); hs->n--;
>      keycode = hs->kbd.keycodes[slot];
>
> +    DEBUG_HID("keycode:0x%x status:%s\n", keycode, (status == PUSHED ? "Pushed"
> +              : "Released"));
> +
>      /* handle Control, Option, GUI/Windows/Command, and Shift keys */
>      if (keycode >= 0xe0) {
>          process_modifier_key(status, keycode, &(hs->kbd.modifiers));


--
Alex Bennée

On Mar 26, 2016, at 2:48 AM, Alex Bennée wrote:

> 
> Programmingkid <programmingkidx@gmail.com> writes:
> 
>> Add debug macros to the code for easier debugging.
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> ---
>> hw/input/hid.c | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>> 
>> diff --git a/hw/input/hid.c b/hw/input/hid.c
>> index 329a27b..42ca592 100644
>> --- a/hw/input/hid.c
>> +++ b/hw/input/hid.c
>> @@ -37,6 +37,13 @@
>> #define RELEASED -1
>> #define PUSHED -2
>> 
>> +/* #define DEBUG_HID_CODE */
>> +#ifdef DEBUG_HID_CODE
>> +    #define DEBUG_HID(fmt, ...) printf(fmt, __VA_ARGS__)
>> +#else
>> +    #define DEBUG_HID(fmt, ...) (void)0
>> +#endif
>> +
> 
> This style of debug setup is discouraged these days as its prone to
> bitrot. It's better to define like this:
> 
> #define DEBUG_HID(fmt, ...) \
>  if (DEBUG_HID_CODE) {     \
>     printf(fmt, __VA_ARGS);\
>  }
> 
> This means you get the benefit of the compiler checking your format
> strings even if the code gets optimised away when DEBUG_HID_CODE isn't
> defined.

I don't like if conditions in the debug macro because they do take up cpu time. The (void)0 is just zero. I don't think it would take any cpu time away from the program.

It won't. The compiler will dead code away any branches where the test
evaluates to a constant.
On 27 Mar 2016 3:30 p.m., "Programmingkid" <programmingkidx@gmail.com>
wrote:

>
> On Mar 26, 2016, at 2:48 AM, Alex Bennée wrote:
>
> >
> > Programmingkid <programmingkidx@gmail.com> writes:
> >
> >> Add debug macros to the code for easier debugging.
> >>
> >> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> >> ---
> >> hw/input/hid.c | 11 +++++++++++
> >> 1 file changed, 11 insertions(+)
> >>
> >> diff --git a/hw/input/hid.c b/hw/input/hid.c
> >> index 329a27b..42ca592 100644
> >> --- a/hw/input/hid.c
> >> +++ b/hw/input/hid.c
> >> @@ -37,6 +37,13 @@
> >> #define RELEASED -1
> >> #define PUSHED -2
> >>
> >> +/* #define DEBUG_HID_CODE */
> >> +#ifdef DEBUG_HID_CODE
> >> +    #define DEBUG_HID(fmt, ...) printf(fmt, __VA_ARGS__)
> >> +#else
> >> +    #define DEBUG_HID(fmt, ...) (void)0
> >> +#endif
> >> +
> >
> > This style of debug setup is discouraged these days as its prone to
> > bitrot. It's better to define like this:
> >
> > #define DEBUG_HID(fmt, ...) \
> >  if (DEBUG_HID_CODE) {     \
> >     printf(fmt, __VA_ARGS);\
> >  }
> >
> > This means you get the benefit of the compiler checking your format
> > strings even if the code gets optimised away when DEBUG_HID_CODE isn't
> > defined.
>
> I don't like if conditions in the debug macro because they do take up cpu
> time. The (void)0 is just zero. I don't think it would take any cpu time
> away from the program.