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

pmw3360 trackball and h scroll #793

Conversation

peterwhitesell
Copy link

@peterwhitesell peterwhitesell commented May 17, 2023

Changes

  • add pmw3360 module to support pmw3360 motion sensor (thanks @bullwinkle3000 and @kbjunky!)
  • add backward compatible support for horizontal scrolling via an optional custom HID device
  • add ability to disable debug logs per module

@xs5871
Copy link
Collaborator

xs5871 commented May 17, 2023

So, first of all: thank you for contributing.
But: those are all different topics and I would like to have independent PRs for them, otherwise the reviews get topically entangled and all complicated.
There's also #796, which implements panning with mechanisms we have already in place that make it a bit more user-friendly, plus actual documentation.

@regicidalplutophage
Copy link
Member

I'd love to see pmw3360 support in master, but we definitely need to leverage the mechanism of loading custom descriptors through bootconfig. There's a descriptor for NKRO keyboard, pointing device, maybe gamepad in the future and who knows what else would be possible. All these need to be structured.

@peterwhitesell
Copy link
Author

So, first of all: thank you for contributing. But: those are all different topics and I would like to have independent PRs for them, otherwise the reviews get topically entangled and all complicated. There's also #796, which implements panning with mechanisms we have already in place that make it a bit more user-friendly, plus actual documentation.

hey @xs5871 thanks for taking a look! I hear you about separate PRs. I overlooked the bootcfg mechanism, I'll have to look into that, and/or perhaps just retire that part of this pr in favor of #796. I can pull out pmw3360 modules and the debug log disabling into separate prs, unless you've got any major course correction on those before that?

Thanks again

@peterwhitesell
Copy link
Author

I opened #797 for just pmw3360

@xs5871
Copy link
Collaborator

xs5871 commented May 17, 2023

Here's my opinion on the log disabling:
Any log output committed to code should firstly help debug user issues. For development, you can put additional temporary debug output wherever you need. That means every debug output in the code should be warranted. If you have to switch something off to make the logs parsable, then that something is noise and should be either refactored, or removed.
We had really spammy logs in the past and at that time I would have agreed with you, but have since changed my mind. More granular debug control was a thing we thought about, but I think we've progressed far enough towards actual helpfull logs, that we don't need it any more, and probably should even avoid it. Not only for the reduced code complexity, but also because it forces us to think twice about putting in hello-world messages every 4 lines.

@peterwhitesell
Copy link
Author

fair enough. we're coming from different logging philosophies. I'd rather have lots of logs and a way to filter them over having to go back and add logs whenever I want to debug something, but I see your perspective. I'll leave that out and close this PR now, as the other two changes are superseded by other PRs. Cheers!

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.

3 participants