Driver #1
Driver #1
Conversation
.gitmodules
Outdated
| path = lib/pico-sdk | ||
| url = https://github.com/raspberrypi/pico-sdk.git |
There was a problem hiding this comment.
| path = lib/pico-sdk | |
| url = https://github.com/raspberrypi/pico-sdk.git | |
| path = lib/pico-sdk | |
| url = ../../raspberrypi/pico-sdk.git |
You can do relative URLs so that this works with either HTTPS or SSH.
CMakeLists.txt
Outdated
| ) | ||
|
|
||
| pico_add_extra_outputs(gyro_test) | ||
|
|
lib/pico-sdk
Outdated
There was a problem hiding this comment.
It's better to pin the pico-sdk to a specific release tag than whatever was the latest commit at the time you added the submodule.
gyro.c
Outdated
|
|
||
|
|
||
| int main(){ | ||
|
|
||
|
|
There was a problem hiding this comment.
| int main(){ | |
| int main(void) | |
| { |
Let's try to cut down on unnecessary whitespace.
gyro.c
Outdated
| 4, /* SDA pin # */ | ||
| 5, /* SCL pin # */ | ||
| 1, /* gyro_fs */ | ||
| 1, /* accel_fs */ | ||
| 100000);/* baudrate */ |
There was a problem hiding this comment.
Instead of adding comments for these literals, it's better to #define constants with descriptive names so that you can immediately read off what's going on and not depend on the comments. More importantly, what happens when you want to reference any of these constant values later in your program? Right now you have magic numbers floating around.
mpu6050.c
Outdated
| #define MPU6050_PWR_MGMT_1 0x6B | ||
|
|
||
| #define MPU6050_GYRO_CONFIG 0x1B | ||
| #define MPU6050_ACCEL_CONFIG 0x1C | ||
|
|
||
| #define MPU6050_GYRO_XOUT 0x43 | ||
| #define MPU6050_ACCEL_XOUT 0x3B |
There was a problem hiding this comment.
I'm under the belief that these should go into mpu6050.h.
mpu6050.c
Outdated
| /* store i2c instance for read and write */ | ||
| static i2c_inst_t *mpu6050_i2c = NULL; |
There was a problem hiding this comment.
NO!
You've now tied the entire library to one I2C bus and mpu6050! We should instead pass around an "mpu6050" handle object between calls so that we can reuse this for multiple sensors and/or busses.
mpu6050.c
Outdated
| /* store i2c instance for read and write */ | ||
| static i2c_inst_t *mpu6050_i2c = NULL; | ||
|
|
||
| int mpu6050_init(i2c_inst_t *i2c, |
There was a problem hiding this comment.
You should break up each of the individual init phases to smaller static functions to make it easier to read the init function. Static functions will be inlined with any modern compiler optimization enabled so there's really no downside to using them to encapsulate functionality.
mpu6050.c
Outdated
| return 0; | ||
| } | ||
|
|
||
| int mpu6050_read_gyro(int16_t *gx, int16_t *gy, int16_t *gz) |
There was a problem hiding this comment.
I'd personally make a struct for each of these values since they repeat, or even an array of three elements, though this latter approach would require you to define some constants as to make it easier to understand which element you're accessing.
mpu6050.c
Outdated
|
|
||
| int mpu6050_read_gyro(int16_t *gx, int16_t *gy, int16_t *gz) | ||
| { | ||
| uint8_t gyro_reg = MPU6050_GYRO_XOUT; |
There was a problem hiding this comment.
| uint8_t gyro_reg = MPU6050_GYRO_XOUT; | |
| const uint8_t gyro_reg = MPU6050_GYRO_XOUT; |
Try to use constants where possible. (This is but one instance in this code.)
No description provided.