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

Follow me based on stereo camera #1473

Merged
merged 5 commits into from
Dec 12, 2015

Conversation

rmeertens
Copy link
Contributor

Last thing I want to add today: a follow algorithm that uses our stereo camera module.
Just tested an demonstrated it, whoo.

*
*/
/**
* @file "modules/follow_me/follow_me.c"
Copy link
Member

Choose a reason for hiding this comment

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

plz fix path

Copy link
Member

Choose a reason for hiding this comment

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

and no quotes needed...

@flixr flixr added the Module label Dec 10, 2015
@gautierhattenberger
Copy link
Member

it looks like the follow_me module is in two pull request with different names, which one should be kept ?
see #1439, some comments also apply for this PR.

@rmeertens
Copy link
Contributor Author

This one should be kept. I believe I fixed all comments right now. Let me know if there are more requests.

*
*/
/**
* @file "modules/stereocam/stereocam_follow_me/stereocam_follow_me.h"

Choose a reason for hiding this comment

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

quotes not needed

@gautierhattenberger
Copy link
Member

Can you add a few more lines to describe the module and merge/solve conflicts with paparazzi/master ?

Beside that it is okay for me

@rmeertens
Copy link
Contributor Author

Added a short description and removed the quotes!

gautierhattenberger added a commit that referenced this pull request Dec 12, 2015
Follow me based on stereo camera
@gautierhattenberger gautierhattenberger merged commit fa487f5 into paparazzi:master Dec 12, 2015
@flixr
Copy link
Member

flixr commented Dec 12, 2015

You did more than that.... added a STEREO_VELOCITY messages and changed the confs and flight plan...

@flixr
Copy link
Member

flixr commented Dec 12, 2015

Why do we have that message now?

@gautierhattenberger
Copy link
Member

since it was only TUDelft stuff I assumed they were okay with the changes. If it is not the case, @rmeertens can you provide a valid conf in a new pull request ?

@flixr
Copy link
Member

flixr commented Dec 12, 2015

That message is not used anywhere...

Please try to only include changes relevant to that specific pull request... check again before you push...
Also it's good practice to rebase/squash them in cases like this to not clutter the history needlessly...

@rmeertens
Copy link
Contributor Author

Welp, sorry guys. Looks like I already pushed the message of a module I planned to push to master next week :s next time I will also rebase. Is it ok for now to leave the message in although the module that uses it will be pushed next week?

@flixr
Copy link
Member

flixr commented Dec 12, 2015

I'll remove that accidentally added stuff for now.
Also think that we can find a better message that can also be used to report velocity from other sensors...

Also please try to document all options in the module xml (like STEREOCAM_FOLLOW_ME_USE_OPTITRACK)...

@rmeertens
Copy link
Contributor Author

Will document it in the next push to master. What message would you like to use? Currently I plot the velocities in this message, and the velocities of optitrack to be able to compare them. I would like to keep the distinction between for example this measurement and the fused INS message.

@flixr
Copy link
Member

flixr commented Dec 12, 2015

I think it would be nice if we added a slightly more generic message that basically reports a velocity estimate (e.g. also from optic flow or px4flow).
For that we could e.g. add an ID to indicate the "sensor" this is coming from....
Also some shorter field names like e.g. vx, vy vz would make sense (more consistent with other messages).
And lastly add the units and some doc on how to interpret the message (what frame are the velocities in, etc.)

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

Successfully merging this pull request may close these issues.

None yet

3 participants