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

AP_OpticalFlow - Add rover support #21510

Merged
merged 6 commits into from
Nov 30, 2022
Merged

Conversation

stephendade
Copy link
Contributor

@stephendade stephendade commented Aug 22, 2022

This PR adds optical flow support for Rover. No rangefinder is needed, as a new parameter (FLOW_HGT_OVR) is used to specify an "override" constant height above ground for the sensor.

Successfully tested with an UPixels UPFlow sensor, Mro X2.1-777 and https://www.servocity.com/bogie-runt-rover/

Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

LGTM

@rmackay9
Copy link
Contributor

This was my attempt at the same thing #20038

@stephendade I guess your implementation actually works? :-). Mine has a lot of code changes but it was only partially successful during actual testing (on a Copter).

@geofrancis
Copy link

geofrancis commented Aug 22, 2022

very cool. does it have to track the ceiling? Would ground tracking work if it was in focus?

@simon88
Copy link

simon88 commented Aug 22, 2022

NIce I'll try it, so you've tested with upwards hereflow and one upwards RF? A chance to port it to copter like @rmackay9 with #20038 ?

@stephendade
Copy link
Contributor Author

@stephendade I guess your implementation actually works? :-). Mine has a lot of code changes but it was only partially successful during actual testing (on a Copter).

Yes, tested and does work nicely. This PR is simpler than your implemention - I only assume the sensor is upwards or downwards facing.

very cool. does it have to track the ceiling? Would ground tracking work if it was in focus?

Yes, ground tracking should work for Rover. Most sensors have a minimum focus distance of 10 or 20cm, so the chassis will need to be some distance off the ground.

NIce I'll try it, so you've tested with upwards hereflow and one upwards RF? A chance to port it to copter like @rmackay9 with #20038 ?

This does work with copter (though untested). The only sensor I've tested with is the UpFlow sensor, but it should work with all other supported sensors. Note that ArduPilot only supports having 1 optical flow sensor connected.

@stephendade
Copy link
Contributor Author

stephendade commented Aug 23, 2022

Happy to assist anyone who wants to test this too!

EDIT:
During ceiling-tracking testing, I've noticed that the ceiling does need some texture/colour to be tracked properly. Most (plain white) ceilings won't give a good track. I'd suggest putting some high-contrast posters or shapes on the ceiling for better tracking - I used a bunch of AprilTags (A4 printouts, 1 per m^2) and got a decent track.

@simon88
Copy link

simon88 commented Aug 31, 2022

Hi @stephendade
I've just tried to use your commit with copter, I suffered a crash just after take off. I had lot of EKF3 IMUX STOPPED AIDING and EKF3 IMUX STARTED RELATIVE AIDING. The copter take off and immediatly he went to the left, I had no control over the roll. Altitude was good. I also had some memory messages like can[125] exeptions : 88 free maybe a memoty issue?

My config Cube Orange HereFlow + 1 upwards LightWare lidar SF000/B on quadcopter.
here is the BIN in zip file 5 01-01-1980 01-00-00.zip
Just for information I add an another BIN file if you want to see my hereflow calibration with OF.flowX and OF.flowY 2 01-01-1980 01-00-00.zip (for me the calibration is good)

@rishabsingh3003
Copy link
Contributor

@stephendade I was trying to come up with something very similar a while back. As far as I can recall, when the EKF fuses the rangefinder data, it pretty much assumes that the orientation is downwards. It's hardcoded (I think) and it required a lot of changes to the EKF, so I gave up mid-way.
I don't see any changes to the EKF library in this PR.. I was wondering if my initial assessment was wrong?

I see those changes in @rmackay9 's PR.

@stephendade
Copy link
Contributor Author

My config Cube Orange HereFlow + 1 upwards LightWare lidar SF000/B on quadcopter.

I don't see any changes to the EKF library in this PR.. I was wondering if my initial assessment was wrong?

@simon88 and @rishabsingh3003: There is no support for upward-facing rangefinders.

I suspect, since I've only been testing with rovers, any issues with height-above-ground (rangefinders) being fused with optical flow have been hidden. Obviously, rovers are always at 0m height-above-ground.

Unfortunately, I haven't got any copters to test with :(

What I'll likely do is limit the upwards-facing sensors to rover vehicles only.

@rmackay9
Copy link
Contributor

rmackay9 commented Sep 1, 2022

As @rishabsingh3003 mentioned, my PR #20038 modifies the EKF to use the appropriate range finder to match the orientation of the optical flow sensor.

So I think we could get this PR in if it only covered adding downward facing optical flow support to Rover. Once we try to add in the upward facing support then we need the EKF changes.

@stephendade
Copy link
Contributor Author

My config Cube Orange HereFlow + 1 upwards LightWare lidar SF000/B on quadcopter.

I don't see any changes to the EKF library in this PR.. I was wondering if my initial assessment was wrong?

@simon88 and @rishabsingh3003: There is no support for upward-facing rangefinders.

I suspect, since I've only been testing with rovers, any issues with height-above-ground (rangefinders) being fused with optical flow have been hidden. Obviously, rovers are always at 0m height-above-ground.

Unfortunately, I haven't got any copters to test with :(

So I think we could get this PR in if it only covered adding downward facing optical flow support to Rover. Once we try to add in the upward facing support then we need the EKF changes.

Agree.

@stephendade stephendade changed the title AP_OpticalFlow - Add rover and upwards facing support AP_OpticalFlow - Add rover a̶n̶d̶ ̶u̶p̶w̶a̶r̶d̶s̶ ̶f̶a̶c̶i̶n̶g̶ support Sep 7, 2022
@stephendade
Copy link
Contributor Author

I've updated this to remove the upwards facing support and have added a FLOW_HEIGHT parameter (only used in rover) to specify the height of the sensor above the ground.

It did require a number of changes in the AHRS/EKF to pass the height variable through correctly.

Initial tests were good.

Copy link
Contributor

@rishabsingh3003 rishabsingh3003 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 better. However, I would imagine that since most optical flow sensors typically perform best when they are slightly high above the ground (at least 5-10 meters), the use case for this would be very limited. I think for Rover, it'll be much more useful to have an upward-facing optical flow (which I think now you have removed from this PR).
I would also imagine that this probably does not work for fast rovers, since the flow change will be too quick.

libraries/AP_OpticalFlow/AP_OpticalFlow.cpp Outdated Show resolved Hide resolved
libraries/AP_NavEKF3/AP_NavEKF3_core.h Outdated Show resolved Hide resolved
@geofrancis
Copy link

geofrancis commented Sep 7, 2022

This looks better. However, I would imagine that since most optical flow sensors typically perform best when they are slightly high above the ground (at least 5-10 meters), the use case for this would be very limited. I think for Rover, it'll be much more useful to have an upward-facing optical flow (which I think now you have removed from this PR). I would also imagine that this probably does not work for fast rovers, since the flow change will be too quick.

where I think this will be very useful is tracking the ground outside on terrain where wheel slip is an issue so wheel encoders wont work. your correct that it would be speed limited but i suspect even 1m/s would be enough for most people when moving in a confined space. combined with proximity it should allow more precise navigation between obstacles without needing RTK.

looking at the px4flow specs show that it could do reasonable speeds just 1m above the ground, so i suspect it should work down to 10 or 20cm with some adjustments to focus.

Ground distance 1m 3m 10m
16mm lens 2.4m/s 7.2m/s 24m/s
8mm lens 4.8m/s 14.4m/s 48m/s
6mm lens 6.4m/s 19.2m/s 64m/s
4mm lens 9.6m/s 28.8m/s 96m/s

@stephendade
Copy link
Contributor Author

looking at the px4flow specs show that it could do reasonable speeds just 1m above the ground, so i suspect it should work down to 10 or 20cm with some adjustments to focus.

I've got a UPFlow sensor mounted ~10cm above the ground. It's not great quality - I'll need to look at mounting it a bit higher for decent measurements.

This is mostly for indoor and/or slow vehicle, particularly non-GPS vehicles that need an EKF velocity source (and use computer vision for the position source).

@geofrancis
Copy link

I've got a UPFlow sensor mounted ~10cm above the ground. It's not great quality - I'll need to look at mounting it a bit higher for decent measurements.

have you tried adding a bright light at an angle similar to an optical mouse, it should give it more contrast and cast shadows on surface imperfections making them easier to track.

@stephendade stephendade force-pushed the roverflow branch 3 times, most recently from 909c7e6 to c0bd0f4 Compare September 30, 2022 05:41
@stephendade
Copy link
Contributor Author

@rishabsingh3003 and @rmackay9 this should be good to go. There appear to be a few unrelated tests failing though.

It's been tested with a rover and works fairly well!

@stephendade stephendade changed the title AP_OpticalFlow - Add rover a̶n̶d̶ ̶u̶p̶w̶a̶r̶d̶s̶ ̶f̶a̶c̶i̶n̶g̶ support AP_OpticalFlow - Add rover support Oct 10, 2022
Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

need some checks by @priseborough

libraries/AP_DAL/AP_DAL.h Outdated Show resolved Hide resolved
@@ -289,8 +289,12 @@ void NavEKF3_core::FuseOptFlow(const of_elements &ofDataDelayed, bool really_fus
ftype vd = stateStruct.velocity.z;
ftype pd = stateStruct.position.z;

// constrain height above ground to be above range measured on ground
// constrain height above ground to be above range measured on ground, or user specified height (if given, for rover)
#if APM_BUILD_TYPE(APM_BUILD_Rover)
Copy link
Contributor

Choose a reason for hiding this comment

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

@priseborough why is this working? @stephendade reports he can get optical flow fusion with no rangefinder. Does this mean the EKF is not properly checking the health of the rangefinder before fusing flow data?

Copy link
Contributor

Choose a reason for hiding this comment

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

The optical flow fusion shouldn't be checking the range finder, it should be checking that the terrain height offsets state estimate is still valid. I had a quick look at the code and it does not appear that this is being done.

@tridge
Copy link
Contributor

tridge commented Nov 7, 2022

maybe we should not be dividing by DCM c.z ?

@rmackay9
Copy link
Contributor

rmackay9 commented Nov 8, 2022

As discussed on the dev call I tested disabling the rangefinder in Copter while using optical flow and it appears we have a bug (as @tridge suspected). I've raised an issue here.

@stephendade stephendade force-pushed the roverflow branch 2 times, most recently from 232f19b to 428a4e1 Compare November 14, 2022 04:28
@stephendade
Copy link
Contributor Author

I've now fixed up the issues raised in the devcall by @rmackay9 and @tridge.

I will perform a few more tests with a real rover (once the weather is better) and then it should be good to go in.

@stephendade
Copy link
Contributor Author

I've tested this on a real rover. Works pretty well.

I'm not great at EKF debugging, so happy to post other graphs as needed :)

Using GPS as Velocity source:
VE_EKFvsGPS_157

Using Optical flow as Velocity source:
VE_EKFvsGPS_158

@tridge
Copy link
Contributor

tridge commented Nov 28, 2022

testing copter flow in CIRCLE mode would be good, checking we get low innovations on flow and good pos hold in wind

Copy link
Contributor

@priseborough priseborough left a comment

Choose a reason for hiding this comment

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

OK wth EKF changes but need to confirm that the height override cannot be set by other platforms.

@stephendade
Copy link
Contributor Author

I've now tested this in copter SITL:

OK wth EKF changes but need to confirm that the height override cannot be set by other platforms.

Have confirmed that height override param can't be set in other platforms.

testing copter flow in CIRCLE mode would be good, checking we get low innovations on flow and good pos hold in wind

Circle mode still works well in windy (10m/s) conditions. Flow Innovations were the same vs master.

@rmackay9 rmackay9 merged commit e6b3028 into ArduPilot:master Nov 30, 2022
@rmackay9
Copy link
Contributor

Great stuff, merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants