Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor hid.py #1068

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Refactor hid.py #1068

merged 2 commits into from
Jan 8, 2025

Conversation

xs5871
Copy link
Collaborator

@xs5871 xs5871 commented Jan 8, 2025

This is a big one. It started out as smaller atomic commits, but the recent #1061 made a clean rebase a bad decision from the perspective of time and effort that would have been spent on it.

Summary:

  • Small memory and performance improvements, mostly const() and bytearray ops.
  • Separate out report specific code into report classes. Serves as a base for easier addition of custom endpoints in "the future".
  • Lots of small integration changes, for example a simpler mechanism to record reports for unit tests (now with potential to verify other reports than 6KRO).

* Small memory and performance improvements, mostly const() and bytearray
  ops.
* Separate out report specific code into report classes. Serves as a base
  for easier addition of custom endpoints in "the future".
* Lots of small integration changes, for example a simpler mechanism to
  record reports for unit tests (now with potential to verify other
  reports than 6KRO).
Copy link
Member

@regicidalplutophage regicidalplutophage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else looks good

kmk/hid.py Outdated
# active at any time.
memoryview(self._cc_report)[1:3] = cc.code.to_bytes(2, 'little')
self._cc_pending = True
class HSPointingDeviceReport(Report):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this inherit from PointingDeviceReport?

class HSPointingDeviceReport(PointingDeviceReport):

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct.
I found even more bits and pieces that escaped the rebase. Also, you're now partially responsible for #1069.

@regicidalplutophage regicidalplutophage self-requested a review January 8, 2025 21:12
Copy link
Member

@regicidalplutophage regicidalplutophage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge unless you find something else

@xs5871
Copy link
Collaborator Author

xs5871 commented Jan 8, 2025

Tested all the endpoint configurations again, pretty sure it's complete now. If not, I probably have a fix for it somewhere already.

@xs5871 xs5871 merged commit 0b26d06 into main Jan 8, 2025
2 checks passed
@xs5871 xs5871 deleted the refactor-hid branch January 8, 2025 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants