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

fmu-v5: Add support for ICM-42688-P #19306

Merged
merged 3 commits into from
Mar 29, 2022
Merged

fmu-v5: Add support for ICM-42688-P #19306

merged 3 commits into from
Mar 29, 2022

Conversation

mxiaogit
Copy link
Contributor

No description provided.

dagar
dagar previously approved these changes Mar 10, 2022
@dagar dagar requested a review from davids5 March 10, 2022 15:38
davids5
davids5 previously approved these changes Mar 10, 2022
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

The select pin PF11 is SPI4_CS2 and a no connect on the other brands of V5. This should work fine. if it does not cause backfeeding. If it does cause a problem. Then the Sensor Set method used on V5X will be needed. Please verify with a scope that the CS pin is not backfeeding the ICM44688p while the VDD_3V3_SENSORS is off.

@mxiaogit mxiaogit closed this Mar 11, 2022
@mxiaogit mxiaogit reopened this Mar 11, 2022
@mxiaogit
Copy link
Contributor Author

@davids5
I observed that fmu-v5 does not have VDD_3V3_SENSORS_EN pin, and VDD_3V3_SENSORS is turned on by default on CUAV V5+ IMU, so there is no situation that VDD_3V3_SENSORS is turned off.

@dagar
Copy link
Member

dagar commented Mar 11, 2022

@davids5
I observed that fmu-v5 does not have VDD_3V3_SENSORS_EN pin, and VDD_3V3_SENSORS is turned on by default on CUAV V5+ IMU, so there is no situation that VDD_3V3_SENSORS is turned off.

It's PE3 here. https://github.com/PX4/PX4-Autopilot/blob/06181fedb9c9d3d33424651d31e25bf91a44063d/boards/px4/fmu-v5/src/spi.cpp#L45

@mxiaogit
Copy link
Contributor Author

The select pin PF11 is SPI4_CS2 and a no connect on the other brands of V5. This should work fine. if it does not cause backfeeding. If it does cause a problem. Then the Sensor Set method used on V5X will be needed. Please verify with a scope that the CS pin is not backfeeding the ICM44688p while the VDD_3V3_SENSORS is off.

It does cause backfeeding.

@davids5
Copy link
Member

davids5 commented Mar 11, 2022

The select pin PF11 is SPI4_CS2 and a no connect on the other brands of V5. This should work fine. if it does not cause backfeeding. If it does cause a problem. Then the Sensor Set method used on V5X will be needed. Please verify with a scope that the CS pin is not backfeeding the ICM44688p while the VDD_3V3_SENSORS is off.

It does cause backfeeding.

That is a problem. The CS needs to be low while the power is off or it can latch up the sensor.

@mxiaogit
Copy link
Contributor Author

The select pin PF11 is SPI4_CS2 and a no connect on the other brands of V5. This should work fine. if it does not cause backfeeding. If it does cause a problem. Then the Sensor Set method used on V5X will be needed. Please verify with a scope that the CS pin is not backfeeding the ICM44688p while the VDD_3V3_SENSORS is off.

It does cause backfeeding.

That is a problem. The CS needs to be low while the power is off or it can latch up the sensor.

How do we do it?

@@ -160,6 +160,7 @@ static px4_hw_mft_list_entry_t mft_lists[] = {
{0x0000, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)},
{0x0105, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5 R:5 V:1
{0x0500, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5+ R:0 V:5
{0x0502, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5+ R:2 V:5
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{0x0502, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5+ R:2 V:5
{0x0552, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5+ R:2 V:5 ICM42688P
{0x0562, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5+ Nano R:2 V:6 ICM42688P

@@ -5,8 +5,14 @@

board_adc start

# Internal SPI bus ICM-20602
icm20602 -s -R 2 -q start
if ver hwtypecmp V552
Copy link
Member

Choose a reason for hiding this comment

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

if ver hwtypecmp V552 V562

This will cover the

2 5 CUAV V5+ ICM42688P
2 6 CUAV V5Nano ICM42688P

initSPIConfigExternal(SPI::CS{GPIO::PortI, GPIO::Pin7}),
initSPIConfigExternal(SPI::CS{GPIO::PortI, GPIO::Pin8})

initSPIHWVersion(HW_VER_REV(0, 2), {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
initSPIHWVersion(HW_VER_REV(0, 2), {
initSPIHWVersion(HW_VER_REV(5, 2), {

A duplicate is needed for the Nano with InitSPIHWVersion(HW_VER_REV(6, 2), {

Copy link
Member

Choose a reason for hiding this comment

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

While Appling power to the unit, please capture the PE3, VDD_3V3_SENSORS, and PC5, PB4 on a scope and post the images here.

Copy link
Contributor Author

@mxiaogit mxiaogit Mar 15, 2022

Choose a reason for hiding this comment

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

The channels are PE3, VDD_3V3_SENSORS, PC5, PB4 in sequence. Details are here:DSLogic-la-220315-100851.csv

批注 2022-03-15 102617
批注 2022-03-15 102506
批注 2022-03-15 102537
批注 2022-03-15 110152

Copy link
Member

Choose a reason for hiding this comment

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

hmm this looks incorrect to me. Even the other CS line. What is the width if the PE3 off time? ---_---

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1647480348(1)
PE3(VDD_3V3_SENSORS_EN) is always on after initialization is complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our last version of the hardware is similar, when the chip is still ICM-20602.
批注 2022-03-17 164347
批注 2022-03-17 164406
批注 2022-03-17 164447

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davids5 What do you think there is a problem?

@davids5
Copy link
Member

davids5 commented Mar 21, 2022

@mxiaogit The signals do not look correct to me. I need to review the power up sequence in the code.

Power should be off and all the node (CS, MOSI, MISO etc) to the sensors should be low until after power is applied.

@davids5
Copy link
Member

davids5 commented Mar 22, 2022

@mxiaogit - I found what I think is the root cause. It is probably systemic and came in when the SPI/I2C work was done by @bkueng. The initial condition out of reset for the sensors power needs be off, this was done in the stm32_boardinitialize and we get a built-in delay to when board_app_initialize is called.

I do not have current V5+ HW and It would take quite a bit to instrument it. Since you already have the HW set up can you please pull in e367997 and re-test and post the same signals timing here.

@davids5
Copy link
Member

davids5 commented Mar 23, 2022

@mxiaogit - I found what I think is the root cause. It is probably systemic and came in when the SPI/I2C work was done by @bkueng. The initial condition out of reset for the sensors power needs be off, this was done in the stm32_boardinitialize and we get a built-in delay to when board_app_initialize is called.

I do not have current V5+ HW and It would take quite a bit to instrument it. Since you already have the HW set up can you please pull in e367997 and re-test and post the same signals timing here.

Never mind I missed the call to board_control_spi_sensors_power_configgpio();. I need to step through and make sure it is really setting it to off.

davids5
davids5 previously approved these changes Mar 23, 2022
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

The code change is fine.

The change is no worse then the existing code.

@mxiaogit
Copy link
Contributor Author

mxiaogit commented Mar 25, 2022

@mxiaogit - I found what I think is the root cause. It is probably systemic and came in when the SPI/I2C work was done by @bkueng. The initial condition out of reset for the sensors power needs be off, this was done in the stm32_boardinitialize and we get a built-in delay to when board_app_initialize is called.

I do not have current V5+ HW and It would take quite a bit to instrument it. Since you already have the HW set up can you please pull in e367997 and re-test and post the same signals timing here.

@davids5 I found some problems with our hardware and after fixing the problem, VDD_3V3_SENSORS is controllable. Here are the waveforms tested by e367997, in order of PE3(VDD_3V3_SENSORS_EN), VDD_3V3_SENSORS(IMU1_3V3), VDD_3V3_SENSORS(IMU2_3V3), PC5(SPI1_DRDY4_ICM20602), PB4(SPI1_ DRDY1_ICM20689).
133024ba929c3cd5aa8875cd45ddf84
b60a36e53dd11fc2a5709dd6bdfcb03
There exist two cases, VDD_3V3_SENSORS closing time has two cases, about 4 seconds or about 8 seconds.

@mxiaogit mxiaogit requested a review from davids5 March 25, 2022 10:15
@davids5
Copy link
Member

davids5 commented Mar 25, 2022

@mxiaogit - That looks more like I would expect.

Do you check the timing on this version of the code with the fixed HW?

What is the time from VDD_3V3_SENSORS_EN to VDD_3V3_SENSORS(IMU1_3V3) valid? Then from VDD_3V3_SENSORS(IMU1_3V3) to the first chip select or datardy going High (Zoom in a bit)

Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

There needs to be a manifest entry for each rev/ver pair.

@@ -160,6 +160,8 @@ static px4_hw_mft_list_entry_t mft_lists[] = {
{0x0000, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)},
{0x0105, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5 R:5 V:1
{0x0500, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5+ R:0 V:5
{0x0502, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5+ R:2 V:5 ICM42688P
{0x0602, hw_mft_list_v0500, arraySize(hw_mft_list_v0500)}, // Alias for CUAV V5nano R:2 V:6 ICM42688P
Copy link
Member

Choose a reason for hiding this comment

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

What about the entries for V552 and V562?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the macro definition: define HW_VER_REV(v,r) ((uint32_t)((v) & 0xff) << 8) | ((uint32_t)(r) & 0xff), 0x0502 is V552, 0x0602 is V562.

Copy link
Member

Choose a reason for hiding this comment

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

right! Would you mind adding and using the format from

https://github.com/PX4/PX4-Autopilot/blob/master/boards/px4/fmu-v5x/src/board_config.h#L182-L199

So this will no be confused again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified. Please review.

davids5
davids5 previously approved these changes Mar 28, 2022
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

Let's Bring this in and can make the #define change later

@mxiaogit
Copy link
Contributor Author

mxiaogit commented Mar 29, 2022

@mxiaogit - That looks more like I would expect.

Do you check the timing on this version of the code with the fixed HW?

What is the time from VDD_3V3_SENSORS_EN to VDD_3V3_SENSORS(IMU1_3V3) valid? Then from VDD_3V3_SENSORS(IMU1_3V3) to the first chip select or datardy going High (Zoom in a bit)

@davids5
The following are the waveforms tested, followed by PE3(VDD_3V3_SENSORS_EN), VDD_3V3_SENSORS(IMU1_3V3), VDD_3V3_SENSORS(IMU2_3V3), PC5(SPI1_DRDY4_ICM20602), PB4(SPI1_DRDY1_ICM20689). There are two cases, VDD_3V3_SENSORS has two kinds of off time, about 4 seconds or about 8 seconds. Added zoom-in view.

Group 1:
00
01
02
03
04
05
06

Group 2:
00
01
02
03
04
05
06

Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@mxiaogit - thank you

@davids5
Copy link
Member

davids5 commented Mar 29, 2022

@dagar - based on the voltage supply rise time and that this is digital interpretation: The digital value of the VDD_3V3_SENSORS is indicating VIH threshold (of the logic analyzer) not stable 3V3 VDD. Note in image #5 that the CS is leading VDD_3V3_SENSORS the voltage change. Therefore we may need to increase the delay from activating VDD_3V3_SENSORS_EN to initializing the CS,SPI, RDY lines.

@dagar dagar merged commit 1870b92 into PX4:master Mar 29, 2022
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