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

qmi8658c: Add support for the QMI8658C sensor #467

Merged
merged 3 commits into from
Oct 2, 2022

Conversation

rcbadiale
Copy link
Contributor

Add initial support for the QMI8658C Accelerometer and Gyroscope sensor by QST Solution.

This sensor is used in the WaveShare RP2040 Round LCD 1.28in development board.

Add an example based on the aforementioned development board.

Add smoke test to makefile and update README.md

Copy link
Member

@conejoninja conejoninja left a comment

Choose a reason for hiding this comment

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

This number needs to be updated too.
I think somewhere in the process lost count, with your driver it should be 89 devices on the list, but please count them again just in case I'm wrong.

}
}

// Check if the usar has defined a desired configuration, if not uses the
Copy link
Member

Choose a reason for hiding this comment

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

usar* typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the typo, thanks for noticing.

}

// Read the temperature from the sensor, the values returned are in Celsius.
func (d *Device) ReadTemperature() (t float32) {
Copy link
Member

Choose a reason for hiding this comment

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

Please, follow the signature ReadTemperature() (int32, error) { as other drivers do. It should return millidegrees:

https://github.com/tinygo-org/drivers/blob/release/bme280/bme280.go#L116

As a rule of thumb, avoid float when possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the signature as proposed and updated the example file.

My main language is Python and I'm new to Go and TinyGo, so I have to ask, this would be for memory optimization? Since float32 would use a lot more memory than int32?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know the details, but yes, the boards targeted don't have much memory and don't play well with floats in general.

@rcbadiale
Copy link
Contributor Author

Apparently they are 90 devices, if I can count correctly.

All changes were made, hope it's alright now.

Thank you very much for the review.

@conejoninja
Copy link
Member

conejoninja commented Oct 2, 2022

Thanks for your contribution, it's a very well written driver.
Now merged

@conejoninja conejoninja merged commit c00cb3a into tinygo-org:dev Oct 2, 2022
deadprogram pushed a commit that referenced this pull request Dec 23, 2022
* Add support for the QMI8658C sensor

* qmi8656c: update ReadTemperature signature

* Update README devices count
roman-dvorak pushed a commit to roman-dvorak/tinygo-drivers that referenced this pull request Feb 17, 2023
* Add support for the QMI8658C sensor

* qmi8656c: update ReadTemperature signature

* Update README devices count
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