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

Add Follow me example #217

Merged
merged 16 commits into from
Jan 11, 2018
Merged

Add Follow me example #217

merged 16 commits into from
Jan 11, 2018

Conversation

shakthi-prashanth-m
Copy link
Contributor

@shakthi-prashanth-m shakthi-prashanth-m commented Jan 5, 2018

This adds Follow Me example.

  • Adds FakeLocationProvider class which mocks platform-specific location provider. It uses Boost Asio for sending periodic report of fake location update. [Note: Boost is chosen for portability across major platforms]

  • Example registers with FakeLocationProvider for location updates and sends them to Follow Me plugin.

  • Fixes naming issue in Follow Me plug-in implementation.

  • Addresses FollowMeImpl::set_config() returns immediately though Device didn't confirm #221

Note: Algorithm that generates latitude, longitude to make drone revolve is rudimentary. As this just serves as an example, it should be fine. If there's a better algorithm, please feel free to replace it.

Thanks

void FakeLocationProvider::request_location_updates(location_callback_t callback)
{
location_callback_ = callback;
timer_.async_wait(boost::bind(&FakeLocationProvider::compute_next_location, this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really really need boost for this? I tried to keep everything to plain C++11, if at all possible, I'd vote to do this without boost.

I mean do you actually need to do it async? Or could you just use a loop where you set the location?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@julianoes I really like the async approach - it is far more realistic than just using a loop in the example. Is there something other than boost that could be used here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion the example doesn't need to show how you could/should build this with async. It should only show how to use the DroneCore API. The simpler the better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@julianoes A simple example in this case can show how the API is called, but now how it should be used. I think the way it is now will save readers time and energy. Your call though.

Copy link
Contributor Author

@shakthi-prashanth-m shakthi-prashanth-m Jan 9, 2018

Choose a reason for hiding this comment

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

Thanks for the review @julianoes @hamishwillee. As discussed earlier our example should help application developers including Android, Windows, etc; I simulated approximate layout for the same. I tried simulating Android Location Provider: requestLocationUpdates.
Even Apple and Windows use this style. As mentioned earlier, I thought using Boost is portable way. I will try to check whether it can be done without Boost.

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Would be good if it could set a Config as well?

Otherwise I really like this.

@shakthi-prashanth-m
Copy link
Contributor Author

@hamishwillee Sure, I can. I think Min height is most obvious among the Follow Me configuration. What do you think ?

@hamishwillee
Copy link
Collaborator

@hamishwillee Sure, I can. I think Min height is most obvious among the Follow Me configuration. What do you think ?

Yes. Perhaps even better, both that and the FollowDirection?

@shakthi-prashanth-m
Copy link
Contributor Author

@hamishwillee I added in the example.

@julianoes
Copy link
Collaborator

@shakthi-prashanth-m if you rebase this on origin/develop and then force push I can merge it.

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.

I'm not sure why the. gitmodules and CMakeLists.txt changes are in this PR. Otherwise this is good to go in with the comment. Thanks @shakthi-prashanth-m.

Shakthi Prashanth M added 14 commits January 11, 2018 11:20
1. Added methods for error handling.
2. Improved periodic location update logic.
3. Removed redundant header `cstdint`.
4. Removed log from FollowMe plugin.
5. Improved overall FollowMe example.
This change uses Boost APIs for registering with `FakeLocationProvider`.
Removed location generator logic in example.
Also, adds stop Follow Me before landing.
Experimental changes in Follow Me integration test
was accidentally committed. This commit restore original.
Removed unused variables `curr_direction_s` & `new_direction_s` in
`FollowMeImpl::receive_param_follow_direction()`.
This is done for auto-deletion of the location object when
control goes out of scope.
1. Added methods for error handling.
2. Improved periodic location update logic.
3. Removed redundant header `cstdint`.
4. Removed log from FollowMe plugin.
5. Improved overall FollowMe example.
This change uses Boost APIs for registering with `FakeLocationProvider`.
Removed location generator logic in example.
Also, adds stop Follow Me before landing.
Experimental changes in Follow Me integration test
was accidentally committed. This commit restore original.
1. This commit address DroneCore issue #221.
2. Corresponding changes (from #1 above) in integration tests & example.
3. Replaces boost::bind by std::bind.
4. Adds important note about using Boost in the example.
5. Adds debug_str to distinguish FollowMe plugin debug messages.
@shakthi-prashanth-m
Copy link
Contributor Author

shakthi-prashanth-m commented Jan 11, 2018

@julianoes Sorry CMakeLists.txt & .gitmodules changes were accidental. I removed it. Its built successfully and ready to be merged unless you or @hamishwillee have some comments on the latest commit.

@julianoes julianoes merged commit c134de0 into develop Jan 11, 2018
@julianoes julianoes deleted the follow_me_example branch January 11, 2018 10:51
dlech pushed a commit to dlech/MAVSDK that referenced this pull request Jan 14, 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