WIP: Adding Alt Frames (ie. Terrain) to API #460

billbonney · 2016-10-30T03:47:48Z

NOTE: this is not intended to be merge (yet 😀), more as a point of discussion.

This is the first working draft of adding frames to the current API to support terrain following.

It's also dependent of Tower PR DroidPlanner/Tower#1826

Comments welcome. Thx.

- This allow support for using different frames - Absolute (GLOBAL_ABS) which relates to AMSL - Relative (GLOBAL_RELATIVE) which means relative to home location - Terrain (GLOBAL_TERRAIN) which means AGL (via Range Finder or STRM Data) - Tested with “round-robbin” download/upload with APM Planner 2.0 Frame setting is preserved from original setting.

billbonney · 2016-11-09T05:13:29Z

@ne0fhyk @kellyschrock Ok, I think this is ready to go... some comments would be nice... 👍 Thx

Also see DroidPlanner/Tower#1826 for the required changes in Tower

m4gr3d · 2016-11-15T07:28:50Z

ClientLib/src/main/java/com/o3dr/services/android/lib/coordinate/LatLongAlt.java

    }

-    public void set(LatLongAlt source){
+    public void set(LatLongAlt source){ // TODO: this looks wrong


@billbonney How so?

It's like a shallow copy, i'm not sure when set is called and why? why is using set different from
latLgAlt1 = latLgAlt2

m4gr3d · 2016-11-15T07:31:11Z

ClientLib/src/main/java/com/o3dr/services/android/lib/coordinate/LatLong.java

@@ -18,6 +18,11 @@
    private double latitude;
    private double longitude;

+    public LatLong(){


@billbonney I don't agree with having a default constructor. This is usually the cause of subtle and hard to find bugs. In which scenario would having a default constructor be helpful?

m4gr3d · 2016-11-15T07:41:07Z

ClientLib/src/main/java/com/o3dr/services/android/lib/coordinate/Frame.java

+
+    private final int frame;
+    private final String name;
+    private final String abbreviation;


@billbonney I'd advocate for the removal of the frame field since it can be substitute by the enum itself.

m4gr3d · 2016-11-15T07:42:38Z

ClientLib/src/main/java/com/o3dr/services/android/lib/coordinate/LatLongAlt.java

@@ -14,24 +15,34 @@
     * Stores the altitude in meters.
     */
    private double mAltitude;
+    private Frame mFrame;
+
+    public LatLongAlt() {


@billbonney Same comment as with LatLong. I fail to see the utility of a default constructor.

I can refactor this. It was a 'quick fix'. To resolve some npe I was having.

LATER: The issue is this in BaseSpatial. And that it constructed without a point.

public LatLongAlt getCoordinate() { if (coordinate == null) coordinate = new LatLongAlt(); return coordinate; }

Which means it's partially created object. And then set to call here

public void addSpatialWaypoint(BaseSpatialItem spatialItem, LatLong point) { double alt = getLastAltitude(); spatialItem.getCoordinate().set(point); spatialItem.getCoordinate().setAltitude(alt); addMissionItem(spatialItem); }

The fix was to have getCoordinate to create a LatLngAlt when none existed. I can add the parameter list there.

The real fix is to refactor BaseSpatial on construction to take a point so it's not null in the first place (but that may break getNewItem for other mission items (i'll need to think about that)

@Override public void onMapClick(LatLong point) { if (missionProxy == null) return; // If an mission item is selected, unselect it. missionProxy.selection.clearSelection(); if (selectedType == null) return; BaseSpatialItem spatialItem = (BaseSpatialItem) selectedType.getNewItem(); missionProxy.addSpatialWaypoint(spatialItem, point); }

it looks like the above is related with change b343b8c

m4gr3d · 2016-11-15T07:44:31Z

ClientLib/src/main/java/com/o3dr/services/android/lib/coordinate/LatLongAlt.java

@@ -53,7 +76,9 @@ public boolean equals(Object o) {

        LatLongAlt that = (LatLongAlt) o;

-        if (Double.compare(that.mAltitude, mAltitude) != 0) return false;
+        if ((Double.compare(that.mAltitude, mAltitude) != 0)
+            && (that.mFrame.asInt() != mFrame.asInt()) )


@billbonney Since mFrame is an enum, you can just compare it using that.mFrame != mFrame.

In addition, the equals(), hashCode() and toString() methods were generated automatically via Android Studio: Code => Generate.

Ok, good tip... I'll check that.

m4gr3d · 2016-11-15T07:51:42Z

ClientLib/src/main/java/com/o3dr/services/android/lib/coordinate/LatLongAlt.java


-    public LatLongAlt(double latitude, double longitude, double altitude) {
+    public LatLongAlt(double latitude, double longitude, double altitude, Frame frame) {


@billbonney Given that Frame.GLOBAL_RELATIVE is the default frame, you could include an additional constructor that omit the frame parameter and sets mFrame to Frame.GLOBAL_RELATIVE.
This removes the need to update all the files where the constructor is invoked.

I did that for a reason in that it exposes the parts in the code that sets the frame. I.e you can see explicitly what frame is being assumed. You don't want to accidentally put abs alt into relative. It would seem like a flyaway.

(LATER: Ironically, this is similar thought why default constructors are bad, it hides assumptions)

m4gr3d · 2016-11-15T07:52:19Z

ClientLib/src/main/java/com/o3dr/services/android/lib/drone/connection/ConnectionParameter.java

@@ -277,7 +277,7 @@ public static ConnectionParameter newSoloConnection(String ssid, @Nullable Strin
            eventsDispatchingPeriod);
    }

-    private ConnectionParameter(@ConnectionType.Type int connectionType, Bundle paramsBundle){
+    public ConnectionParameter(@ConnectionType.Type int connectionType, Bundle paramsBundle){


@billbonney Why make this public?

I was having a build issue with this being private. I haven't investigated, and had not meant for it to be checked in. I should put a TODO on it, and come back and investigate.

As far as I remember class BasicTest complains about the access to this constructor while compilation:
https://github.com/dronekit/dronekit-android/blob/develop/ClientLib/src/androidTest/java/org/droidplanner/services/android/impl/BasicTest.java#L60

@mariansoban Thanks. I can revert this change for now and we can look at fixing the test cases

billbonney added 2 commits October 29, 2016 10:48

Add Frame to LatLongAlt for Terrain Following

6906cf8

billbonney changed the title ~~WIP: Adding Frames to API~~ WIP: Adding Alt Frames (ie. Terrain) to API Oct 30, 2016

billbonney mentioned this pull request Oct 30, 2016

WIP: Update to Tower to support Alt Frames like Terrain DroidPlanner/Tower#1826

Open

billbonney added 4 commits November 2, 2016 17:31

Refactor accessor to asInt() to be more succinct

99e218e

SpatialItem: Fix mistake in not saving newly created Coordinate

ea4a36c

GuidedPoint: Fix crash when drone position is null

9836b92

LatLongAlt: Update to return true when changed.

8482dbe

m4gr3d reviewed Nov 15, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Adding Alt Frames (ie. Terrain) to API #460

WIP: Adding Alt Frames (ie. Terrain) to API #460

billbonney commented Oct 30, 2016 •

edited

Loading

billbonney commented Nov 9, 2016 •

edited

Loading

m4gr3d Nov 15, 2016

billbonney Nov 15, 2016

m4gr3d Nov 15, 2016

m4gr3d Nov 15, 2016

m4gr3d Nov 15, 2016

billbonney Nov 15, 2016 •

edited

Loading

billbonney Nov 15, 2016

m4gr3d Nov 15, 2016

m4gr3d Nov 15, 2016

billbonney Nov 15, 2016

m4gr3d Nov 15, 2016

billbonney Nov 15, 2016 •

edited

Loading

m4gr3d Nov 15, 2016

billbonney Nov 15, 2016 •

edited

Loading

mariansoban Nov 15, 2016

billbonney Nov 15, 2016


		public LatLongAlt(double latitude, double longitude, double altitude) {
		public LatLongAlt(double latitude, double longitude, double altitude, Frame frame) {

WIP: Adding Alt Frames (ie. Terrain) to API #460

Are you sure you want to change the base?

WIP: Adding Alt Frames (ie. Terrain) to API #460

Conversation

billbonney commented Oct 30, 2016 • edited Loading

billbonney commented Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billbonney Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billbonney Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billbonney Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billbonney commented Oct 30, 2016 •

edited

Loading

billbonney commented Nov 9, 2016 •

edited

Loading

billbonney Nov 15, 2016 •

edited

Loading

billbonney Nov 15, 2016 •

edited

Loading

billbonney Nov 15, 2016 •

edited

Loading