keyboardio / KeyboardioHID

A HID library for Arduino
MIT License
34 stars 17 forks source link

Mouse: Pull sendReport out of move & buttons #14

Closed algernon closed 6 years ago

algernon commented 6 years ago

Instead of sending a report immediately after a move or a button press, let the user of the library decide when to send it. While there, also expose a releaseAll method, so we follow the same pattern as Keyboard, allowing the user to send & clear the report each cycle.

This separation is required to address keyboardio/Kaleidoscope-MouseKeys#10.

obra commented 6 years ago

If we're going to do this, we need to copy the pattern that prevents duplicate reports from being sent from MultiReport/keyboard.cpp.

algernon commented 6 years ago

Do we need that for ConsumerControl as well?

obra commented 6 years ago

Yes.

On Mon, Nov 27, 2017 at 3:48 PM, Gergely Nagy notifications@github.com wrote:

Do we need that for ConsumerControl as well?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/keyboardio/KeyboardioHID/pull/14#issuecomment-347367541, or mute the thread https://github.com/notifications/unsubscribe-auth/AACxaCd5wv88s4uK5X9ElrYiSfXQSydyks5s60pZgaJpZM4QqNvH .

algernon commented 6 years ago

Haven't tested this theory yet, but... what happens if I have acceleration turned off, and keep holding the Mouse Left key? If we don't send a report that says otherwise, will the host keep moving the mouse left at the same speed?

(will try & answer this myself tonight, just logging it here; rebased the PR on top of master for the time being)

algernon commented 6 years ago

Rebased on top of master (fixing the conflict), and added a guard to sendReport.

algernon commented 6 years ago

Just tested the code with the guard - its no good. With the guard, if I keep holding the mouse key, it will stop moving as soon as the report remains identical. With the guard dropped, it all works. So in the case of mouse keys, we can't cheat and send only when there's a diff.

obra commented 6 years ago

I believe we need the guard if the report is empty and unchanged, otherwise, we do really spam the bus.

algernon commented 6 years ago

Aha! That makes more sense. Adjusting & giving it a try in a bit.

algernon commented 6 years ago

PR updated, now the guard also checks for emptiness too. Shows signs of working =)

obra commented 6 years ago

At this point, do you think it's ready for merge, @algernon?

algernon commented 6 years ago

There's one simplification I'll try today: stop comparing reports, and skip it if it is empty. That'll save us a few bytes of RAM & PROGMEM, and should have the same effect (and one less report sent).

obra commented 6 years ago

Well, you want to send the report if it's empty but the last one wasn't empty. Otherwise, I expect you'll get weird button state issues.

algernon commented 6 years ago

While skipping the empty report under Linux works, I agree that it is not the safest assumption to make that it'll work elsewhere. So the comparison to an empty state will remain.

As per our discussion on IRC, I'll simplify the empty report check: we'll compare against an empty report.

algernon commented 6 years ago

Everything's up to date now, ready to merge, I believe.