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.

Sign up for GitHub

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

Jump to bottom

Add support for PAW3204 Optical Sensor #17669

Merged
merged 10 commits into from
Jul 20, 2022
Merged

Add support for PAW3204 Optical Sensor #17669

merged 10 commits into from
Jul 20, 2022

Conversation

drashna
Copy link
Member

@drashna drashna commented Jul 13, 2022

Description

Adds support for the PAW3204 low powered optical sensor.

This is basic support, as the device supports multiple power modes and more, but aren't actually implemented here.

Uses bitbanged communication

Types of Changes

  • Core
  • 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).

@drashna drashna requested a review from a team July 13, 2022 23:26
KarlK90
KarlK90 reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

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

Nice! Do we have any prove that the driver works? As I don't have the sensor myself this would be good.

drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
@drashna
Copy link
Member Author

drashna commented Jul 14, 2022

Nice! Do we have any prove that the driver works? As I don't have the sensor myself this would be good.

Unfortunately, no. The code is taken from a fork. And unfortunately, I don't have actual hardware to test on.

@qmk-ci
Copy link

qmk-ci bot commented Jul 14, 2022

QMK CI Run, PR #17669

Commit SHA1: 4c0eb6e541f9fb6d7976bd414abc114f45999b38
  Base SHA1: 8ee42cd6c4bdb49604b3ae9e54a094921c8a1737

Build succeeded. See the CI output for more information.

@drashna drashna requested a review from a team July 14, 2022 19:14
KarlK90
KarlK90 requested changes Jul 14, 2022
View reviewed changes
Copy link
Member

@KarlK90 KarlK90 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

More thorough review of the code. I'm beginning to think that we should get somebody with a PAW3204 to verify that this driver works an intended. (Maybe the original author?) Pinged Gompa on Discord.

drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
@drashna
Copy link
Member Author

drashna commented Jul 15, 2022

More thorough review of the code. I'm beginning to think that we should get somebody with a PAW3204 to verify that this driver works an intended. (Maybe the original author?) Pinged Gompa on Discord.

Yeah, ideally, we should.

That said, @GEIGEIGEIST (?) was designing a PCB around this sensor, which is what kicked this off for me.

@github-actions github-actions bot removed the keyboard label Jul 15, 2022
@KarlK90
Copy link
Member

KarlK90 commented Jul 16, 2022

Gompa is away from their computer for a week, but will report back afterwards.

@GEIGEIGEIST
Copy link
Contributor

I'm back at my computer/my keyboard on Monday and will report back then.

@freznel10
Copy link

I tested on a blackpill and can confirm that it works.

@GEIGEIGEIST
Copy link
Contributor

I tested it on my PCB, running on a RP2040 Pro Micro and it works too. Thank you!

@drashna drashna requested review from KarlK90 and a team July 19, 2022 01:14
KarlK90
KarlK90 approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@KarlK90 KarlK90 left a comment

Choose a reason for hiding this comment

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

One last round of review comments (hope you don't feel nitpicked) and then this is good to go from my side.

drivers/sensors/paw3204.c Outdated Show resolved Hide resolved
drivers/sensors/paw3204.h Outdated Show resolved Hide resolved
drivers/sensors/paw3204.h Outdated
Comment on lines 39 to 45
void paw3204_init(void);
report_paw3204_t paw3204_read(void);
uint8_t paw3204_serial_read(void);
void paw3204_serial_write(uint8_t reg_addr);
uint8_t paw3204_read_reg(uint8_t reg_addr);
void paw3204_write_reg(uint8_t reg_addr, uint8_t data);
int8_t convert_twoscomp(uint8_t data);
Copy link
Member

@KarlK90 KarlK90 Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Only paw3204_init, paw3204_read, paw3204_set_cpi and paw3204_get_cpi are needed in the pointing device drivers code aka. need to be pulbic, these should get doxygen comments (like the pwm33xx_common.h stuff). The other functions don't need to be exposed on the public api and can be defined as static in paw3204.c.

@drashna
Copy link
Member Author

drashna commented Jul 19, 2022

One last round of review comments (hope you don't feel nitpicked) and then this is good to go from my side.

Nope, not at all. Helps me get into better practice :)

And applied.

@KarlK90
Copy link
Member

KarlK90 commented Jul 19, 2022

One last round of review comments (hope you don't feel nitpicked) and then this is good to go from my side.

Nope, not at all. Helps me get into better practice :)

And applied.

Good to hear! Feel free to merge.

@drashna drashna merged commit 12eb644 into qmk:develop Jul 20, 2022
@drashna drashna deleted the sensor/paw3204 branch July 20, 2022 00:46
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
@drashna @gompa
@KarlK90 @nolanseaton
Add support for PAW3204 Optical Sensor (qmk#17669)
de7f583 
Co-authored-by: gompa <gompa@h-bomb.nl>
Co-authored-by: Stefan Kerkmann <karlk90@pm.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KarlK90 KarlK90 KarlK90 approved these changes

Assignees
No one assigned
Labels
core documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@drashna @KarlK90 @GEIGEIGEIST @freznel10