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

Fix xtext grammar and reference node in process #194

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ipa-rwu
Copy link
Contributor

@ipa-rwu ipa-rwu commented May 10, 2023

Exmaple can be fund here: ipa-nhg/ros-model-examples#8

@ipa-nhg
Copy link
Member

ipa-nhg commented May 15, 2023

@ipa-rwu Thanks a lot for the PR and the changes. Good job!

Comments:

std_msgs:
 specs:
 msg: ColorRGBA
 	message:
 		float32 r
 		float32 g
 		float32 b
 		float32 a
 msg: Header
 	message:
 		int32 seq 
 		time stamp 
 		string frame_id
std_srvs:
	specs:
	 srv: Trigger
	 	response:
	 		bool sucess
	 		string message

Your examples are quite different -> https://github.com/ipa-nhg/ros-model-examples/pull/8/files#diff-94d4e10765013b88643ad70600a187b9035d5103d84c8c1428eb2476947a1114

I will cherry pick the changes that are fine for now and create new separate PRs to discuss the others.

ipa-nhg added a commit that referenced this pull request May 16, 2023
;

RosParameter returns RosParameter:
Name=EString':'
PreListElement name=EString':' from=[ros::Parameter|EString]
Copy link
Member

Choose a reason for hiding this comment

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

PreListElement was defined in another commit, I will remove it from here. This commit is about uncapitalize the definition of the attributes

END
;

ComponentRef returns ComponentRef:
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed if the metamodel is correctly defined, see: d1ce353#r113516155

END
name=EString':'
BEGIN
('nodes:' '['nodes+=ComponentRef (',' nodes+=ComponentRef)+']')?
Copy link
Member

Choose a reason for hiding this comment

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

this can be simplified to:

nodes+=[RosNode| EString]

Copy link
Member

Choose a reason for hiding this comment

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

The grammar is wrong, this implementation doesn't allow a process with a single node. It should be:

('nodes:' '['nodes+=ComponentRef (',' nodes+=ComponentRef)*']')?

@@ -72,44 +73,43 @@ Artifact returns Artifact:

Node returns Node:
'node:' name=RosNames
BEGIN
Copy link
Member

Choose a reason for hiding this comment

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

was this "BEGIN" removed intentionally? why?
The interfaces should be defined under the node.

@ipa-nhg
Copy link
Member

ipa-nhg commented May 16, 2023

This PR can be closed, it is superseded by:

#195 #196 #197

@ipa-nhg ipa-nhg marked this pull request as draft May 16, 2023 11:07
@ipa-nhg ipa-nhg force-pushed the main branch 3 times, most recently from f84a682 to c9fb502 Compare October 2, 2023 16:30
@ipa-nhg ipa-nhg force-pushed the main branch 2 times, most recently from 39190a9 to 5837d31 Compare November 14, 2023 10:33
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