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

GuidedActions: fix incorrect slider value conversion #12259

Closed
wants to merge 1 commit into from

Conversation

or-rhssk
Copy link
Contributor

Description

The floating point value got converted to a string, which in turn broke arithmetic in GuidedActionsController (e.g. Vehicle::guidedModeOrbit always received NAN as altitude).

Checklist:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

The floating point value got converted to a string, which in turn broke
arithmetic in GuidedActionsController (e.g. Vehicle::guidedModeOrbit
always received NAN as altitude).
@DonLakeFlyer
Copy link
Contributor

I don't see how this is getting a NaN. I tried to reproduct a problem with guided orbit using px4 sitl and it worked fine. It went tot he correct altitude I set. How do you reproduce this?

@or-rhssk
Copy link
Contributor Author

or-rhssk commented Jan 3, 2025

Weird, just tried with fresh QGC (1e99268) and PX4 (81552fc036 / gazebo-classic_iris), altitude doesn't change after entering orbit.

orbit-altitude.mp4

Here's debug output after applying this patch:

diff --git a/src/FlightDisplay/GuidedActionsController.qml b/src/FlightDisplay/GuidedActionsController.qml
index 881742c73..0e808d820 100644
--- a/src/FlightDisplay/GuidedActionsController.qml
+++ b/src/FlightDisplay/GuidedActionsController.qml
@@ -626,7 +626,9 @@ Item {
             _activeVehicle.setCurrentMissionSequence(actionData)
             break
         case actionOrbit:
+            console.log("sliderOutputValue: " + sliderOutputValue + " / " + typeof sliderOutputValue);
             var valueInMeters = _unitsConversion.appSettingsVerticalDistanceUnitsToMeters(sliderOutputValue)
+            console.log("valueInMeters: " + valueInMeters + " / " + typeof valueInMeters);
             _activeVehicle.guidedModeOrbit(orbitMapCircle.center, orbitMapCircle.radius() * (orbitMapCircle.clockwiseRotation ? 1 : -1), _activeVehicle.altitudeRelative.rawValue + valueInMeters)
             break
         case actionLandAbort:
diff --git a/src/Vehicle/Vehicle.cc b/src/Vehicle/Vehicle.cc
index a5b7d963f..2ce200332 100644
--- a/src/Vehicle/Vehicle.cc
+++ b/src/Vehicle/Vehicle.cc
@@ -2168,6 +2168,7 @@ Vehicle::guidedModeChangeEquivalentAirspeedMetersSecond(double airspeed)
 
 void Vehicle::guidedModeOrbit(const QGeoCoordinate& centerCoord, double radius, double amslAltitude)
 {
+    qInfo() << "Vehicle::guidedModeOrbit: amslAltitude:" << amslAltitude;
     if (!orbitModeSupported()) {
         qgcApp()->showAppMessage(QStringLiteral("Orbit mode not supported by Vehicle."));
         return;

Before PR:

qml: sliderOutputValue: 121.9 / string
qml: valueInMeters: 121.9 / string
Vehicle::guidedModeOrbit: amslAltitude: nan

After PR:

qml: sliderOutputValue: 121.92 / number
qml: valueInMeters: 121.92 / number
Vehicle::guidedModeOrbit: amslAltitude: 125.171

@or-rhssk
Copy link
Contributor Author

or-rhssk commented Jan 3, 2025

For me this fixes the "nan-as-AMSL" but there appears to be another issue.

After 404324d this can happen:

  • vehicle takes off from 500m AMSL to 510m AMSL (10m relative altitude),
  • user executes orbit with slider value of 120m,
  • Vehicle::guidedModeOrbit sends 120 + 10 = 130m AMSL altitude to PX4 (well below ground).

@DonLakeFlyer
Copy link
Contributor

Yeah, you are right. I finally got the same NaN thing you are seeing. I'm trying to figure out what is going on with PX4 and orbit altitude, It doesn't respect MAV_FRAME as far as I can tell. Asking the px4 devs.

@DonLakeFlyer DonLakeFlyer mentioned this pull request Jan 3, 2025
@DonLakeFlyer
Copy link
Contributor

Replaced by other pull

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.

2 participants