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

eliminate global static GCS_MAVLINK::send_home_all #6554

Closed
wants to merge 7 commits into from

Conversation

peterbarker
Copy link
Contributor

No description provided.

@OXINARF
Copy link
Member

OXINARF commented Jul 9, 2017

LGTM, but I'll make two notes before merging:

  • behavior changes as it will no longer only send home to active channels
  • a new chan method wasn't needed, only making the current one be const (instead of the new one which is const and returns a const reference).

@peterbarker
Copy link
Contributor Author

@OXINARF a const function returning a reference must return a const reference, which is unfortunate if you would like to fiddle with the returned value....

I've changed the GCS_MAVLink patch to take into account the active channel mask. That mask is going to die in future, replaced with a .active or .active() methodcall. Similarly with the streaming channel mask.

@amilcarlucas
Copy link
Contributor

Sorry, I do not get the advantage of this PR. Can you elaborate Please ?

@magicrub
Copy link
Contributor

@peterbarker has had this task in the queue for a long time. We've been wanting to change the whole GCS messaging system to use a singleton instead of all the libraries and vehicles reaching back and forth over the GCS_MAVLink:: const reference which is a bit ugly. He has a series of PRs lined up to totally remove the GCS const refs which were held off to make sure it doesn't conflict with the copter release

@OXINARF
Copy link
Member

OXINARF commented Jul 10, 2017

a const function returning a reference must return a const reference, which is unfortunate if you would like to fiddle with the returned value....

@peterbarker Ah you are right and it makes sense, you can't mark the method const if you give the caller the chance to change the internal state of the object. Learning every day! 🙂

@magicrub Why was this marked as DevCallTopic? This looks ready to go in, there is no change in behavior I can see.

} \
} while (0);

void GCS::send_home(const Location &home) const
Copy link
Member

Choose a reason for hiding this comment

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

Given we already have a GCS.cpp file should this go there? I think it would be better, easier to find.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Hadn't been created when I originally made this PR, I think.

Moved!

@OXINARF
Copy link
Member

OXINARF commented Jul 12, 2017

Merged, thanks! I've changed the commit message of the last commit here by adding "to GCS_Dummy".

@OXINARF OXINARF closed this Jul 12, 2017
@peterbarker peterbarker deleted the gcs-send-home branch July 20, 2017 06:50
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.

4 participants