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

Pixhawk 6C external I2C bus hack #20200

Merged
merged 3 commits into from
Sep 19, 2022
Merged

Pixhawk 6C external I2C bus hack #20200

merged 3 commits into from
Sep 19, 2022

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Sep 12, 2022

This contains two changes:

  1. This swaps the internal baro and mag back to the bus which is both internal an external but configured as external for this case, in the same way as Update fmu-v6c rc.board_sensors #20151.

  2. Also, it adds a hack for fmu-v6c regarding mag calibration: In order for the mag calibration to treat the mag as an internal one, as well as sensors_status_mag and sensors_status_baro to be correct, we need to insert this override to change these two sensors specifically to internal.

FYI @vincentpoont2

Tests:

Mag calibration:

...
MAG 396321 EN: 1, offset: [-0.039 -0.009 -0.025], scale: [0.991 1.069 0.981], Internal

sensor status topics:

nsh> listener sensors_status_mag

TOPIC: sensors_status_mag
 sensors_status_mag
    timestamp: 233004294 (0.008210 seconds ago)
    device_id_primary: 396321 (Type: 0x06, I2C:4 (0x0C))
    device_ids: [396321, 0, 0, 0]
    inconsistency: [0.0000, nan, nan, nan]
    healthy: [True, False, False, False]
    priority: [50, 0, 0, 0]
    enabled: [True, False, False, False]
    external: [False, False, False, False]

nsh> listener sensors_status_baro

TOPIC: sensors_status_baro
 sensors_status_baro
    timestamp: 241376486 (0.003908 seconds ago)
    device_id_primary: 4028193 (Type: 0x3D, I2C:4 (0x77))
    device_ids: [4028193, 0, 0, 0]
    inconsistency: [0.0000, nan, nan, nan]
    healthy: [True, False, False, False]
    priority: [50, 0, 0, 0]
    enabled: [True, False, False, False]
    external: [False, False, False, False]

@dagar
Copy link
Member

dagar commented Sep 13, 2022

In terms of something we can live with, rather than a board define slipping into common code maybe we could flip it around so that DeviceExternal() respects something like an override list (or call) that the board support could optionally provide?

@vincentpoont2
Copy link
Member

If this goes in, then we should we remove this #20198 ?

@julianoes julianoes force-pushed the pr-pixhawk-6c-hack branch 2 times, most recently from 66e67d0 to 1f97149 Compare September 13, 2022 03:46
@julianoes
Copy link
Contributor Author

@dagar I've updated it, have another look. I believe this is cleaner and doesn't have the board specific ifdef.

@julianoes
Copy link
Contributor Author

@vincentpoont2 yes, this replaces #20198, and #20151.

@vincentpoont2
Copy link
Member

This #20198 just got merged and reverted fmuv6c I2C4 back to "internal" so that the board would start Baro & Mag automatically using Main FW.

initI2CBusInternal(4),

@julianoes Does this need to move back to "external" with this hack?

davids5
davids5 previously approved these changes Sep 13, 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.

LGTM

@davids5
Copy link
Member

davids5 commented Sep 13, 2022

In terms of something we can live with, rather than a board define slipping into common code maybe we could flip it around so that DeviceExternal() respects something like an override list (or call) that the board support could optionally provide?

It may need to be expanded later to be a call out that the board could override but that is not worth the code space now.

@julianoes
Copy link
Contributor Author

@vincentpoont2 yes, it needs to go back to external.

@vincentpoont2
Copy link
Member

vincentpoont2 commented Sep 14, 2022

Everything look good on our test with this PR. @julianoes

Board sensors: /etc/init.d/rc.board_sensors
bmi055_accel #0 on SPI bus 1 rotation 4
bmi055_gyro #0 on SPI bus 1 rotation 4
icm42688p #0 on SPI bus 1 rotation 6
ms5611 #0 on I2C bus 4 (external) address 0x77
ist8310 #0 on I2C bus 4 (external) address 0xC
ist8310 #1 on I2C bus 1 (external) address 0xE rotation 10
ms4525do #0 on I2C bus 4 (external) address 0x28

IL}}(R0I4L9855{7Y)`OX}5

davids5
davids5 previously approved these changes Sep 14, 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.

Looks good to me

This swaps the internal baro and mag back to the bus which is both
internal an external but configured as external for this case.
@julianoes
Copy link
Contributor Author

@dagar I've addressed your two comments, fixed up the original commit, rebased on master and force pushed.

@dagar
Copy link
Member

dagar commented Sep 15, 2022

That looks good, I was just hoping to keep it a little more consistent with px4_i2c_bus_external.

How's this?
338ca2c

I wanted to make sure we capture the same notion of external consistently (I2C::external(), etc).

@julianoes
Copy link
Contributor Author

@dagar your "hack" is much nicer. I will revert my changes, cherry-pick it here, and test it.

@julianoes
Copy link
Contributor Author

Tested, the calibration is correct, and the sensors_status_mag/sensors_status_baro topics as well.

@davids5 davids5 self-requested a review September 19, 2022 16:59
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.

Much cleaner. What is the CI failure?

@julianoes
Copy link
Contributor Author

@davids5 looks unrelated. I'll create a separate PR to fix it.

/Users/runner/work/PX4-Autopilot/PX4-Autopilot/platforms/posix/src/px4/common/lockstep_scheduler/src/lockstep_scheduler.cpp:53:55: fatal error: format specifies type 'unsigned long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Wformat]
                PX4_INFO("setting initial absolute time to %lu us", time_us);

@julianoes
Copy link
Contributor Author

@davids5 nevermind, it's already fixed: c707997.

@julianoes julianoes merged commit a47895e into main Sep 19, 2022
@julianoes julianoes deleted the pr-pixhawk-6c-hack branch September 19, 2022 22:31
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.

4 participants