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

Remove Qt5 Compatibility Module #11259

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Conversation

HTRamsey
Copy link
Collaborator

Resolves #11081. Removes all usage of the Qt5 Compatibility Module which I believe was made due to licensing issues with libraries Qt was using for the GraphicalEffects.
This also fixed a couple of issues:

  • APMParameterMetaData::getParameterMetaDataVersionInfo was not getting the version numbers correctly
  • APMFirmwarePlugin::_handleIncomingStatusText was never setting _coaxialMotors

All Qml visuals are 100% identical except for the ADSB shadow because I am terrible at graphical design. Here is a before and after:
DropShadow_Before
DropShadow_After

@DonLakeFlyer Hopefully you can just tune a couple of the settings here to make it look better. I try to avoid touching Qml files since you're much better at it but I needed to in this circumstance.

return QString(b);
mavlink_statustext_t data;
mavlink_msg_statustext_decode(message, &data);
QString text = QString::fromStdString(data.text);
Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree here. The previous implementation was there for a reason. data.text is not necessarily 0 terminated by spec, so this can read garbage memory.

What about using QString::fromLocal8Bit() which allows to pass the max length as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure I had a reason, but I don't remember why I even touched this part in this PR. I'll have to go back and check.

Copy link
Collaborator Author

@HTRamsey HTRamsey Mar 29, 2024

Choose a reason for hiding this comment

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

So maybe this and the other fix should be separate PRs, but the reason I did this was because when changing the QRegExp in _handleIncomingStatusText, I noticed it was impossible for _coaxialMotors to be set as true because of the trailing 0's, as shown in this image. The change I had made removed all of the trailing 0's, but I guess your solution would too?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. A statustext is a string closed with \0 or not if the full length is used. If you construct QString from it, it will read past the array into garbage and we need to protect against that. I'm not sure how that matters with a regexp? Also, this looks like statustext is potentially somewhat abused for this use case 🤔?

Copy link
Collaborator Author

@HTRamsey HTRamsey Apr 1, 2024

Choose a reason for hiding this comment

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

So it's not really related to QRegExp, which is why I probably should have made a separate issue/PR, but I tried to fix it when I was changing this code anyways. The problem the way things are is that you cannot do an equality comparison of frameType (which is always 50 bytes) directly to "Y6", "OCTA_QUAD", etc. That comparison will never be true. So something needs to be changed with the frameType comparison, either by dropping the trailing zeros or using substring or something like that. I changed the _getMessageText function such that it drops trailing zeros when converting to QString.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, what about if you convert both to QString and then compare? That would drop trailing 0s, right?

Copy link
Collaborator Author

@HTRamsey HTRamsey Apr 1, 2024

Choose a reason for hiding this comment

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

They already are both QStrings. The thing is in _getMessageText, the \0 termination is always added at the very end. So you will always have a bunch of trailing zeros until the 50th byte.

Edit: Oh I get what the original thinking was. The \0 is added at the 51st byte in case the 50th byte is not a termination. Typically there will be two terminations if the original statustext string is less than 50 bytes. Unfortunately it seems that QString will ignore the first termination, and always terminate the string at the 51st byte.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, I guess we should copy into length 51 char array, then do a strlen and then use QString::fromLocal8Bit() with the length we have?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seemed to work.

@HTRamsey HTRamsey force-pushed the dev-qt5compat branch 4 times, most recently from ff2a6be to 5cad8e4 Compare April 5, 2024 16:42
static const QRegularExpression regMap( "\"*https?://mt\\D?\\d..*/vt\\?lyrs=m@(\\d*)", QRegularExpression::CaseInsensitiveOption );
const QRegularExpressionMatch matchMap = regMap.match( html );
if( matchMap.hasMatch() )
{
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: The QGC coding style is to not use an extra line for all of these these. They should be on the line above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I switch between working on QGC and FreeRTOS a lot, and that was FreeRTOS coding style

@DonLakeFlyer DonLakeFlyer merged commit db2d26d into mavlink:master Apr 5, 2024
10 checks passed
@HTRamsey HTRamsey deleted the dev-qt5compat branch April 5, 2024 23:17
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.

Remove usage of Qt5Compat
3 participants