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

Gate pose estimator #2330

Merged
merged 25 commits into from
Sep 19, 2018
Merged

Conversation

guidoAI
Copy link
Contributor

@guidoAI guidoAI commented Sep 17, 2018

Next step in bringing the autonomous drone race code into master: figuring out the position of the drone relative to the gate. The PnP solver is in principle generic, but is for now only used in conjunction with the gate detection. Some additional math functions were necessary, these have been added as well.

Copy link
Member

@gautierhattenberger gautierhattenberger left a comment

Choose a reason for hiding this comment

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

except the ABI id issue, it looks fine. Maybe you should also run the code_style script.

struct FloatVect3 pos_drone_E_vec;

// Make an identity matrix:
static struct FloatRMat I_mat;

Choose a reason for hiding this comment

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

there are a FLOAT_MAT33_ZERO and a FLOAT_MAT33_DIAG macros

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed!


// vectors in world coordinates, that will be "attached" to the world coordinates for the PnP:
struct FloatVect3 gate_vectors[4], vec_B, vec_E, p_vec, temp_vec;
p_vec.x = 0;

Choose a reason for hiding this comment

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

FLOAT_VECT3_ZERO() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check!

@@ -147,7 +148,7 @@ int snake_gate_detection(struct image_t *img, int n_samples, int min_px_size, fl

int x, y;
best_quality = 0;
best_gate.quality = 0;
(*best_gate).quality = 0;

Choose a reason for hiding this comment

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

I think best_gate->quality is cleaner but it's not that important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a next time : )

Copy link
Member

Choose a reason for hiding this comment

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

Why do things tomorrow when they can be done today? It would be easiest to revert this file and search replace best_gate. with best_gate-> (plus the greater than or equal to on line 279).

Choose a reason for hiding this comment

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

hint:

sed -i 's#(*best_gate).#best_gate->#g' snake_gate_detection.c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pthread_mutex_lock(&gate_detect_mutex);
if (detect_gate_has_new_data) {
detect_gate_has_new_data = false;
AbiSendMsgRELATIVE_LOCALIZATION(DETECT_GATE_ABI_ID, cnt++,

Choose a reason for hiding this comment

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

When sending an ABI message, it shouldn't be with the broadcast value, but with a unique ID (for this type of message) from subsystems/abi_sender_ids.h.
The ABI_BROADCASTshould only be used when binding to a message without filtering on the sender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an ABI id to the abi_sender_ids.h file

@guidoAI
Copy link
Contributor Author

guidoAI commented Sep 18, 2018

Followed (almost) all advice (see above).

Copy link
Member

@kirkscheper kirkscheper 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. Just remove DETECT_GATE_ABI_ID from the xml. The rest are comments

for (l=0; l<_i; l++){ \
printf("{"); \
for (c=0; c<_j; c++){ \
if(c<(_j-1)){printf("%f,",A[l][c]);} \
Copy link
Member

Choose a reason for hiding this comment

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

spacing here is inconsistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - corrected it now

@@ -34,6 +34,7 @@
<define name="DETECT_GATE_U_MAX" value="121" description="Maximal U of the color filter."/>
<define name="DETECT_GATE_V_MIN" value="134" description="Minimal V of the color filter."/>
<define name="DETECT_GATE_V_MAX" value="230" description="Maximal V of the color filter."/>
<define name="DETECT_GATE_ABI_ID" value="ABI_BROADCAST" description="ID of the RELATIVE_LOCALIZATION message sent"/>
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

VECT3_SDIV(vec_E, vec_E, vec_norm);

// to ned
vec_E.z = -vec_E.z;
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit strange... Flipping the sign of only one of the axis would suggest that one of the rotations is not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will tackle this in a future pull request - for now this is necessary to comply with the control code that we will work on next.

@@ -147,7 +148,7 @@ int snake_gate_detection(struct image_t *img, int n_samples, int min_px_size, fl

int x, y;
best_quality = 0;
best_gate.quality = 0;
(*best_gate).quality = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why do things tomorrow when they can be done today? It would be easiest to revert this file and search replace best_gate. with best_gate-> (plus the greater than or equal to on line 279).

(*best_gate).quality = temp_check_gate.quality;
(*best_gate).n_sides = temp_check_gate.n_sides;
memcpy(&((*best_gate).x_corners[0]), &(temp_check_gate.x_corners[0]), sizeof(int) * 4);
memcpy(&((*best_gate).y_corners[0]), &(temp_check_gate.y_corners[0]), sizeof(int) * 4);
Copy link
Member

Choose a reason for hiding this comment

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

This was also reverted to an old form, should just be best_gate->x_corners and sizeof(best_gate->x_corners) instead of sizeof(int) * 4 is more robust to possible changes to the type and size of the array being copied. Writing past the end of the array could be pretty bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strange that it is back in the old form... I have a habit of putting things off till tomorrow when they do not have to be done today, since there are other pressing deadlines tomorrow that need to be solved today. But I will do it.

Copy link
Member

Choose a reason for hiding this comment

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

Hahahaha

@gautierhattenberger gautierhattenberger merged commit 5fe023a into paparazzi:master Sep 19, 2018
shuoli-robotics pushed a commit to shuoli-robotics/paparazzi that referenced this pull request Oct 15, 2018
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.

None yet

4 participants