Fix mouse report comparison failing on shared EP (fixes KB preventing sleep)

[drzony]

Reset report ID that gets changed when sending mouse report to host.

Description

Properly reset the in-memory mouse report, so that it's not sent all the time. Current behavior prevents power saving on host.

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[drzony]

@drashna Do you mind checking this out?

[drashna]

As far as I'm aware, this shouldn't be needed.
https://github.com/drashna/qmk_firmware/blob/8e9ee29fe352b98ee1380213fc2e2b0e945cdf3f/tmk_core/protocol/host.c#L95

So the report ID that is sent shouldn't matter, as it's immediately set.

Also, I've been using the mouse shared EP for a while and have no issues with sleeping.

[drzony]

The problem appears on split keyboard, when the pointing device is on slave. local_mouse_report is replaced by shared_mouse_report. If the pointing device is on master, then always the same local_mouse_report is used, so this bug does not present itself.

[drashna]

The thing is I am using a split keyboard and master is left, device is right.

However, are you using AVR or ARM here?

And it may be better to just zero out the reports on init here?

[drzony]

I'm on AVR. According to the comment the buttons must remain. The problem here is that host_mouse_send modifies the report_id field, then local_mouse_report is copied to old_report. When coming from slave side shared_mouse_report has report_id = 0 so has_mouse_report_changed always reports true.
See:

qmk_firmware/quantum/pointing_device.c

Line 242 in e44b624

local_mouse_report = POINTING_DEVICE_THIS_SIDE ? pointing_device_driver.get_report(local_mouse_report) : shared_mouse_report;

[drzony]

Changed to zeroing the whole report, preserving buttons.

[drashna]

I'm on AVR. According to the comment the buttons must remain. The problem here is that host_mouse_send modifies the report_id field, then local_mouse_report is copied to old_report. When coming from slave side shared_mouse_report has report_id = 0 so has_mouse_report_changed always reports true. See:

qmk_firmware/quantum/pointing_device.c

Line 242 in e44b624

local_mouse_report = POINTING_DEVICE_THIS_SIDE ? pointing_device_driver.get_report(local_mouse_report) : shared_mouse_report;

Aah, okay.

Might be worth:

    local_mouse_report = POINTING_DEVICE_THIS_SIDE ? pointing_device_driver.get_report(local_mouse_report) : shared_mouse_report; 
#ifdef MOUSE_SHARED_EP
    local_mouse_report.report_id = REPORT_ID_MOUSE;
#endif

[drzony]

I would need to add it to all paths, so a little bit lower. However it seems a bit more natural to me to zero it in the place that was already setting other members to 0. (Probably it was also broken on extended reports since it did not zero extended members)
pointing_device_send seems to be the place where all paths converge and it's responsible for checking if the report changed, so it looks as a better place to put resetting of local_mouse_report.

[dkao]

Should shared_mouse_report also be zeroed out here? Both sides update pointing device at generally the same rate due to the throttle, but there is no explicit synchronization for this AFAICT so shared_mouse_report potentially contains stale information.

[drzony]

Added

[dkao]

Thanks for this!
Looking at the problem I feel like host_send_mouse() should either take input by value, or make a copy of report and pass on the modified copy instead of modifying input. Same probably goes for host_send_keyboard().
Thoughts on this? struct copy too wasteful?

[drzony]

I think we should merge it as is, since preventing sleep is a major bug. Then we can discuss any further refactoring. Maybe create an issue with your ideas and we can pick it up from there.

[drzony]

@drashna I think the final solution would be to rewrite the code @dkao mentioned to avoid copy and probably some other minor improvements here and there. For now I would merge this PR to at least solve the sleep problem.

[drashna]

Yeah, I agree.