KMKfw / kmk_firmware

Clackety Keyboards Powered by Python
https://kmkfw.zulipchat.com
Other
1.38k stars 474 forks source link

pmw3360 trackball and h scroll #793

Closed peterwhitesell closed 1 year ago

peterwhitesell commented 1 year ago

Changes

xs5871 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I opened https://github.com/KMKfw/kmk_firmware/pull/797 for just pmw3360

xs5871 commented 1 year ago

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 commented 1 year ago

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!