[Qemu-devel] [PATCH 3/3] hid.c: Add debug support

[Qemu-devel] [PATCH 3/3] hid.c: Add debug support

[Programmingkid]

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
+
 /* 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));
-- 
2.7.2

Re: [Qemu-devel] [PATCH 3/3] hid.c: Add debug support

[Alex Bennée]

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

Re: [Qemu-devel] [PATCH 3/3] hid.c: Add debug support

[Programmingkid]

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. 

Re: [Qemu-devel] [PATCH 3/3] hid.c: Add debug support

[Alex Bennée]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

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.

[-- Attachment #2: Type: text/html, Size: 2182 bytes --]

[Qemu-devel] [PATCH 0/3] Switch USB HID to QKeyCode

[John Arbuckle]

This patchset switches from the PS/2 keycode to QKeyCode support in the hid.c
file. 

John Arbuckle (3):
  usb-keys.h: initial commit
  hid.c: convert to QKeyCode
  hid.c: Add debug support

 hw/input/hid.c              | 279 ++++++++++++++++++++++++++++++--------------
 include/hw/input/usb-keys.h | 154 ++++++++++++++++++++++++
 2 files changed, 343 insertions(+), 90 deletions(-)
 create mode 100644 include/hw/input/usb-keys.h

-- 
2.5.0

[Qemu-devel] [PATCH 3/3] hid.c: Add debug support

[John Arbuckle]

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 3e1b46e..efe703e 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
+
 /* 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));
-- 
2.5.0

Re: [Qemu-devel] [PATCH 3/3] hid.c: Add debug support

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On 06/30/2016 03:32 PM, John Arbuckle wrote:
> 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 3e1b46e..efe703e 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

NACK to this approach - it is too prone to bitrot.  Instead, you want to
do something like commit ed79f37d, so that the compiler always compiles
the arguments to printf() even when debugging is disabled (any compiler
too stupid to optimize out dead 'if (0)' code is not worth using for
compiling qemu).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]