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

mavlink: improve comments about message forwarding #11323

Merged
merged 1 commit into from
Feb 5, 2019
Merged

mavlink: improve comments about message forwarding #11323

merged 1 commit into from
Feb 5, 2019

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Jan 28, 2019 •
edited by AuterionWrikeBot
Loading

This removes the confusing ugly magic number of 233 introduced as part of #7670.

Also, the convoluted if is cleaned up using 3 separate bools with some comments to explain what's going on.

This should not change anything function-wise except that the flight controller could now potentially also use system ID 233 and not break forwarding.

LorenzMeier
LorenzMeier previously approved these changes Jan 28, 2019
View reviewed changes
sanderux
sanderux previously approved these changes Jan 28, 2019
View reviewed changes
@LorenzMeier
Copy link
Member

LorenzMeier commented Jan 28, 2019 •
edited
Loading

@julianoes Build failure: ../../src/modules/mavlink/mavlink_main.cpp 542../../src/modules/mavlink/mavlink_main.cpp: In static member function ‘static void Mavlink::forward_message(const mavlink_message_t*, Mavlink*)’: 543../../src/modules/mavlink/mavlink_main.cpp:574:4: error: expected ‘,’ or ‘;’ before ‘if’ 544 if (target_system_id_ok && target_component_id_ok && heartbeat_check_ok) { 545 ^~ 546compilation terminated due to -Wfatal-errors. 547[523/702] Building CXX object src/modules/mavlink/CMakeFiles/modules__mavlink.dir/mavlink_log_handler.cpp.o 548

@julianoes
Copy link
Contributor Author

Wow, what was I thinking I could push with not enough semicolons 😄.

@julianoes julianoes dismissed stale reviews from sanderux and LorenzMeier via dfeb6e0 January 29, 2019 08:51
@julianoes julianoes force-pushed the fix-mavlink-forwarding branch from f6f415e to dfeb6e0 Compare January 29, 2019 08:51
@julianoes
mavlink: improve comments about message forwarding
6197831 
This removes the confusing ugly magic number of 233 introduced as part
of #7670.

Also, the convoluted if is cleaned up using 3 separate bools with some
comments to explain what's going on.

This should not change anything function-wise except that the flight
controller could now potentially also use system ID 233 and not break
forwarding.
@julianoes julianoes force-pushed the fix-mavlink-forwarding branch from dfeb6e0 to 6197831 Compare January 31, 2019 08:19
@julianoes
Copy link
Contributor Author

What's going on with CI?

FAILED: NuttX/nuttx_copy.stamp 
cd /tmp/jenkins/workspace/e-compile_fix-mavlink-forwarding && cp -aRfl platforms/nuttx/NuttX/nuttx build/auav_x21_default/NuttX && cmake -E touch /tmp/jenkins/workspace/e-compile_fix-mavlink-forwarding/build/auav_x21_default/NuttX/nuttx_copy.stamp
cp: cannot create hard link 'build/auav_x21_default/NuttX/nuttx/libs/libc/sched/task_startup.c' to 'platforms/nuttx/NuttX/nuttx/libs/libc/sched/task_startup.c': No such file or directory
ninja: build stopped: subcommand failed.
Makefile:171: recipe for target 'auav_x21_default' failed

@dagar
Copy link
Member

dagar commented Jan 31, 2019

What's going on with CI?

FAILED: NuttX/nuttx_copy.stamp 
cd /tmp/jenkins/workspace/e-compile_fix-mavlink-forwarding && cp -aRfl platforms/nuttx/NuttX/nuttx build/auav_x21_default/NuttX && cmake -E touch /tmp/jenkins/workspace/e-compile_fix-mavlink-forwarding/build/auav_x21_default/NuttX/nuttx_copy.stamp
cp: cannot create hard link 'build/auav_x21_default/NuttX/nuttx/libs/libc/sched/task_startup.c' to 'platforms/nuttx/NuttX/nuttx/libs/libc/sched/task_startup.c': No such file or directory
ninja: build stopped: subcommand failed.
Makefile:171: recipe for target 'auav_x21_default' failed

Not sure, but I restated the job.

@julianoes
Copy link
Contributor Author

Please review again since this passed CI now.

dagar
dagar approved these changes Feb 5, 2019
View reviewed changes
Copy link
Member

@dagar dagar left a comment

Choose a reason for hiding this comment

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

Nice cleanup.

dagar
dagar reviewed Feb 5, 2019
View reviewed changes
src/modules/mavlink/mavlink_main.cpp
@@ -543,7 +543,7 @@ Mavlink::forward_message(const mavlink_message_t *msg, Mavlink *self)
const mavlink_msg_entry_t *meta = mavlink_get_msg_entry(msg->msgid);

int target_system_id = 0;
int target_component_id = 233;
Copy link
Member

Choose a reason for hiding this comment

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

What is/was 233?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nobody knows, not even @sanderux who put it there initially.

Copy link
Member

Choose a reason for hiding this comment

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

I seem to remember this was some "all components" magic number but i cannot find any reference to it anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

"All components" is 0 - https://mavlink.io/en/messages/common.html#MAV_COMP_ID_ALL

This is empty and unreserved. If it needs to be all components then it should be changed to zero. If it means something else then it will need a new component ID. As it is, we're likely to eventually overwrite this, and you will end up with unexpected behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Yes i realize that, i just can't imagine i made one up, nor can i imagine lorenz would have approved it if i did.

@dagar dagar added this to the Release v1.9.0 milestone Feb 5, 2019
@dagar dagar merged commit b17c0a1 into master Feb 5, 2019
@dagar dagar deleted the fix-mavlink-forwarding branch February 5, 2019 15:33
@julianoes
Copy link
Contributor Author

Thx @dagar!

@weekly-digest weekly-digest bot mentioned this pull request Feb 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dagar dagar dagar approved these changes

@hamishwillee hamishwillee hamishwillee left review comments

@sanderux sanderux sanderux approved these changes

@LorenzMeier LorenzMeier LorenzMeier left review comments

@RomanBapst RomanBapst Awaiting requested review from RomanBapst

Assignees
No one assigned
Labels
Admin: Cleanup (housekeeping) 🧹
Projects
None yet
Milestone
Release v1.9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@julianoes @LorenzMeier @dagar @hamishwillee @sanderux