AP_Rally ready to be merged into plane.

[Michael Day]

Howdy,

This is mostly a shout out to Tridge to note a pull request, but others can read and comment as well.  The more eyes on commits, the better, I say!  If anybody has suggestions/error reports/etc. about the AP_Rally lib, please post them here.

I've reviewed Andrew Chpaman's excellent implementation of AP_Rally.  It is great that Rally Point handling will be in a library!  I rebased off master today and did some tests.  The result is a pull request here:

https://github.com/diydrones/ardupilot/pull/1005

I made two minor changes to AP_Rally as a result of review and testing:

1. If a user has specified an altitude at a Rally point, then it should always be respected.  We should not choose the higher of that value and the home point altitude value.  It is perfectly legitimate to have rally point altitudes that are below the home altitude (e.g., starting high and flying cross country).
2. No need to pass the size of RallyLocation to the constructor.

All my SITL and bench tests checkout.  AP_Rally _can_ load Rally Points created with the previous implementation, which I think was one of Tridge's concerns. I think AP_Rally is ready to be merged into master.

Thanks,

Michael

[Andrew Tridgell]

Hi Michael,

Thanks very much for the testing! Much appreciated.

I have had a look at your pull request (which includes both Andrews
library and your changes). I have only noticed two issues:

1) it doesn't look like the two entries needed in conversion_table[] in
ArduPlane/Parameters.pde have been added.

The conversion_table[] is what allows parameters to change name or
move into a library without losing their existing value. So if
someone had a set rally points loaded and/or had a RALLY_LIMIT_KM
value set then it looks to me as though those would not be preserved
on upgrade.

2) The second (and much more serious) problem is the removal of the
k_param_rally_limit_km enum from Parameters.h. That will change the
numbering of the 9 subsequent parameters in that group, which means
those 9 parameters will become corrupt when users upgrade. When
changing Parameters.h you need to be very careful to preserve
numbering.

The usual method is to rename the old enum to a _old, so
k_param_rally_limit_km becomes k_param_rally_limit_km_old. You then
use the k_param_rally_limit_km_old value in the conversion_table[] so
the AP_Param code can auto-upgrade variables when the user updates
firmware.

I haven't actually tried the AP_Rally code in SITL, I just based the
above on a quick look with gitk.

> All my SITL and bench tests checkout. AP_Rally _can_ load Rally
> Points created with the previous implementation, which I think was one
> of Tridge's concerns. I think AP_Rally is ready to be merged into
> master.

What I was concerned about was not just that rally points can be loaded,
but that they are preserved on upgrade. So users should be able to
upgrade to the new firmware without having to reload their
parameters. That is particularly important for a safety critical
parameter like rally points.

One trick to testing this is to take a copy of eeprom.bin and restore it
when testing so you can wind back to an old version.

Note that you will almost certainly have a corrupt eeprom.bin in SITL
now, due to the enum bug above. You will need to remove it and
reconfigure (see the -w option to sim_vehicle.sh).

Cheers, Tridge

[Andrew Tridgell]

Hi Michael,

I'd also note that if you have put this code in any real boards then
your eeprom is probably corrupt.

You will need to wipe the eeprom by setting FORMAT_VERSION to 0, then
reboot, then reload parameters.

Don't try to fly with that patch, or with an eeprom that has ever been
used with that patch without resetting :-)

Cheers, Tridge

[Michael Day]

Tridge,

Sorry about the indexing blunder.  I have updated my patch to deal with k_param_rally_km as you indicated (looks like github auto-fixes the pull request ... awseome! ).

My SITL tests were specific to Rally and none of those 9 parameter values got exercised during the tests, doh!  Do you guys have a method you use to quickly exercise your unit and regression tests?

Thanks,

Michael

[Andrew Tridgell]

Hi Michael,

> Sorry about the indexing blunder. I have updated my patch to deal with
> k_param_rally_km as you indicated (looks like github auto-fixes the pull
> request ... awseome! ).

It's still not quite right, sorry!

The k_param_rally_limit_km is correct now, but k_param_rally has taken
on the same index as k_param_rally_total. That means existing eeproms
will have an AP_Int8 value that points at an object.

This may work under some circumstances, but it is not safe, and could
lead to eeprom corruption. We need to rename k_param_rally_total to
k_param_rally_total_old and add a new k_param_rally that is used for the
new object.

We then need two conversion_table[] entries, one for
k_param_rally_limit_km_old and one for k_param_rally_total_old

> My SITL tests were specific to Rally and none of those 9 parameter values got
> exercised during the tests, doh! Do you guys have a method you use to quickly
> exercise your unit and regression tests?

There is no really generic was of testing for this sort of breakage. You
just need to think carefully about how the eeprom works (read the
comments in Parameters.{pde,h} to start with) and then try different
upgrade scenarious.

Cheers, Tridge

[Michael Day]

Tridge,

3rd time's the charm.  The k_param_rally_total param now has it's old index back and is in the conversion table.  The k_param_rally doesn't reuse the index, it gets a shiny new one.

How's that?

Michael

[Andrew Tridgell]

> How's that?

Looks good, pushed to master

thanks for working through this, much appreciated!

Cheers, Tridge

[Andrew Chapman]

On Friday, April 18, 2014 2:17:16 PM UTC-7, Andrew Tridgell wrote:

...

1) it doesn't look like the two entries needed in conversion_table[] in
   ArduPlane/Parameters.pde have been added.

   The conversion_table[] is what allows parameters to change name or
   move into a library without losing their existing value. So if
   someone had a set rally points loaded and/or had a RALLY_LIMIT_KM
   value set then it looks to me as though those would not be preserved
   on upgrade.

Sorry, I had no idea about that, could we add that to the 'how to add a parameter' page?

http://dev.ardupilot.com/wiki/code-overview-adding-a-new-parameter/

AC. 

[Andrew Tridgell]

> Sorry, I had no idea about that, could we add that to the 'how to add a
> parameter' page?
>
> http://dev.ardupilot.com/wiki/code-overview-adding-a-new-parameter/

yep, please do!

CCd Bruce in case he has time.

[Randy Mackay]

I've just merged AC's copter integration as well.

On the way, I made a couple of small changes:
1. removed a bunch of commented out debug from the code
2. moved RALLY_WP_SIZE and the the RallyLocation structure
definition to the AP_Rally.h

I noticed that if only 1 rally point is defined, that point will
always be used as the RTL point. I guess this is the way it's meant to be?
I had assumed that the home position was always counted as a Rally point but
it seems not to be. So basically if the users wants RTL to actually return
home he/she needs to redefine a rally point to be at the home location.

-Randy

--
You received this message because you are subscribed to the Google Groups
"drones-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to drones-discus...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Philip Rowse]

There was a parameter in plane that set a limit... If the plane was close to home, and further than xKM from the closest rally point, it would prefer home. This is to allow for the unplanned flight, where the user goes for a fly and forgets to set new rally points.

Philip

[Andrew Tridgell]

Hi Randy,

> I noticed that if only 1 rally point is defined, that point will
> always be used as the RTL point. I guess this is the way it's meant to be?

yes. For planes the home location is usually where you get GPS
lock. That is where you power on the plane, and is usually a very bad
place to circle if you get in trouble (it is often above people).

One of the main reasons for wanting rally points is to stop planes
circling above home, which can be very dangerous.

Cheers, Tridge

[Randy Mackay]

Yes, that is also still there. RALLY_LIMIT_KM.

The default for that parameter is different for each vehicle: 2km for
copter, 5km for plane, 0.5km for rover.

I'd kind of like it better if we just made it the same for all vehicles
because having the #define for vehicle type is a bit ugly. 5km is a long
way for a copter but it might be ok.

[Andrew Chapman]

Yep. Part of my motivation for getting Rally Points ported across to copter was to ensure it can RTL to somewhere other than home, even if home is closest, as we don't allow moving the home point (which I personally think is a safety issue, but I'll let that one lie).

I have a gig coming up to shoot aerials of a yacht, taking off and landing from the deck, so I wanted to be able to set the emergency bail-out to be above the nearest point of solid ground rather than to land on the water where the boat previously was at arming time. There are other similar situations, e.g. people (albeit stupidly) flying off their highrise balconies or nearby a crowd of people would also be better off returning to an alternative location.

AC.

[Maurice Barnes]

Andrew,

Your motivation brings me back to an issue I have with setting an alternate landing point which is potentially more than 10m above the home location. This will cause the copter to come down hard for a landing (using RTL or LAND). When setting Rally Points we need to take into consideration the relative altitude of the Rally Point to Home.

Regards,
Maurice

--

[Michael Day]

Hey guys,

I have made two minor changes to the AP_Rally library that are in my AP_Rally branch.  I'd like them to be considered for inclusion in master.

First change is that rally_location_to_location doesn't need to pass home_loc as a parameter.  Home is already known since AHRS is available to AP_Rally.  Also I made the method public as I have found it useful to be able to have a utility method to translate a RallyLocation into a Location on occasion.

Second change is the addition of a getter method to learn what _rally_limit_km is.

Thanks,

Michael

[Randy Mackay]

Michael,

 

     Sounds good to me.  You’ll do a pull request or you just want to point me at where you’ve got the code (pull request is better if possible)?

 

-Randy

[Michael Day]

Randy,

Pull request in!

Thanks,

Michael