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

IMU ICM42688 Driver #63

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

IMU ICM42688 Driver #63

wants to merge 7 commits into from

Conversation

kelvinos2624
Copy link

@kelvinos2624 kelvinos2624 commented Oct 9, 2024

Description

What was completed, changed, or updated?
The ICM Driver (previously created, tested, and stored in https://github.com/UWARG/efs-kitchen-sinks/blob/main/icm42688.zip) has been restructured into a virtual class header file, the actual driver header file, and the driver code. Refer to this original PR for comments: #62

Why was this done (if applicable)?
This was done to leave the option for the flight controller to initiate a different imu hardware driver or run a mock imu class for unit test purposes

Testing

What manual tests were used to validate the code?
Functionality was validated with the original code on an STM board, where accelerometer and gyroscope data were successfully read from the IMU. The accuracy of accelerometer data was determined by testing for a 1g reading on each axis. The accuracy of gyroscope data was determined by temporarily implementing sensor fusion to obtain angles, from which the angles were validated.
Note: Testing for the code in this PR has not been done; only for the code in kitchensinks

What unit tests were used to validate the code?
N/A

Documentation

Milestone number and name:
M3
Link to Asana task:
https://app.asana.com/0/1203458353737758/1204645027312230
Link to Confluence documentation:
N/A

Created cpp and header files for the ICM42688. Code has been structured from the tested code found at https://github.com/UWARG/efs-kitchen-sinks/blob/main/icm42688.zip
- Implemented a constructor to take in SPI_HANDLE, GPIO PORT, and CS_PIN
- Moved some methods to private for abstraction
- Used new defines to reduce magic numbers
Altered cmakelist to support compilation
@kelvinos2624 kelvinos2624 marked this pull request as ready for review October 30, 2024 00:45
Copy link
Member

@antholuo antholuo left a comment

Choose a reason for hiding this comment

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

Looks good, only thing I have are small nits.

Maybe squash the commits into one descriptive commit message using rebase -i.

Also, try and include some screenshots of the test data you received in the PR desc, if you have any.

Drivers/imu/inc/icm42688.hpp Outdated Show resolved Hide resolved
Drivers/imu/inc/icm42688.hpp Outdated Show resolved Hide resolved
Drivers/imu/inc/icm42688.hpp Outdated Show resolved Hide resolved
Drivers/imu/inc/icm42688.hpp Outdated Show resolved Hide resolved

//Variables used in gyro calibration
float gyro_scale = 0;
uint8_t current_fssel = 0;
Copy link
Member

Choose a reason for hiding this comment

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

fs_sel?

Copy link
Author

Choose a reason for hiding this comment

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

I have renamed it in the latest commit

Drivers/imu/src/icm42688.cpp Outdated Show resolved Hide resolved
void ICM42688::calibrate() {
//Set at a lower range (more resolution since IMU not moving)
const uint8_t current_fssel = gyroFS;
setGyroFS(0x03); //Set to 250 dps
Copy link
Member

Choose a reason for hiding this comment

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

magic number 0x03. Should this be a #define or constexpr?

Copy link
Author

Choose a reason for hiding this comment

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

Ah thanks, you're right; I changed it to be a #define in the latest commit.

Drivers/imu/src/icm42688.cpp Outdated Show resolved Hide resolved
IMU Driver restructured with a virtual class header file, actual driver header file, and driver code. Implemented code to calibrate, read, and process data from a 6-axis IMU
Copy link
Contributor

@HardyYu HardyYu left a comment

Choose a reason for hiding this comment

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

Fantastic work Kelvin! I only left a minor request for change. Sorry for the late review.

Drivers/imu/inc/icm42688.hpp Outdated Show resolved Hide resolved
Modified member variable names to align with the WARG naming convention
Created cpp and header files for the ICM42688. Code has been structured from the tested code found at https://github.com/UWARG/efs-kitchen-sinks/blob/main/icm42688.zip

IMU Driver has been restructured with a virtual class header file,
actual driver header file, and driver code.
- Implemented a constructor to take in SPI_HANDLE, GPIO PORT, and CS_PIN
- Implemented code to calibrate, read, and process data from the 6-axis
  IMU
- Altered cmakelist to support compilation
Copy link
Contributor

@HardyYu HardyYu left a comment

Choose a reason for hiding this comment

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

Thank you for making the change Kelvin!

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