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.

Sign up for GitHub

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

Jump to bottom

UAVCAN SensorBridge Improvements #14345

Merged
merged 5 commits into from
Mar 21, 2020
Merged

UAVCAN SensorBridge Improvements #14345

merged 5 commits into from
Mar 21, 2020

Conversation

JacobCrabill
Copy link
Member

Describe problem solved by this pull request
This PR should allow multiple sensors of each supported UavcanSensorBridge type (GNSS, mag, baro, etc.) to be published just like "normal" sensor drivers do. One particularly useful change is that you can now calibrate multiple UAVCAN-connected compasses along with your internal compass(es).

Describe your solution

  • GNSS: use the same publication method as the other SensorBridge sub-classes (to allow multiple ORB instances)
  • mag, baro: Use the PX4Magnetometer and PX4Barometer classes to create the sensor driver instances. This gets around the issue of having only one /dev/magx or /dev/barox registered from the SensorBridge class. It also lets those dedicated classes handle the ioctl calls for calibration and scaling.

Describe possible alternatives
Specifically for the compass bridge, the alternative would be to re-implement all of what PX4Magnetometer is already doing. Let's not re-invent the wheel here.

Test data / coverage
This has all been tested on a private fork; when I get a chance I'll test this specific PR and upload logs.

Additional context
I'll also be submitting a second PR soon that allows us to make use of >2 GNSS units connected via CAN (changes to EKF2 to support arbitrary numbers of GPS). My testing to date of this PR's features actually includes those changes as well.

@dagar dagar self-requested a review March 10, 2020 17:57
@JacobCrabill JacobCrabill mentioned this pull request Mar 11, 2020
Closed
@AlexKlimaj
Copy link
Member

AlexKlimaj commented Mar 11, 2020 •
edited
Loading

This compliments my PR for UAVCAN battery support. #14198

@mrpollo
Copy link
Contributor

mrpollo commented Mar 18, 2020

@JacobCrabill Please upload a log 👍

@JacobCrabill JacobCrabill changed the title [WIP] UAVCAN SensorBridge Improvements UAVCAN SensorBridge Improvements Mar 18, 2020
@JacobCrabill
Copy link
Member Author

Have a log -- will upload once FlightReview is back online
Was able to calibrate two external compasses along with 2 internal compasses (I disabled an internal compass in the Cube by simply commenting out some drivers in fmu-v3/init/rc.board_sensors file).

Here's some console screenshots from testing:

image

Note that the Zubax is crap and does not get a fix (this is an unrelated issue). However, both GPSs show up via UAVCAN and get published.

image

The above shows that the two external UAVCAN compasses are being published as expected. Note that since the UAVCAN data is sent in units of Gauss using floats, the sensor_mag x/y/z_raw data, once cast to integer type, is essentially always rounded to '0'.

@dakejahl dakejahl self-requested a review March 18, 2020 18:54
dakejahl
dakejahl previously approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@dakejahl dakejahl left a comment

Choose a reason for hiding this comment

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

This looks great, seems like a huge improvement!

src/drivers/uavcan/sensors/baro.cpp Outdated Show resolved Hide resolved
src/drivers/uavcan/sensors/baro.cpp Outdated Show resolved Hide resolved
src/drivers/uavcan/sensors/flow.cpp Outdated Show resolved Hide resolved
src/drivers/uavcan/sensors/mag.cpp Outdated Show resolved Hide resolved
src/drivers/uavcan/sensors/mag.cpp Outdated Show resolved Hide resolved
@JacobCrabill
Copy link
Member Author

@dakejahl I've made all of your suggested changes, and I've also rebased (again!) on top of current master.

@dakejahl
Copy link
Contributor

Not sure why this was failing, I restarted CI. It was failing on
nxp_fmuk66-v3_default

Otherwise good to merge @dagar

@JacobCrabill
Copy link
Member Author

@dakejahl pushed another commit to restart the CI checks; all good but one.
Any idea why the one Jenkins test is failing? This looks like it's unrelated to this PR (this was building fine before I did the most recent rebase on top of @MaEtUgR 's last few commits). Not sure how the MC stuff would be related to this.

@dakejahl
Copy link
Contributor

Looks like a SITL test for safe landing is failing

@MaEtUgR
Copy link
Member

MaEtUgR commented Mar 19, 2020

@JacobCrabill It has very likely nothing to do with your changes but the test respectively the simulation running it is probably flaky depending on server resources. The same test passed with my changes and the latest master changes. I tried to just rerun it but it misses a preliminary requirement. We should just rebase on master and push again to see if that was the problem.

@MaEtUgR MaEtUgR mentioned this pull request Mar 19, 2020
Closed
@MaEtUgR
Copy link
Member

MaEtUgR commented Mar 19, 2020

Like expected all tests pass cleanly after pushing a rebased on master version (just to make it a new one): #14424

@JacobCrabill
Copy link
Member Author

Thanks @MaEtUgR
I used this as an excuse to squash some commits together; now I realize I should've rebased on master again to bring in a few new commits 🙄
I'll rebase and push once the CI tests complete.

@JacobCrabill
Copy link
Member Author

Checks passed just before this last rebase, so should be all set once they complete again.

Flight Review is also back up, so here's the log from testing yesterday:
https://review.px4.io/plot_app?log=e7c38167-5209-443c-b371-336ab407f51e

dakejahl
dakejahl previously approved these changes Mar 20, 2020
View reviewed changes
@JacobCrabill
UAVCAN: GNSS sensor bridge: Support multiple pubs
1329a9d 
UavcanGnssBridge did not support more than 1 GNSS callback/publisher.
This has now been fixed; it works the same as the baro, mag, and flow
sensor bridges.  The EKF2 still doesn't support more than 2 GPS
publishers, however.
JacobCrabill and others added 4 commits March 20, 2020 07:35
@JacobCrabill
uavcan: Add device IDs to baro, flow publishers
5ef5bc9 
Can now see proper UAVCAN bus and unique device ID for barometer
instances; optical flow will also have the UAVCAN node ID assigned as
sensor ID.
@JacobCrabill
UAVCAN: Overhaul SensorBridge drivers
58a7f62 
Supposedly multiple sensor callbacks were supported; in reality, this
was not the case, as the mag SensorBridge in particular can only
calibrate one compass, leading to a race condition on which compass
appears first on the bus to get published and calibrated (with no
warning to the user that the 'wrong' compass is being used).

For sensors with existing generic driver classes (baro and mag) the
sensor bridges use these classes for the driver registration, and uORB
publication, and calibration interface (ioctl) handling.
@JacobCrabill @dagar
Update src/drivers/uavcan/sensors/baro.cpp
9b5ed37 
Use absolute-zero constant instead of 273.15f

Co-Authored-By: Daniel Agar <daniel@agar.ca>
@JacobCrabill
UAVCAN Sensors: Improve error handling
31e9f81 
Also add new 'generic' device types for UAVCAN sensors

Other misc. cleanup and style changes.
@JacobCrabill JacobCrabill requested a review from dagar March 20, 2020 14:36
@JacobCrabill
Copy link
Member Author

Maybe @bkueng could take a look?

@dakejahl dakejahl mentioned this pull request Mar 20, 2020
Merged
@dakejahl dakejahl self-requested a review March 20, 2020 17:13
@dagar dagar added this to the Release v1.11.0 milestone Mar 20, 2020
@dakejahl
Copy link
Contributor

Can we merge this?

@dagar
Copy link
Member

dagar commented Mar 21, 2020

Looks good to me.

Ideas to think about for the next round.

  • cdev needs to be purged
  • can we allocate the UAVCAN sensor bridges as needed (no PX4Magnetometer dynamic allocations)?

@dagar dagar merged commit 1e57846 into PX4:master Mar 21, 2020
@JacobCrabill JacobCrabill mentioned this pull request Mar 24, 2020
Closed
@JacobCrabill JacobCrabill deleted the pr-can-sensors branch March 31, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dagar dagar dagar approved these changes

@dakejahl dakejahl dakejahl approved these changes

Assignees
No one assigned
Labels
Admin: Enhancement (improvement) 💡 UAVCAN
Projects
None yet
Milestone
Release v1.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@JacobCrabill @AlexKlimaj @mrpollo @dakejahl @MaEtUgR @dagar