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

More specific Airframe type support for better Airframe Reference #19686

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

junwoo091400
Copy link
Contributor

@junwoo091400 junwoo091400 commented May 20, 2022

Describe problem solved by this pull request
As discussed in the Actuators Document Improvement PR in User Guide: PX4/PX4-user_guide#1858 (comment), the current airframe reference document doesn't separate out the airframes accurately.

image

For example, the 'VTOL Tiltrotor' Airframe Reference has a 6-rotor VTOL diagram that doesn't match with the E-Flite Convergence, that is currently under the same "type" in the doc.

Describe your solution

  1. Add more @type categories to the Airframe files to specify the Airframes that is currently in the wrong 'type'
  2. Add the SVG diagrams in the User Doc for those specific @type categories, which will be a separate PR

TODOs

  • Cleanup Airframe Source Parser to clarify the 'airframe' object (which was named 'param' before)
  • Rename E-Flite Convergence Airframe's @type to a more specific type, decoupled from a generic "VTOL Tiltrotor"

Test data / coverage
Tested functionality of the px_process_airframes.py script by: make airframe_metadata command

Additional context
🤞 If there are any other airframes that are not true to the airframe type diagram in the Airframe Reference, please let me know so that I can add the appropriate @type attribute to those airframes!

@junwoo091400
Copy link
Contributor Author

@sfuhrer
Copy link
Contributor

sfuhrer commented May 24, 2022

@sfuhrer Any idea for a better @type for the Convergence airframe? https://github.com/PX4/PX4-Autopilot/blob/master/ROMFS/px4fmu_common/init.d/airframes/13012_convergence#L5

Not really, currently we don't have a way to indicate the number of motors of a tiltrotor.

Given that we want to purge most of the airframes once we have control allocation in place (see #19596 (comment)), I wonder how much effort it is worth to spend on this front. The idea is that we only keep a very small selection of pre-configured airframes and otherwise let users define their own airframe with the new allocation UI in QGC.

@junwoo091400
Copy link
Contributor Author

This would be a useful refactor in preparation for the upcoming Frame component metadata generation in PX4 side.

It is just renaming / making the script more human readable, so no functional changes.

Could we get this in? @bkueng

@PX4 PX4 deleted a comment from sfuhrer Nov 2, 2022

def GetName(self):
"""
Get parameter group name
Get parameter group type
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 fix the whole comment? parameter -> airframe
And why does GetName return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! That's also what I wondered back then I think 😆

This script in general has bad naming conventions / implicit, but I will go over them again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@junwoo091400 junwoo091400 force-pushed the pr_airframe_parser_improvements branch from 25ad074 to 1443f68 Compare November 9, 2022 22:26
The srcparser.py is specific to each use case (e.g. Airframes, Parameters, px4events, etc as in Tool/* folders).
Therefore it is confusing to have the px_process_airframes.py script handle concept of airframes under the generic name 'params'.

This improves readability and sets the baseground for implementing more specific vehicle type supports, as mentioned in PX4/PX4-user_guide#1858 (comment)
@junwoo091400 junwoo091400 force-pushed the pr_airframe_parser_improvements branch from 1443f68 to 31d7418 Compare November 9, 2022 22:31
@junwoo091400
Copy link
Contributor Author

Had rebase error, now properly fixed!

@bkueng bkueng merged commit 855eb42 into PX4:main Nov 10, 2022
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.

3 participants