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

MC_Pos_Control: takeoff bug constrain hover_thrust_estimate #21380

Closed

Conversation

hendjoshsr71
Copy link
Member

This is the version of PR that points to main instead of 1.13

@junwoo091400
Copy link
Contributor

@hendjoshsr71 Could you do make format and commit results to fix the format checker CI that is failing?

@junwoo091400
Copy link
Contributor

junwoo091400 commented Mar 29, 2023

Tailsitter SITL is falining again: https://github.com/PX4/PX4-Autopilot/actions/runs/4544468878/jobs/8011289156?pr=21380

However, the previous commit in the main branch didn't have a failure. Therefore it could be an introduced behavior from this PR? The log shown however is more related to airspeed selector / battery 🤔

image

Log: https://logs.px4.io/plot_app?log=5f0c0ee0-6739-4302-b48b-ddb7dae3eb22

Seems like the failsafe triggered disarm 🤔
image

@mrpollo
Copy link
Contributor

mrpollo commented Mar 29, 2023

@junwoo091400 I restarted the failed test just to verify it wasn't a fluke

@hendjoshsr71 hendjoshsr71 force-pushed the pr/main/pos_control_max_thrust_bug branch from bcf263a to 3758021 Compare April 5, 2023 21:58
@hendjoshsr71
Copy link
Member Author

hendjoshsr71 commented Apr 5, 2023

@hendjoshsr71 Could you do make format and commit results to fix the format checker CI that is failing?

Done! Though I had to fix up how the auto formatter "fixed" things. It looked pretty weird.

Copy link
Member

@bresch bresch left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of merging 2 functions in one and split it internally using a boolean parameter. Especially when reading the function call, I think it would be nicer to have something like setHoverThrustAndUpdateIntegral() than setHoverThrust(true).

@hendjoshsr71
Copy link
Member Author

hendjoshsr71 commented Apr 11, 2023

I'm not a big fan of merging 2 functions in one and split it internally using a boolean parameter. Especially when reading the function call, I think it would be nicer to have something like setHoverThrustAndUpdateIntegral() than setHoverThrust(true).

Agree. I was trying to acheive what @dagar wanted in terms of only one path for these constraints.

If either you or Daniel would like to push my repo your preference to achieve that goal feel free. That would probably be easier than me going back and forth.?

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-community-q-a-april-19-2023/31700/4

@bresch bresch mentioned this pull request Apr 26, 2023
@bresch bresch closed this Apr 26, 2023
@bresch
Copy link
Member

bresch commented Apr 26, 2023

replaced by #21512 . Thanks @hendjoshsr71 for the fix. I rebased, made some minor changes and did another PR with it

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.

6 participants