search

mavlink / qgroundcontrol

Cross-platform ground control station for drones (Android, iOS, Mac OS, Linux, Windows)
http://qgroundcontrol.io
3.06k stars 3.43k forks source link

The serial number in motors setup is incorrect. #3850

Open glody opened 7 years ago

glody commented 7 years ago

Test Environment QGroundControl: Daily Build: HEAD:e6e0429 2016-07-20 18:58:20 -0400 Windows: Windows 8.1 64 Bit Laptop Linux: Ubuntu 14.04 64 Bit Laptop OS X: OS X 10.11.2 MacBook Pro Purpose: Check if the motors serial number is match with real motors Step:

  1. Lanuch QGC
  2. Open motor setup item in setup page
  3. Try to test motors one by one Excepted result: The motors serial number should march with the picture in the right side. Actual result: The serial number is incorrect. It should be 1->4->2->3 instead 1->2->3->4, or use A->B->C->D instead like Mission planner. Please view the snapshot for details motor direction
lucasdemarchi commented 7 years ago

Maybe this is on purpose?

I actually think that numbering the motors as they are and not trying to change them to ABCD or whatever is much clearer: you see the numbers on screen and you check what's rotating.

@DonLakeFlyer was the one implementing it in #3786 and could give a better answer.

DonLakeFlyer commented 7 years ago

The sliders are meant to be in order 1-N. Also I think motor numbers as opposed to letters are more common. Like this: http://ardupilot.org/copter/docs/connect-escs-and-motors.html

glody commented 7 years ago

@DonLakeFlyer But the behavior is difference with Mission planner. I setup the motors correctly in Mission planner, then test them in QGC, If I push slide bar 2 up, motor 4 rotate, If I push slide bar 3 up, motor 2 rotate, and slide bar 4 controls motor 3.

I think we can use number instead ABCD, but the slide bar should match with the motor.

DonLakeFlyer commented 7 years ago

Oh, wait I see what you are saying now. Let me look.

lucasdemarchi commented 7 years ago

@DonLakeFlyer it's because in ardupilot there is "motor order" and "motor test order". Motor test order is so the GCS can tell the user to check if going one by one the order in which motors rotate follow CW order. See: http://ardupilot.org/copter/docs/connect-escs-and-motors.html#checking-the-motor-numbering-with-the-mission-planner-motor-test

As I said I find this more confusing then helpful, particularly because I wire up my system so I know what's each channel and looking at the quad picture I think it's pretty clear. What do you think?

DonLakeFlyer commented 7 years ago

That is totally bizarre! So the implementation in the implementation of MAV_CMD_DO_MOTOR_TEST, passing in motor #1 is not really motor #1 as known to anyone that deals with motor numbering. Yikes.

DonLakeFlyer commented 7 years ago

Ok. This is wrong then. I'll need to fix for 3.0 somehow. Not easy given the fact that MAV_CMD_DO_MOTOR_TEST will fire different motors when you specify #1 depending on frame type. Seems like a pretty horrible implementation to me! I may need to ask for a different/new implementation of motor test api that makes some sense.

lucasdemarchi commented 7 years ago

@DonLakeFlyer I'd be happy to implement such a different API in ardupilot. Is there any other command that could be used for that? What does PX4 use?

DonLakeFlyer commented 7 years ago

PX4 doesn't support this yet. What would be awesome is defining a new thing that will work on both sides.

@dagar You had expressed some interest in this. Do you think you could work with Lucas on this? Then we could have a single motor test implementation for ground stations that works similar to simpler UIs like CleanFlight and also works with both PX4 and ArduPilot.

I think for now. I'm going to remove this from the 3.0 release since it's just too out of whack. I should have tested it more closely before I put it in.

dagar commented 7 years ago

Happy to help with this if there's a better idea. For PX4 I was only going to do a simple motor ordering that's consistent with PWM order, mixer, airframe metadata.

DonLakeFlyer commented 7 years ago

@rjehangir Rusty you can follow along there for ArduSub motor test implementation.

JamOoohJam commented 7 years ago

@glody What version of QGroundControl do you have installed? I have v2.9.7b and I do not see the motors tab under settings. How do I have the Motors tab shown?

capture

DonLakeFlyer commented 7 years ago

The Motors tab is currently not in QGC. It's being re-worked.

LorenzMeier commented 7 years ago

@DonLakeFlyer PX4 supports a motor test facility and I can do any required integration if you have something to test. I'd like to nail down the MAVLink spec for that in particular.

DonLakeFlyer commented 7 years ago

The Motor setup page is pretty much ready to go. It currently uses MAV_CMD_DO_MOTOR_TEST from the ardupilot dialect:

    MAV_CMD_DO_MOTOR_TEST=209, /* Mission command to perform motor test |motor sequence number (a number from 1 to max number of motors on the vehicle)| throttle type (0=throttle percentage, 1=PWM, 2=pilot throttle channel pass-through. See MOTOR_TEST_THROTTLE_TYPE enum)| throttle| timeout (in seconds)| Empty| Empty| Empty|  */

So as far as QGC is concerned the command does what it needs. The issue is with param1 which is motor number. With ArduPilot it isn't really motor number like you would normally think. It is a motor sequence number that goes around the vehicle in sequence. I need something that supports a true motor number. Given the strangeness with ArduPilot implementation it seems a new command would be in order. If that is available (and has same command params available) it would takes me 10 seconds to turn this back on for PX4 Pro.

JamOoohJam commented 7 years ago

How soon can QGC can be updated with this Motor setup page once this matter is resolved?

On Mon, Aug 15, 2016 at 9:51 PM, Don Gagne notifications@github.com wrote:

The Motor setup page is pretty much ready to go. It currently uses MAV_CMD_DO_MOTOR_TEST from the ardupilot dialect:

MAV_CMD_DO_MOTOR_TEST=209, /* Mission command to perform motor test |motor sequence number (a number from 1 to max number of motors on the vehicle)| throttle type (0=throttle percentage, 1=PWM, 2=pilot throttle channel pass-through. See MOTOR_TEST_THROTTLE_TYPE enum)| throttle| timeout (in seconds)| Empty| Empty| Empty|  */

So as far as QGC is concerned the command does what it needs. The issue is with param1 which is motor number. With ArduPilot it isn't really motor number like you would normally think. It is a motor sequence number that goes around the vehicle in sequence. I need something that supports a true motor number. Given the strangeness with ArduPilot implementation it seems a new command would be in order. If that is available (and has same command params available) it would takes me 10 seconds to turn this back on for PX4 Pro.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mavlink/qgroundcontrol/issues/3850#issuecomment-239980491, or mute the thread https://github.com/notifications/unsubscribe-auth/AD2v-rpOxbtcxvG_ay1xPXP9u9dFzSfqks5qgReFgaJpZM4JT51a .

Jamie Jackson

lucasdemarchi commented 7 years ago

@DonLakeFlyer what do you think of reusing MAV_CMD_DO_MOTOR_TEST but adding a new param 5 to tell what param 1 means? We already have that in param 2 for a similar throttle type.

LorenzMeier commented 7 years ago

@lucasdemarchi Your proposed change is bound to result in compatibility bugs because if old code is not interpreting param5 it might end up picking the wrong motor. Conversely if old GCS code is not initialising param5 to a known default it might not work correctly with new flight code and a old GCS.

Apart from that there is also an important general consideration: We need to make sure that we're not loading the spec with too much history when its not required. In this case creating a new command ID in the 16bit command space is a no-brainer.

lucasdemarchi commented 7 years ago

Ok, let's go with a new command then