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: Add YAML merge key parsing functionality. #117

Merged
merged 3 commits into from
May 24, 2020

Conversation

Cartoonman
Copy link
Contributor

Addresses #106.

Hello again @xqms. Over the past year I made a number of improvements and fixes to rosmon in one of my projects. I'll be rolling out these changes into PR's over the next few days.

This is a patch that enables YAML merge key functionality and somewhat addresses #106. While I am aware that yaml-cpp is technically the responsible party, their MR has still not yet been resolved, and YAML proper is partially to blame. Given that we really needed this, I made this patch for my team's purposes and successfully saved a lot of time our our end being able to use YAML anchors.

Here is an example yaml file I ran this patch over with success:

common: &common
    flux_a: 2.4
    type_f: "vv_2de119"
    stable: true
    u_val: 3.0e-5

base_params: &base_params
    class1:
        x_var: 3.4
        y_var: 1.3
        z_var: 0.0
        w_var: 44.5
    class2:
        x_var: 3.3
        y_var: 1.3
        z_var: 0.09
        w_var: 35.3
    <<: *common

node5:
    <<: *base_params
    class1:
        y_var: 2.0
    id: 5
    

node4:
    <<: *base_params
    class2:
        y_var: 2.0
    id: 4

I cross-compared the functionality of this and roslaunch and there is only one major difference in implementation (and I would argue this implementation is superior).

rosmon result

This is the output from rosparam dump with the patched rosmon:

base_params:
  class1: {w_var: 44.5, x_var: 3.4, y_var: 1.3, z_var: 0.0}
  class2: {w_var: 35.3, x_var: 3.3, y_var: 1.3, z_var: 0.09}
  flux_a: 2.4
  stable: true
  type_f: vv_2de119
  u_val: 3.0e-05
common: {flux_a: 2.4, stable: true, type_f: vv_2de119, u_val: 3.0e-05}
node4:
  class1: {w_var: 44.5, x_var: 3.4, y_var: 1.3, z_var: 0.0}
  class2: {w_var: 35.3, x_var: 3.3, y_var: 2.0, z_var: 0.09}
  flux_a: 2.4
  id: 4
  stable: true
  type_f: vv_2de119
  u_val: 3.0e-05
node5:
  class1: {w_var: 44.5, x_var: 3.4, y_var: 2.0, z_var: 0.0}
  class2: {w_var: 35.3, x_var: 3.3, y_var: 1.3, z_var: 0.09}
  flux_a: 2.4
  id: 5
  stable: true
  type_f: vv_2de119
  u_val: 3.0e-05

roslaunch result

This is the output fromrosparam dump with the standard roslaunch on melodic:

base_params:
  class1: {w_var: 44.5, x_var: 3.4, y_var: 1.3, z_var: 0.0}
  class2: {w_var: 35.3, x_var: 3.3, y_var: 1.3, z_var: 0.09}
  flux_a: 2.4
  stable: true
  type_f: vv_2de119
  u_val: 3.0e-05
common: {flux_a: 2.4, stable: true, type_f: vv_2de119, u_val: 3.0e-05}
node4:
  class1: {w_var: 44.5, x_var: 3.4, y_var: 1.3, z_var: 0.0}
  class2: {y_var: 2.0}
  flux_a: 2.4
  id: 4
  stable: true
  type_f: vv_2de119
  u_val: 3.0e-05
node5:
  class1: {y_var: 2.0}
  class2: {w_var: 35.3, x_var: 3.3, y_var: 1.3, z_var: 0.09}
  flux_a: 2.4
  id: 5
  stable: true
  type_f: vv_2de119
  u_val: 3.0e-05

Difference

You'll notice that the override of

    class2:
        y_var: 2.0

of a nested yaml struct in roslaunch completely replaces the structure, whereas in rosmon, it simply replaces the corresponding value in the struct with the new value, leaving all other values in place. I'd argue this is how this aught to be, but you can also make an argument that this 'doesn't align' with roslaunch, and therefore is 'unsatisfactory'. I'll leave this up to you.

Let me know your thoughts!

@xqms
Copy link
Owner

xqms commented May 18, 2020

This looks awesome! I hadn't thought the necessary patch could be that short ;)

I'll have to look into it a little bit deeper - and I would definitely like a unit test for this thing. Here's a good location: https://github.com/xqms/rosmon/blob/master/rosmon_core/test/xml/test_rosparam.cpp
If you have time to turn your example from above into a test, I'd appreciate it. Otherwise I'll probably have some time next weekend.

Regarding nested structs & overrides: Since this is not "official" YAML, I guess there is no correct way to do it. While so far my motto has been "if in doubt, stay compatible with roslaunch",
I guess detecting overrides and deleting all resulting parameters would increase the complexity a lot. So I'm fine with this behavior for now - it's at least better than not supporting anchors at all.

Thanks for the PR - and I'm looking forward to your other contributions :)

@Cartoonman
Copy link
Contributor Author

@xqms Added a testcase for testing merge keys and the various edge cases.

I actually didn't know how best to view the tests but I just used catkin run_tests and looked at the verbose output to see if there was any error coming from catch_ros. This passes the tests.

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