Skip to content
This repository has been archived by the owner on Feb 19, 2021. It is now read-only.

Fix compile time and runtime issues with testapp #174

Merged
merged 4 commits into from
Feb 27, 2019
Merged

Conversation

lbegani
Copy link

@lbegani lbegani commented Jun 4, 2018

No description provided.

@lbegani
Copy link
Author

lbegani commented Jun 6, 2018

Fixes the issue #173 and #172
@hamishwillee

@lbegani lbegani changed the title WIP : Fix compile time and runtime issues with testapp Fix compile time and runtime issues with testapp Jun 6, 2018
sleep(intervel + 1);
log_info("Enter the interval in which images to be taken");
int interval;
scanf("%d", &interval);

Choose a reason for hiding this comment

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

Using scanf is vulnerable and discouraged in C++. If user provides non-digit input, then app behavior is undefined. So, I suggest to use std::cin. I suggest to apply throughout the test application.

@@ -40,6 +40,8 @@

using namespace std;

void discovercam(void *cntx);

Choose a reason for hiding this comment

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

What about making it a public static method of Drone ?

@@ -1,5 +1,5 @@
/*
* This file is part of the Camera Streaming Daemon project
* This file is part of the Dronecode Camera Manager

Choose a reason for hiding this comment

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

I suggest to add a brief description of what the app does. Sample here.

@julianoes
Copy link
Contributor

@lbegani should this still go in?

@@ -18,7 +18,7 @@

/**

@brief This is a test application to test mavlink messages in the Camera Streaming Daemon.
@brief This is a test application to test mavlink imessages in the Dronecode Camera Manager.
Copy link
Contributor

Choose a reason for hiding this comment

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

imessages ?

@lbegani
Copy link
Author

lbegani commented Feb 21, 2019

@lbegani should this still go in?

For now, we can close this by merging it. Improvements can go in later.

@julianoes
Copy link
Contributor

To me it looks like @shakthi-prashanth-m 's comments should be addressed though. I might add them before merging.

@MatejFranceskin MatejFranceskin merged commit 27869fc into master Feb 27, 2019
@MatejFranceskin MatejFranceskin deleted the fix-testapp branch February 27, 2019 20:39
Copy link
Contributor

@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.

Looks good.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants