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

weathervane: added configuration option via paramter #10883

Closed
wants to merge 2 commits into from
Closed

Conversation

RomanBapst
Copy link
Contributor

@RomanBapst RomanBapst commented Nov 20, 2018

@sanderux Requires the weathervane feature to be disabled for takeoff, landing and loiter.

LorenzMeier
LorenzMeier previously approved these changes Nov 21, 2018
@sanderux
Copy link
Member

Just to clarify, we just need it disabled for takeoff as we see danger in having the vehicle transition in an arbitrary direction and it does not seem to have so much trouble during takeoff.

We do require it for all other modes

@RomanBapst
Copy link
Contributor Author

Please do not merge

Signed-off-by: Roman <bapstroman@gmail.com>
- general enable / disable bit
- auto takeoff enable / disable bit

Signed-off-by: Roman <bapstroman@gmail.com>
@RomanBapst
Copy link
Contributor Author

@sanderux @dagar I updated the configuration possibilities. You can now enable weathervane generally for all position controlled modes, regardless of manual or mission. Additionally you can disable weathervane independently for auto takeoff. We can extend the bitmask in the future should we require more configuration options.

return (bool)(_wv_config.get() & MASK_EN);
}

bool WeatherVane::should_run_in_mission(uint8_t position_setpoint_type)
Copy link
Member

Choose a reason for hiding this comment

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

What about only making this call on position setpoint updates?

@@ -44,8 +44,12 @@
*
* @boolean
* @group Multicopter Position Control
* @min 0
* @max 3
* @bit 0 Enabled.
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this isn't exactly obvious that "Enabled" covers everything except for takeoff. Any input @hamishwillee?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RomanBapst I prefer "Enable" over "Enabled".

  1. I assume that if bit 0 is unset, it doesn't matter what the other bits are set to? i.e. to have on takeoff you must have both bit 0 and 1?
  2. You say this is active for all position control modes by default, except takeoff. I assume that means it is also active for landing and loiter by default?

Assuming that, how about:

/**
 * Enable weathervane and configure use in optional flight modes.
 *
 * When enabled, weathervane is active for most position control modes.
 * The additional bits can be used to enable weathervane for the remaining modes.
 * 
 * @boolean
 * @group Multicopter Position Control
 * @min 0
 * @max 3
 * @bit 0 Enable.
 * @bit 1 Enable for auto takeoff.
 */
PARAM_DEFINE_INT32(WV_CONFIG, 3);

We will also need to update https://docs.px4.io/en/config_vtol/vtol_weathervane.html

@sanderux
Copy link
Member

It seems to be getting a bit convoluted.
@dagar: The use case is that we need weathervane except during takeoff as we do not allow the vehicle to takeoff in an arbitrary direction for safety reasons. Also, during takeoff there is enough thrust as not to have an issue and the users are clear that the nose should be pointing into the wind.

To me it seems the simplest solution is:
WV_NAV: enable WV for WP navigation or POSCTL
WV_LOITER: enable WV for loiter
WV_TAKEOFF: enable for takeoff
WV_LAND: enable during landing

Either in bitmask or simple params

I do not understand the added value of WV during manual modes. seems dangerous.

@dagar
Copy link
Member

dagar commented Nov 26, 2018

What happened to setting it on the mission item? One way this could be kept simple is to have a single controller default weathervane setting (VTOL_TRANSITION_HEADING_VEHICLE_DEFAULT), but then set VTOL_TRANSITION_HEADING_TAKEOFF in your MAV_CMD_NAV_VTOL_TAKEOFF.

https://mavlink.io/en/messages/common.html#MAV_CMD_NAV_VTOL_TAKEOFF
image

https://mavlink.io/en/messages/common.html#VTOL_TRANSITION_HEADING
image

@LorenzMeier
Copy link
Member

@RomanBapst The suggestion from Daniel makes a lot of sense.

@stale
Copy link

stale bot commented Mar 2, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@hamishwillee
Copy link
Contributor

@RomanBapst Isn't this still valid?

@stale stale bot removed the Admin: Wont fix label Mar 4, 2019
@RomanBapst
Copy link
Contributor Author

@hamishwillee yes, still valid

@stale
Copy link

stale bot commented Jul 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Jul 24, 2019

Closing as stale.

@stale stale bot closed this Jul 24, 2019
@julianoes julianoes reopened this Jul 25, 2019
@stale stale bot removed the Admin: Wont fix label Jul 25, 2019
@julianoes
Copy link
Contributor

@RomanBapst what's the status here?

@stale
Copy link

stale bot commented Oct 23, 2019

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Oct 23, 2019
@sfuhrer
Copy link
Contributor

sfuhrer commented Nov 9, 2019

From my point okay to close this. We've addressed the main issue (arbitrary direction of transition) with #12630. It does now always align with direction of VTOL_TAKEOFF before transitioning.
Being able to set a transition heading in the waypoint would still be nice, I've therefore opened this issue: #12660.
Beside this, I don't see it necessary to turn off weathervane for manual flight mode or landing.

@stale
Copy link

stale bot commented Feb 7, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@RomanBapst
Copy link
Contributor Author

Fixed long time ago.

@RomanBapst RomanBapst closed this Apr 28, 2020
@RomanBapst RomanBapst deleted the pr-wv branch April 28, 2020 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants