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

Use GLOBAL_POSITION_INT instead of VFR_HUD to set altitude and speeds… #489

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

anbello
Copy link

@anbello anbello commented Nov 3, 2018

… in copter like in plane

@anbello
Copy link
Author

anbello commented Nov 3, 2018

This is necessary because after a commit on 29 May 2018 Arducopter always return height above MSL in VFR_HUD message whereas GLOBAL_POSITION_INT return relative alt.
This also resolve a problem with Follow Me on Tower.

@peterbarker
Copy link

My bad for killing this, sorry.

I'm a touch confused as to why you've implemented this in org.droidplanner.services.android.impl.core.drone.autopilot.apm.ArduCopter rather than in org.droidplanner.services.android.impl.core.drone.autopilot.apm.ArduPilot

After all, this problem was caused by the effort to make all of the vehicles behave the same over MAVLink as far as we can!

By-the-by, there's a low-bandwidth google group we formed to try to coordinate this sort of thing with GCS authors: https://groups.google.com/forum/#!forum/ardupilot-gcs

@anbello
Copy link
Author

anbello commented Nov 3, 2018

My bad for killing this, sorry.

Don't worry that commit did the right thing

I'm a touch confused as to why you've implemented this in org.droidplanner.services.android.impl.core.drone.autopilot.apm.ArduCopter rather than in org.droidplanner.services.android.impl.core.drone.autopilot.apm.ArduPilot

In part for the lack of programming skill and in part because I see that in ArduPlane was implemented in the same way

After all, this problem was caused by the effort to make all of the vehicles behave the same over MAVLink as far as we can!

By-the-by, there's a low-bandwidth google group we formed to try to coordinate this sort of thing with GCS authors: https://groups.google.com/forum/#!forum/ardupilot-gcs

Thanks, I will try to follow this group

@billbonney
Copy link
Collaborator

billbonney commented Nov 4, 2018

I agree this should be refactored from ArduPlane code into Ardupilot base class. Then any ArduPIlot variant will have the same behaviour. That said older variants would not be able to use the library. It should really be dependent on wether it is new or old FW.

Do you think you can add a check for the FW version and modify the behaviour?

@anbello
Copy link
Author

anbello commented Nov 4, 2018

I agree this should be refactored from ArduPlane code into Ardupilot base class. Then any ArduPIlot variant will have the same behaviour. That said older variants would not be able to use the library. It should really be dependent on wether it is new or old FW.

Do you think you can add a check for the FW version and modify the behaviour?

I can move the new code from ArduCopter to base class ArduPilot. ArduPlane already override the two methods processVfrHud and processGlobalPositionInt before my PR.

I don't think there is the need to check for FW version, the info about altitude and speed are sent to FC with setAltitudeGroundAndAirSpeeds method, this method, before my PR, was called by processVfrHud and now by processGlobalPositionInt, FC receive the right info independently from FW version.
But I could be wrong, I will test this.

@anbello anbello force-pushed the develop branch 3 times, most recently from 73dfa16 to b93c505 Compare November 15, 2018 12:08
@anbello
Copy link
Author

anbello commented Nov 15, 2018

I did the changes in ArduPilot base class. Do you think I did it the right way?

@@ -512,10 +513,28 @@ private void checkControlSensorsHealth(msg_sys_status sysStatus) {
}

protected void processVfrHud(msg_vfr_hud vfrHud) {

Choose a reason for hiding this comment

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

Could just kill this entirely.

Copy link
Author

Choose a reason for hiding this comment

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

OK done.

Copy link

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Haven't tested it, but this looks to be the right shape.

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.

3 participants