IMU Driver for ICM42688 #62
Conversation
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
kelvinos2624
requested review from
antholuo,
DerekTang04,
pollyliu1 and
HardyYu
| // icm42688.cpp | ||
|
|
||
| #include "icm42688.hpp" | ||
| #include "stm32l5xx_hal_conf" |
There was a problem hiding this comment.
this line looks weird, it doesn't have a file extension
There was a problem hiding this comment.
Initially i didn't have a file extension because my intellisense was giving me errors when I included a .h at the end and I wasn't able to find out why. Are these squiggles something that I can ignore if I do include the file extension?
There was a problem hiding this comment.
Are you on Linux? Try building on linux, that might tell you where the error is.
There was a problem hiding this comment.
I also just went through the build - I can't find any reference to "icm" or "hal_conf", are we sure your files are included properly in cmakelists.txt and are being built?
| * | ||
| * @return none | ||
| */ | ||
| void readRegister(uint8_t sub_address, uint8_t count, uint8_t * dest); |
There was a problem hiding this comment.
Ideally, we keep all the methods below private. Since the three methods above are the methods that users would call/interface, the functions below are those that help facilitate imu reading, not something that users want to interact with directly. We should keep them private and hide them.
|
|
||
| class ICM42688 : public IMUDriver { | ||
| public: | ||
| /** |
There was a problem hiding this comment.
Probably the constructor is missing here. It would be great if we could make the constructor take in an SPI instance and the CS pin. This way, this driver is not limited to only working with spi1 and cs pin at GPIOA4.
| float gyroBD[3] = {0, 0, 0}; | ||
| float gyrB[3] = {0, 0, 0}; | ||
| float gyr[3] = {0, 0, 0}; | ||
| uint8_t gyro_buffer[14]; |
There was a problem hiding this comment.
The numbers 14, 7, and 7 here are magic numbers in this context. I would suggest creating macros to name these numbers, so the code would be much more readable.
I would actually also recommend replacing lines such as const uint8_t REG_BANK_SEL = 0x76;, to #define REG_BANK_SEL 0x76. In this way, these numbers won't take up any memory, they are handled by the preprocessor instead.
|
|
||
| void ICM42688::readRegister(uint8_t sub_address, uint8_t count, uint8_t * dest) { | ||
| //Set read bit for register address | ||
| uint8_t tx = sub_address | 0x80; |
| IMUData.accx = (float)raw_meas[1] / 2048.0; //Sensitivity Scale Factor in LSB/g | ||
| IMUData.accy = (float)raw_meas[2] / 2048.0; //2048.0 Corresponds with +/- 16g | ||
| IMUData.accz = (float)raw_meas[3] / 2048.0; | ||
|
|
||
| IMUData.gyrx = (float)raw_meas[4] / 16.4; //Sensitivity Scale Factor LSB/dps | ||
| IMUData.gyry = (float)raw_meas[5] / 16.4; //16.4 Corresponds with +/- 2000dps | ||
| IMUData.gyrz = (float)raw_meas[6] / 16.4; |
There was a problem hiding this comment.
Are these scales selectable? Why did we choose these values?
There was a problem hiding this comment.
They are selectable and I believe the values chosen are scale factors reflective of the IMU's default accel and gyro ranges (which are +/- 16g and +/- 2000dps respectively). These values prioritize a larger range over higher sensitivity. If we want more sensitivity, we would need to sacrifice range to do so. Currently, the code is not designed to adjust accelFS; if required though, it could be added. In the meantime, I'll use macros for these values for better readability and to make future changes easier.
There was a problem hiding this comment.
Sounds good.
If you're doing this properly, create an ENUM (const) or defines (follow the warg style guide), and then when you call these numbers you would go SENSITIVITY.12G or SENSITIVITY.LOW.
Created cpp and header files for the ICM42688. The code has been structured from the tested code found at https://github.com/UWARG/efs-kitchen-sinks/blob/main/icm42688.zip
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.
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