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

QGC Parameters #265

Merged
merged 6 commits into from
Dec 5, 2023
Merged

QGC Parameters #265

merged 6 commits into from
Dec 5, 2023

Conversation

afwilkin
Copy link
Contributor

@afwilkin afwilkin commented Nov 1, 2023

When you want to download a parameter file... it can not be directly uploaded into QGC. This givces you the option to download the QGC compatible one. This PR was based off of @farhangnaderi PR here: #264

@afwilkin
Copy link
Contributor Author

afwilkin commented Nov 2, 2023

@bkueng let me know if there is any changes you would like me to make

@farhangnaderi
Copy link

Thanks Andrew! I appreciate it!

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

There's also a couple of CI problems. We should start to split the long method into individual methods by download type.

@@ -56,7 +56,9 @@ <h1>
<div class="dropdown-menu">
<a class="dropdown-item" href="download?log={{ log_id }}">Log File</a>
<a class="dropdown-item" href="download?log={{ log_id }}&type=1" target="_blank">Parameters</a>
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this option (but keep the implementation), and just offer the QGC versions.


delimiter = ' '
for param_key in param_keys:
self.write("1")
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment that this is the sysid (and compid below)?

self.write('\n')
except:
pass

else:
Copy link
Member

Choose a reason for hiding this comment

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

This else case was part of the elif download_type == '3': # download the non-default parameters case above and needs to be copied up to that case again, and here it needs to be adjusted for the QGC format.

@afwilkin
Copy link
Contributor Author

Apologies Beat, I did not see you had reviewed this. I will fix these

@afwilkin
Copy link
Contributor Author

@bkueng I addressed your comments. I know you mentioned there are some CI problems that we will incur. Mainly the long method. what is the method length that will pass the CICD test so I know how to modify the code?

@bkueng
Copy link
Member

bkueng commented Nov 28, 2023

I needed to approve so the checks are running. They're queued now and should run shortly. This is from a previous run: https://github.com/PX4/flight_review/actions/runs/6726581710/job/18290754736#step:5:14.
You can run the checks locally with ./run_pylint.sh

@afwilkin
Copy link
Contributor Author

afwilkin commented Dec 4, 2023

@bkueng made changes.

Also added something to the bottom of the readme to let others know to run ./run_pylint before making PR to help with CICD.

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Thanks

@bkueng bkueng merged commit 31a4915 into PX4:main Dec 5, 2023
4 checks passed
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