Add airspeed guided action support for Ardupilot fixed wing, fix speed change slider #10716
Conversation
|
Thanks for the great contribution, as always @Davidsastresas |
|
|
||
| if (actionCode === actionTakeoff) { | ||
| guidedValueSlider.setMinVal(_activeVehicle.minimumTakeoffAltitude()) | ||
| guidedValueSlider.setValue(_activeVehicle ? _activeVehicle.minimumTakeoffAltitude() : 0) | ||
| guidedValueSlider.setDisplayText("Height") | ||
| } else if (actionCode === actionChangeSpeed) { | ||
| if (_activeVehicle.vtolInFwdFlight) { | ||
| if (_activeVehicle.vtolInFwdFlight || _activeVehicle.fixedWing) { |
There was a problem hiding this comment.
You could use _fixedWing here as well
|
Thanks @bkueng . Lets hold on just for a moment on this one, I've received reports from Arace company that the initial value for the slider is not always making sense. It should follow the airspeed setpoint, I will investigate this and report back. I will let you know with the changes for you to double check it is all right with PX4. Thanks! |
Davidsastresas
force-pushed
the
master-PR/Add_speed_change_support_AP
branch
from
b252557 to
617f2fa
Compare
Davidsastresas
force-pushed
the
master-PR/Add_speed_change_support_AP
branch
from
617f2fa to
5b05668
Compare
|
I updated using _fixedWing as @bkueng suggested. About the issue for the initial value reported by Arace, it is because on the previous implementation it was using airspeed setpoint instead of airspeed. This setpoint was varying a lot during flight, it was extremely unstable changing +- 10 m/s in a matter of seconds, so the initial value for the slider wasn't making sense. I think it makes sense to use airspeed here, and not airspeed setpoint, as the user probably wants to set airspeed based on the current airspeed reading. I made that modification and now it seems to be stable. I also spend a bit of extra time adding the speed unit translations so the slider shows in the proper speed units, I thought it was worth the effort. Based on this work, I think we could probably rework at some point the guidedValue slider to support better different usecases ( altitude, speed ), because I feel it was thought for altitude at the begining, and I don't think it is completely neat and elegant right now. But I think for the moment it should be fine. I tested the change for several units and it works fine. Please @bkueng let me know if the above changes make sense for you and I will merge. Thanks! |
Davidsastresas
force-pushed
the
master-PR/Add_speed_change_support_AP
branch
2 times, most recently
from
5b05668 to
1496046
Compare
|
This doesn't make sense to me. When I did the initial implementation if deliberately used the airspeed setpoints as it's not changing erratically as measured airspeed. Given your description above I assume that something is wrong in the calculation of the airspeed setpoint. Can you please check that? I would still prefer the slider to init with the current airspeed setpoint TECS is trying to achieve. |
|
@Davidsastresas Also I think given how much development you have been doing recently I think you should really setup PX4 simulation on your machine. I actually did the same with Ardupilot when I developed this feature just to make sure that I don't break anything for Ardupilot. You can of course always ask people who use PX4 to test it for you but in my experience it's sometimes more efficient if you just do it yourself. |
|
@RomanBapst thanks for the answer. That makes sense, thanks for clarifying. Maybe that setpoint calculation is what is wrong here. I will take a look at it at some point. About PX4, I have a setup in my environment, and I test it usually, but I am not as familiarized with PX4 as I am with Ardupilot. In ardupilot I can trace real quick in the code how a particular mavlink message affects all the vehicles, or how a particular telemetry field is generated. In PX4 I do not, so even if I can test it and confirm it works, I still prefer confirmation from anybody really familiar with it, to be 100% we don't break anything. Thanks for the suggestion! |
|
Close and reopen to get CI to run again |
|
@Davidsastresas is this ready to merge? |
There were some errors where fixed wing would not show the action, also unit conversion support for speed was added
Ultimately it sends a MAV_CMD_DO_CHANGE_SPEED in meters per second, so it is clearer if we have this name for the function, could help avoiding confusion in the future
Davidsastresas
force-pushed
the
master-PR/Add_speed_change_support_AP
branch
from
1496046 to
df23986
Compare
|
@DonLakeFlyer I rebased to current master, tested in Ardupilot, and force pushed. My only concern was about PX4 being fine with it, but as @bkueng confirmed back at the beginning it should be fine. About setpoint or not discussed with @RomanBapst , I don't have a lot of time to fix the setpoint calculation and test all of this again. As it is now it has been running in production in a custom build for some months now, Flying Ardupilot VTOLs. So if we are okay with using the actual airspeed and not the airspeed setpoint this should be ready to merge now. |
This PR adds support for Arduplane to change airspeed. The slider will show ONLY if we are in fixed wing firmware. Under actions tab, a new option available "set airspeed" will be available, and after selecting, we will have a slider to select the desired airspeed. The slider will be automatically scaled using ARSPD_FBWA_MAX and MIN parameters. This is sending a MAV_CMD_DO_CHANGE_SPEED, with:
In a different commit I also modified the guided slider so it shows "m/s" instead of "m" when we are dealing with speeds. This is hardcoded and it is not getting system unit settings, this is done on purpose as we don't have implemented unit conversion here, it is always meters/s.
I also modified the conditions in guidedactionscontroller to show this slider. It used to show the airspeed slider only if Vtol in forward flight mode, and the rest of the time will show groundspeed.
Now it will show airspeed if vtol in forward flight or fixed wing ( before this the fixed wing case was not covered ) and it will show groundspeed only if not fixedwing and we have multirrotor speed limits on firmware.
The latest changes are fine with how we are dealing with this in AP, but I am not sure if this is appropiate for PX4. My feeling is that it is, but I would appreciate another look here from anybody familiar with PX4. @mrpollo can you get somebody to double check this? @RomanBapst was the one adding initial support for this in PX4, maybe he could take a look as well. Thanks!