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

server yaml file substitute with temporary files #454

Merged
merged 5 commits into from
Apr 9, 2024
Merged

Conversation

knmcguire
Copy link
Collaborator

@knmcguire knmcguire commented Mar 18, 2024

I figured out a solution that kind of works here without much change on the user side. So if the user just does this:

ros2 launch crazyflie launch.py backend:=sim

Nothing changes really, except for there is a tmp file that is made especially for the server

but if the user defines a yaml file with different parameters like here

ros2 launch crazyflie launch.py backend:=sim server_yaml_file:='test.yaml'

then the server is initialized with that yaml file instead, which should include all the parameters necessary for that server.

I don't like that a temporary yaml file needs to be made in order for this to work. On the other hand, it also provides a template which users can use to make an modify their own yaml file.

@knmcguire
Copy link
Collaborator Author

@Khaledwahba1994 or @akmaralAW, since wolfgang will not be available for this, perhaps you can express your opinion of what you think of this strategy?

@akmaralAW
Copy link

Hi @knmcguire!

did I understand correctly that this file (server_yaml_file:='test.yaml') is similar to server.yaml, and you want to be able to support easy, user-friendly configuration of some parameters ?

@knmcguire
Copy link
Collaborator Author

well it's actually a bit more complicated, since the launch.py file appends more parameters to the server like the crazyflies and some things for mocap. What I proposed here is that the launch file makes a temporary yaml file with all of those things combined (so the 'true' server yaml file). These are the parameters than can be replaced by the users' choosing.

This was done because it was otherwise difficult to have the script choose between a dictionary (what the serverparams was inputed for the server node)
or the content of a file.

@knmcguire
Copy link
Collaborator Author

mind that this is just a draft still and not fully distributed to all the other .yaml files. Just looking for an opinion here but don't merge it yet obviously :)

@akmaralAW
Copy link

To me it makes sense to have this separate, easy-to-change server file (if parameters are specific to use cases and need to be changed frequently). And in case no change is needed, then server_yaml_file:='test.yaml' can be skipped and we still can run the command.

@knmcguire knmcguire marked this pull request as ready for review April 9, 2024 09:14
@knmcguire
Copy link
Collaborator Author

knmcguire commented Apr 9, 2024

I've implemented the same strategy for mocap as well, and the teleop node now receives the teleop.yaml file directly (or an external one). The PR is no longer a draft now.

One comment though on this approach, I'm not sure if the temp yaml file fix will be something that works if we release this as an official ROS binary. Perhaps it might be better to have another node that changes the parameters after the nodes has been initialized, but let's cross that bridge once we get there :)

@knmcguire knmcguire changed the title server yaml file substitute try 2 server yaml file substitute with temporary files Apr 9, 2024
@whoenig whoenig self-requested a review April 9, 2024 19:09
@whoenig whoenig merged commit bf00cb7 into main Apr 9, 2024
4 checks passed
@whoenig whoenig deleted the default_yaml_folder2 branch April 9, 2024 19:12
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.

3 participants