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

First version of the DSL grammar with yaml format #181

Open
wants to merge 31 commits into
base: 2.0.0_stable
Choose a base branch
from

Conversation

ipa-nhg
Copy link
Member

@ipa-nhg ipa-nhg commented Nov 7, 2022

This PR holds the changes done for the Xtext grammar implementation to move to a "yaml" format style.

The joy node example before looks like this:

PackageSet {
  CatkinPackage joy {
    Artifact joy_node { Node { name joy_node
      Publishers {
        Publisher { name "joy" message "sensor_msgs.Joy" },
        Publisher { name "diagnostics" message "diagnostic_msgs.DiagnosticArray" }
  }}
}}}

With these changes is converted to:

joy:
  artifacts:
    joy_node:
      node: joy_node
        publishers:
          joy:
            type:"sensor_msgs.Joy"
          diagnostics:
            type: "diagnostic_msgs.DiagnosticArray"

ipa-nhg and others added 19 commits November 10, 2021 11:32
…erface

Create GUI for configuring deployment
…ks, the package of the name was not corresponding to the one set for package.xml
… to avoid authentification issue while cloning the GitHub repository
@ipa-nhg
Copy link
Member Author

ipa-nhg commented Nov 8, 2022

@fmrico @ipa-hsd FYI

@ipa-nhg ipa-nhg force-pushed the YamlMigration branch 3 times, most recently from 1c913b9 to e4a8cce Compare November 16, 2022 12:17
@fmrico
Copy link

fmrico commented Nov 22, 2022

LGTM, but I had no time yet to test in my computer :)

@ipa-nhg ipa-nhg mentioned this pull request Jan 19, 2023
Copy link

@hellantos hellantos left a comment

Choose a reason for hiding this comment

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

Just some things I noticed during usage.

Comment on lines +112 to +131
('serviceserver:'
BEGIN
serviceserver+=ServiceServer*
END
)|
('serviceclient:'
BEGIN
serviceclient+=ServiceClient*
END
)|
('actionserver:'
BEGIN
actionserver+=ActionServer*
END
)|
('actionclient:'
BEGIN
actionclient+=ActionClient*
END
)|

Choose a reason for hiding this comment

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

Probably make this plural.

Suggested change
('serviceserver:'
BEGIN
serviceserver+=ServiceServer*
END
)|
('serviceclient:'
BEGIN
serviceclient+=ServiceClient*
END
)|
('actionserver:'
BEGIN
actionserver+=ActionServer*
END
)|
('actionclient:'
BEGIN
actionclient+=ActionClient*
END
)|
('serviceservers:'
BEGIN
serviceserver+=ServiceServer*
END
)|
('serviceclients:'
BEGIN
serviceclient+=ServiceClient*
END
)|
('actionservers:'
BEGIN
actionserver+=ActionServer*
END
)|
('actionclients:'
BEGIN
actionclient+=ActionClient*
END
)|

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot for the suggestion, I will add this change. It makes sense! :)

Comment on lines +98 to +102
Node returns Node:
'node:' name=RosNames
BEGIN
(
('publishers:'

Choose a reason for hiding this comment

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

One option is to a dd name tag. This will force it to be directly after "node:".

Suggested change
Node returns Node:
'node:' name=RosNames
BEGIN
(
('publishers:'
Node returns Node:
'node:'
BEGIN
'name:' name=RosNames
(
('publishers:'

This seems to also be a problem in other places. We should go with a general solution. I think adding a name tag is probably the easiest and most general solution.

Comment on lines +148 to +169
'msg:'name=(EString|'Header'|'String')
BEGIN
'message:' (BEGIN message=MessageDefinition END)?
END
;

ServiceSpec returns ServiceSpec:
{ServiceSpec}
'srv:'name=EString
BEGIN
'request:' (BEGIN request=MessageDefinition END)?
'response:' (BEGIN response=MessageDefinition END)?
END;

ActionSpec returns ActionSpec:
{ActionSpec}
'ActionSpec'
name=EString
'{'
('goal' goal=MessageDefinition)?
('result' result=MessageDefinition)?
('feedback' feedback=MessageDefinition)?
'}';
'action:'name=EString
BEGIN
'goal:' (BEGIN goal=MessageDefinition END)?
'result:' (BEGIN result=MessageDefinition END)?
'feedback:' (BEGIN feedback=MessageDefinition END)?
END;

Choose a reason for hiding this comment

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

Seems to have the same YAML issues. Probbaly add name tag as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, I made already the fix for other branch: https://github.com/ipa320/ros-model/blob/7d09541be7f5339c09fb79ccecfe32abf21ec2ed/plugins/de.fraunhofer.ipa.ros.xtext/src/de/fraunhofer/ipa/ros/Ros.xtext#L121-L144

I think we came up to exactly the same solution :)

This is part of https://github.com/ipa-nhg/ros-model/tree/YamlMigrationSystem ‼️ that branch is not passing the tests

@hellantos
Copy link

On adding arguments
I had this idea of a structure, but it might be to inflated.

        arguments: 
          named_args:
            controller_manager: 
              indicator: "-c"
              type: "string"
            parameter_file:
              indicator: "-p"
              type: "string"
            controller_type:
              indicator: "-t"
              type: "string"
            controller_manager_timeout:
              indicator: "-t"
              type: "double"
          flag_args:
            load_only:
              indicator: "--load-only"
            stopped:
              indicator: "--stopped"
            inactive:
              indicator: "--inactive"
          positional_args:
            - controller_name:
                type: "string"

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.

4 participants