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

Integer initialPosition coordinates in crazyflies.yaml #487

Merged
merged 8 commits into from
Oct 1, 2021

Conversation

jpreiss
Copy link
Collaborator

@jpreiss jpreiss commented Oct 1, 2021

This PR should be "squash and merge"d into one commit.

See motivating discussion in #380.

I wanted to add a CI test that starts the server and looks for the "Parsed crazyflies.yaml successfully" message on stdout before the server fails due to no radio or CFs existing. However, the server tries to open the radio on USB before it finishes parsing.

It would be nice if the config-parsing and hardware connection were two cleanly separated phases in the server, but the problem is that we initialize the CrazyflieBroadcaster member in the initializer list of CrazyflieGroup:

, m_cfbc("radio://" + std::to_string(radio) + "/" + std::to_string(channel) + "/2M/FFE7E7E7E7")

To fix this enough for single-group testing, we would need to move the broadcaster initialization to the end of the CrazyflieGroup constructor, either by giving CrazyflieBroadcaster a separate init method or using a pointer. However, to fix it enough for multi-group testing, we would need to give CrazyflieGroup itself a separate init method, so we can finish parsing for all of the channels before attempting to connect to any of them.

I think this would be a design improvement, but maybe should be deferred until after the link-cpp version.

@jpreiss jpreiss requested a review from whoenig October 1, 2021 18:12
Copy link
Contributor

@whoenig whoenig left a comment

Choose a reason for hiding this comment

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

Switching to link-cpp won't change much on the server side (see the existing branch), so we could do your suggested changes already. I don't see it as a very high priority, though (this parsing code will change for the ROS2 port, unfortunately).

@jpreiss jpreiss merged commit 0dc475d into master Oct 1, 2021
@whoenig whoenig deleted the yaml_integer_coords branch October 3, 2021 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants