IM the new U - new setup procedure #6
Conversation
src/BNO055.cpp
Outdated
| uint8_t resetBytes[2] = {BNO055_SYS_TRIGGER_ADDR, 0x20}; | ||
| i2c.write(i2cAddress, resetBytes, 2); | ||
| time::wait(30); | ||
| // We check that i2c is activated already. If it is, we check if it is in operational mode. |
There was a problem hiding this comment.
Update your comments to make it clearer that we're checking if the BNO's I2C is activated, not if the STM's I2C is activated
src/BNO055.cpp
Outdated
|
|
||
| // Check if i2c returns a detected device and reports a successful connection. Read the ID from chip id register (0x00) | ||
| // this is to make sure we are connected to the correct device. |
There was a problem hiding this comment.
This is actually just to make sure we are connected to the device. There's no way we're going to have the wrong device connected
src/BNO055.cpp
Outdated
|
|
||
| time::wait(10); | ||
| uint8_t result; | ||
| // We read the ST_RESULT register that the startup POST self-test where the result for each sensor is put. |
There was a problem hiding this comment.
Can you reword this? I don't really understand what you're trying to say here
src/BNO055.cpp
Outdated
| i2c.write(i2cAddress, BNO055_ST_RESULT); | ||
| i2c.read(i2cAddress, &result); | ||
| // All four LSB bits of result should be 1 for successful test | ||
| if ((result & 0x15) == 0x15) { |
There was a problem hiding this comment.
Shouldn't this be if ((result & 0x0F) != 0x0F)? That would be how you check that all the least significant bits of the result are 1
src/BNO055.cpp
Outdated
| @@ -13,80 +13,69 @@ IMU::BNO055::BNO055(IO::I2C& i2C, uint8_t i2cSlaveAddress) : i2c(i2C) { | |||
| * @return a boolean indicating whether or not the device was successfully initialized | |||
| */ | |||
| bool IMU::BNO055::setup() { | |||
There was a problem hiding this comment.
Instead of returning a boolean here, you it would be a little cleaner if you defined an enum specific to the BNO that identifies what kind of error occurred
|
Made the changes to the comments and made an enum for the boolean returns. |
include/BNO055.hpp
Outdated
| enum class BNO055Status { | ||
| FAIL_INIT = 0, | ||
| FAIL_SELF_TEST = 1, | ||
| SUCCESS = 2 |
There was a problem hiding this comment.
This is a nitpick, but generally, we have the successful state have a value of 0, and we call it "OK." It's not really an official standard, but it's pretty common across C code I've seen in my experience
|
Awesome, thanks for the review! Do I have to wait for @ActuallyTaylor to approve before I merge? It says that merging is blocked until requested changes are addressed. |
|
It looks good @brainuser5705 you should be able to merge it now! |
IMU setup procedure has been simplified. I tested with BNO055-Sample, and all log messages work as intended. The changes made were referenced from the documentation regarding initial states and setup requirements of the device. I added inline comments for more details.