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

ExtLibs: ArduPilot: Mavlink: try and pull AP firmware type from version string #2972

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

IamPete1
Copy link
Member

@IamPete1 IamPete1 commented Oct 9, 2022

With more configurable types it is harder to tell the vehicle firmware type from the mavlink type enum. This changes to use the version string if it is available.

For example on a fresh setup there is no frame class setup, so it defaults to generic and then we endup with no param descriptions or mode names.

@peterbarker
Copy link
Contributor

This might not work for some people who have changed the announcements. I guess those guys will just not get this new detection.

This might do bad things in the case that the version string contains a (e.g.) company name and that company name contains one of the other vehicles.

I'm generally not in favour of any GCS parsing our statustexts - it's a bad pattern as we can inadvertently break things by seemingly innocuous changes in ArduPilot (e.g. my recent attempts to stop sending through the MCU ID as hex digits in a statustext would break MissionPlanner).

I wonder if there's a better way to differentiate the vehicles than the statustext.... if it is fundamentally the APM_BUILD_TYPE we are wishing to convey, perhaps we could put that into a mavlink message somewhere.

@IamPete1
Copy link
Member Author

IamPete1 commented Oct 9, 2022

I could use longer strings and make it case sensitive. So from plane to ArduPlane V, that would minimize the false positives.

New mavlink messages would be great, but this is a pain point now on current firmware.

@IamPete1 IamPete1 force-pushed the Type_From_version_string branch from 0ee26a0 to c025e97 Compare October 9, 2022 23:46
@IamPete1
Copy link
Member Author

IamPete1 commented Oct 9, 2022

Changed to case-sensitive, starts-with and the full firmware string excluding the version numbers. Still works and should be much less likely to false positive.

We could also move this to only try if the mav type fails, but I think going forward there will be more cross over of mav type between vehicle builds.

@IamPete1 IamPete1 force-pushed the Type_From_version_string branch from c025e97 to ee76384 Compare October 9, 2022 23:52
@tridge
Copy link
Contributor

tridge commented Oct 12, 2022

it is pretty common that vendors change that version string

@IamPete1
Copy link
Member Author

IamPete1 commented Oct 12, 2022

I'm not really sure how else we can do better (without mavlink changes). MAV_TYPE is no longer a good way to tell the firmware vehicle type. In the mean time this change helps some most of the time.

@IamPete1
Copy link
Member Author

IamPete1 commented Nov 3, 2022

This PR would fix such issues as this:
https://discuss.ardupilot.org/t/bicopter-no-tuning-tab/92626

@davidbuzz
Copy link
Contributor

not perfect, but an inprovement

@davidbuzz
Copy link
Contributor

@meee1 ?

@peterbarker
Copy link
Contributor

We probably do this until we can convey it in mavlink properly.

@IamPete1 IamPete1 force-pushed the Type_From_version_string branch from ee76384 to e7f1e34 Compare December 17, 2022 18:49
@IamPete1
Copy link
Member Author

@meee1 I have rebased this, we discussed on the call and the conclusion was that this was better than what we have now, but we will try and add a firmware version into a mavlink message somewhere, however that will only work once that AP fix is done and in the wild, in the mean time this fix helps most on current firmware although it does not work in every case, such as where manufacturers have changed the firmware name.

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.

6 participants