Add support for ICM20789 #7477
Add support for ICM20789 #7477
Conversation
… user_ctrl This bit should never be set on 20789
There was a problem hiding this comment.
Comments inline - none are outdated, GitHub is dumb.
Commit AP_Baro: do not take semaphore if HAL_INS_MPU60x0_NAME not set is a bit useless as it is overwritten in another commit later.
I think this doesn't address @lucasdemarchi's comments, although I'm not sure what exact method he is proposing to fix it. In my opinion, we have enough cases of multi-purpose devices that we need a library with singleton multi drivers - then we don't depend on what library initializes first.
| @@ -486,14 +486,6 @@ void AP_InertialSensor_Invensense::start() | |||
| _register_write(MPUREG_INT_ENABLE, BIT_RAW_RDY_EN); | |||
| hal.scheduler->delay(1); | |||
|
|
|||
| // clear interrupt on any read, and hold the data ready pin high | |||
| // until we clear the interrupt | |||
| uint8_t v = _register_read(MPUREG_INT_PIN_CFG) | BIT_INT_RD_CLEAR | BIT_LATCH_INT_EN; | |||
There was a problem hiding this comment.
You are removing more than you should here. Look at the initial commit on these lines, only the BIT_BYPASS_EN should be removed here (basically revert what was done there).
| @@ -13,7 +13,7 @@ const AP_HAL::HAL& hal = AP_HAL::get_HAL(); | |||
|
|
|||
| static AP_HAL::OwnPtr<AP_HAL::Device> i2c_dev; | |||
| static AP_HAL::OwnPtr<AP_HAL::Device> spi_dev; | |||
| static AP_HAL::OwnPtr<AP_HAL::Device> lidar_dev; | |||
| static AP_HAL::OwnPtr<AP_HAL::Device> dev; | |||
There was a problem hiding this comment.
I was expecting this to be removed, not just renamed.
| printf("checking lidar\n"); | ||
| if (!lidar_dev->get_semaphore()->take(0)) { | ||
| AP_HAL::panic("Failed to get lidar semaphore"); | ||
| printf("checking baro\n"); |
| AP_HAL::panic("Failed to get spi semaphore"); | ||
| } | ||
|
|
||
|
|
||
| i2c_dev = std::move(hal.i2c_mgr->get_device(1, 0x63)); | ||
| lidar_dev = std::move(hal.i2c_mgr->get_device(1, 0x29)); | ||
| dev = std::move(hal.i2c_mgr->get_device(1, 0x29)); |
There was a problem hiding this comment.
The i2c_dev uses an address that is defined the ICM20789 baro driver, the dev / lidar_dev uses an address that is supposedly the VL53L0X. So how are both baro?
| @@ -202,7 +202,7 @@ bool AP_Baro_ICM20789::init() | |||
| bool AP_Baro_ICM20789::send_cmd16(uint16_t cmd) | |||
| { | |||
| uint8_t cmd_b[2] = { uint8_t(cmd >> 8), uint8_t(cmd & 0xFF) }; | |||
| return dev->transfer(cmd_b, 2, nullptr, 0); | |||
| return dev->transfer(cmd_b, ARRAY_SIZE(cmd_b), nullptr, 0); | |||
There was a problem hiding this comment.
This shouldn't be array size, but sizeof. Array size gives how many "spaces" the array has, but here we want how many bytes the array has - and that's given by sizeof.
P.S.: yes, here it is the same because the array is of uint8_t, but let's not give a bad example 🙂
|
this PR has been merged via the skyviper PR, after discussions with Randy. |
| @@ -475,10 +285,6 @@ void AP_InertialSensor_Invensense::start() | |||
| _register_write(MPUREG_INT_ENABLE, BIT_RAW_RDY_EN); | |||
| hal.scheduler->delay(1); | |||
|
|
|||
| // clear interrupt on any read, and hold the data ready pin high | |||
| // until we clear the interrupt | |||
| _register_write(MPUREG_INT_PIN_CFG, _register_read(MPUREG_INT_PIN_CFG) | BIT_INT_RD_CLEAR | BIT_LATCH_INT_EN); | |||
There was a problem hiding this comment.
how this could possibly work? please don't remove this chunk
|
@tridge it looks like at least that one comment I just made needs to be addressed.... I had forgotten to click the "submit review" button. |
This PR is intended to replace #6449
I believe I have addressed most of the review comments.
These patches can be tested on a stand-alone PCNC1 board using #7476