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.

Sign up for GitHub

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

Jump to bottom

Add Status Text #733

Merged
merged 29 commits into from
May 7, 2019
Merged

Add Status Text #733

merged 29 commits into from
May 7, 2019

Conversation

douglaswsilva
Copy link
Collaborator

@douglaswsilva douglaswsilva commented Apr 26, 2019 •
edited
Loading

Get status text through the SDK.

@douglaswsilva douglaswsilva changed the title Add Mavlink Status Text [WIP] Add Mavlink Status Text Apr 26, 2019
@douglaswsilva
Copy link
Collaborator Author

douglaswsilva commented Apr 26, 2019 •
edited
Loading

  • Will wait until Add mavlink messages support. MAVSDK-Proto#82 gets merged.
  • There are a couple of Keywords Anotacao in the code which means I'd like your guys' input.
  • I ran fix_style.h, but I'm afraid my computer is still not working correctly, so might have messed up some indentation, I'll most certainly need you guys to run it as well once we have made all PR changes.

@douglaswsilva douglaswsilva changed the title [WIP] Add Mavlink Status Text [WIP] Add Status Text Apr 27, 2019
JonasVautherin
JonasVautherin reviewed Apr 29, 2019
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Looking pretty good to me! The notes are probably more for Julian, I'm waiting to learn from his answers 👀.

plugins/telemetry/telemetry_impl.cpp Outdated Show resolved Hide resolved
plugins/telemetry/telemetry_impl.cpp Outdated Show resolved Hide resolved
plugins/telemetry/telemetry_impl.cpp Show resolved Hide resolved
julianoes
julianoes reviewed Apr 29, 2019
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

A couple of comments but overall looks good. Thanks a lot!

core/mavlink_parameters.cpp Outdated Show resolved Hide resolved
integration_tests/telemetry_async.cpp Outdated Show resolved Hide resolved
integration_tests/telemetry_async.cpp Outdated Show resolved Hide resolved
plugins/telemetry/include/plugins/telemetry/telemetry.h Show resolved Hide resolved
plugins/telemetry/include/plugins/telemetry/telemetry.h Outdated Show resolved Hide resolved
plugins/telemetry/telemetry_impl.cpp Outdated Show resolved Hide resolved
plugins/telemetry/telemetry_impl.cpp Outdated Show resolved Hide resolved
plugins/telemetry/telemetry_impl.cpp Show resolved Hide resolved
plugins/telemetry/telemetry_impl.cpp Show resolved Hide resolved
plugins/telemetry/telemetry_impl.cpp Outdated Show resolved Hide resolved
julianoes
julianoes reviewed Apr 30, 2019
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Something with the build fails now, can you check?

plugins/telemetry/telemetry_impl.h Outdated Show resolved Hide resolved
@douglaswsilva douglaswsilva changed the title [WIP] Add Status Text Add Status Text May 2, 2019
@douglaswsilva
Copy link
Collaborator Author

douglaswsilva commented May 2, 2019 •
edited
Loading

@julianoes I just tested locally and it builds fine. Just updated everything from develop, let's see if it will build. I think the problem is that you guys need to run fix.style from your computer. =/

@julianoes
Copy link
Collaborator

@douglaswsilva ok I've squashed your commits and removed the unrelated style changes (I have the same problem on Arch Linux though).

Let's see if CI is happy.

@julianoes
Copy link
Collaborator

@douglaswsilva ok it doesn't build yet if you include the backend:

make BUILD_BACKEND=1

@julianoes
backend/proto: update submodule
43f988d 
This includes the addition for StatusText.
@julianoes
Copy link
Collaborator

Presumably the proto submodule was not updated. I've checked out master of the submodule and commited that.

JonasVautherin
JonasVautherin requested changes May 3, 2019
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Missing some doc, and then I believe we're good to go!

Generating docs for nested compound dronecode_sdk::Offboard::Attit/tmp/jenkins/workspace/Dronecode_DronecodeSDK_PR-733/install/include/dronecode_sdk/plugins/telemetry/telemetry.h:147: warning: Member type (variable) of class dronecode_sdk::Telemetry::StatusText is not documented.

@douglaswsilva
Merge branch 'douglas/mavlinkDebugLogs' of https://github.com/3drobot…
36c6d22 
…ics/DronecodeSDK into douglas/mavlinkDebugLogs

# Conflicts:
#	plugins/telemetry/include/plugins/telemetry/telemetry.h
JonasVautherin
JonasVautherin previously approved these changes May 3, 2019
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot @douglaswsilva!

I fixed the style, updated the proto submodule and merged develop. It passes all unit tests (backend + core) on my end.

@JonasVautherin
Copy link
Collaborator

Oh, still missing documentation:

Generating docs for nested compound dronecode_sdk::Offboard::Attit/tmp/jenkins/workspace/Dronecode_DronecodeSDK_PR-733/install/include/dronecode_sdk/plugins/telemetry/telemetry.h:147: warning: Member type (variable) of class dronecode_sdk::Telemetry::StatusText is not documented.

@douglaswsilva
Copy link
Collaborator Author

@JonasVautherin I added the description (actually it seems like one of you guys had done it already), but let me know if still doesn't work.

@JonasVautherin JonasVautherin mentioned this pull request May 6, 2019
Closed
JonasVautherin
JonasVautherin approved these changes May 6, 2019
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @douglaswsilva!

@JonasVautherin JonasVautherin merged commit b706135 into mavlink:develop May 7, 2019
@douglaswsilva douglaswsilva mentioned this pull request May 7, 2019
Closed
@douglaswsilva douglaswsilva deleted the douglas/mavlinkDebugLogs branch October 21, 2019 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianoes julianoes julianoes left review comments

@JonasVautherin JonasVautherin JonasVautherin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@douglaswsilva @julianoes @JonasVautherin