Re: [PATCH] hw/char: suppress sunmouse events with no changes

[Carl Hauser]

Yes, just equality, no masking needed. Boneheaded.

I think I could figure out how to do the state migration if that's the direction you want to go.

I don't think I could do the migration to qemu_input_handler_register, especially as I would think that the keyboard should be done at the same time.

I'll wait on a V2 patch with the equality fix for a decision about migration/vs new style.

-- Carl

On 8/20/24 00:34, Richard Henderson wrote:

> On
>       8/20/24 09:18, Carl Hauser wrote:
>       
> > @@ -959,6 +960,15 @@ static void
> >         sunmouse_event(void *opaque,
> >         
> >               int ch;
> >         
> >         
> >               trace_escc_sunmouse_event(dx, dy, buttons_state);
> >         
> >         +
> >         
> >         +    /* Don't send duplicate events without motion */
> >         
> >         +    if (dx == 0 &&
> >         
> >         +        dy == 0 &&
> >         
> >         +        (s->sunmouse_prev_state ^ buttons_state) == 0) {
> >         
> >       
>       
>       Were you intending to mask vs MOUSE_EVENT_*BUTTON?
>       
>       Otherwise this is just plain equality.
>       
>       
> > diff --git a/include/hw/char/escc.h
> >         b/include/hw/char/escc.h
> >         
> >         index 5669a5b811..bc5ba4f564 100644
> >         
> >         --- a/include/hw/char/escc.h
> >         
> >         +++ b/include/hw/char/escc.h
> >         
> >         @@ -46,6 +46,7 @@ typedef struct ESCCChannelState {
> >         
> >               uint8_t rx, tx;
> >         
> >               QemuInputHandlerState *hs;
> >         
> >               char *sunkbd_layout;
> >         
> >         +    int sunmouse_prev_state;
> >         
> >       
>       
>       This adds new state that must be migrated.
>       
>       
>       While the patch is relatively simple, I do wonder if this code
>       could be improved by converting away from the legacy mouse
>       interface to qemu_input_handler_register. Especially if that might
>       help avoid needing to add migration state that isn't "really" part
>       of the device.
>       
>       
>       Mark?
>       
>       
>       
>       r~
>       
>       
>