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

rosmon_core: Adding fix for rosparam parsing to equivilate with undocumented behavior of roslaunch #118

Merged
merged 2 commits into from
May 24, 2020

Conversation

Cartoonman
Copy link
Contributor

@Cartoonman Cartoonman commented May 18, 2020

This was a really insidious and potentially controversial patch we needed to make to rosmon to handle some undocumented behavior of roslaunch.

The tl;dr is that roslaunch parses all <rosparam> tags before it parses all <param> tags. This is in contrast to what rosmon does:

	for(TiXmlNode* n = element->FirstChild(); n; n = n->NextSibling())
	{
                 .
                 .
                 .
		if(e->ValueStr() == "param")
			parseParam(e, ctx, PARAM_IN_NODE);
		else if(e->ValueStr() == "rosparam")
			parseROSParam(e, ctx);
		else if(e->ValueStr() == "remap")
			parseRemap(e, ctx);
		else if(e->ValueStr() == "env")
        }

This manifested itself as a really annoying bug when we were working with mavros. Specifically this launch file of theirs made the above difference matter.

We had setups running this file on roslaunch work with custom param inputs properly overriding the mavros frame_id namespaces. But when run on rosmon the custom param namespaces were not propagated, since in the above logic, the param fields got overridden by rosparam anyway.

I believe I did attempt a patch of simply moving rosparam above param but this did not solve the problem since the for-loop would still loop around params before reaching rosparam tags

The solution isn't great but it matches how roslaunch works, and shouldn't affect existing users.

@xqms
Copy link
Owner

xqms commented May 18, 2020

Great, more roslaunch craziness :D

I don't particularly like roslaunch's behavior, but we have to match it. I guess almost all rosmon users use it in a dual-stack fashion together with rostest / roslaunch. Discrepancies are really bad.

Apart from the undocumented semantics, is there some other thing you don't like about your solution? I.e. do you think there is some technical issue which might bite us later on?

@Cartoonman
Copy link
Contributor Author

Apart from the undocumented semantics, is there some other thing you don't like about your solution?

No, the only thing that irked me was that effectively I have to do two passes through the launch context to pull all rosparam tags, then fetch all the rest. This is a simple fix though, and trying to fold this into one for-loop is likely to increase the complexity more than necessary. Functionality-wise this should work fine and not affect downstream users.

@xqms xqms merged commit 3cb47ea into xqms:master May 24, 2020
@xqms
Copy link
Owner

xqms commented May 24, 2020

Thanks!

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