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.

Sign up for GitHub

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

Jump to bottom

Adding messages for sequences #65

Merged
merged 10 commits into from
Apr 23, 2020
Merged

Adding messages for sequences #65

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
63aa1d5
adding messages for sequences
ct2034 Feb 21, 2020
f0218dc
updating license headers
ct2034 Feb 24, 2020
732cfd6
add MotionSequenceResponse
jschleicher Mar 6, 2020
f5d30af
rename result trajectories -> plural
jschleicher Mar 9, 2020
ff2aaad
drop license headers that clutter the message files
jschleicher Mar 9, 2020
08a03d8
extended inline documentation
ct2034 Apr 20, 2020
390d083
renaming commands to request
ct2034 Apr 21, 2020
763bd08
comments in srv file
ct2034 Apr 21, 2020
02d000f
fixing typos
ct2034 Apr 23, 2020
3fd6dd5
only singular start state in sequence planning response
ct2034 Apr 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion action/MoveGroupSequence.action
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
# A list of motion commands
# A list of motion commands with one for each section of the sequence
ct2034 marked this conversation as resolved.
Show resolved Hide resolved
MotionSequenceRequest request

# Planning options
PlanningOptions planning_options
---

# Response of the request containing informaiton on all sections of the sequence
ct2034 marked this conversation as resolved.
Show resolved Hide resolved
MotionSequenceResponse response

---
Expand Down
6 changes: 4 additions & 2 deletions msg/MotionSequenceItem.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# The plan request
# The plan request for this item.
# It is the planning request for this segment of the sequence, as if it were a solitary motion.
MotionPlanRequest req

# The blend radius in meter (used between this and the next command), 0 means no blending
# To blend between sequence items, the motion may be smoothed using a circular motion.
# The blend radius of the circle between this and the next command, where 0 means no blending.
float64 blend_radius
3 changes: 2 additions & 1 deletion msg/MotionSequenceRequest.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
# List of motion planning request with blend_radius
# List of motion planning request with a blend_radius for each.
# In the response of the planner all of these will be executable as one sequence.
MotionSequenceItem[] items
Copy link
Member

Choose a reason for hiding this comment

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

I would probably remove this abstraction and directly include lists of MotionPlanRequest and blend_radius

Copy link

Choose a reason for hiding this comment

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

Can you explain why? Having the two together in a structure enforces a 1-to-1 mapping of radii to requests.

With separate lists that is note as easily enforced.

Copy link
Member

@henningkayser henningkayser Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's not really a 1-to-1 mapping since the last blend_radius is redundant. Also, message types usually have their own semantics with multiple use cases, but here the semantics already implies the use case of being an array item. So, while this abstraction removes a sanity check it makes the message structure a little more complicated than necessary. There are many messages that just use a convention for array lengths (i.e. Marker, RobotTrajectory..).

Copy link

@gavanderhoorn gavanderhoorn Apr 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's not really a 1-to-1 mapping since the last blend_radius is redundant.

may be, but that's not a very strong argument to not do it.

Also, message types usually have their own semantics with multiple use cases, but here the semantics already implies the use case of being an array item.

Not sure I follow this: the semantics of MotionSequenceRequest or something else? Because the name does not necessarily imply that it's an array to me (but: not a native speaker).

So, while this abstraction removes a sanity check it makes the message structure a little more complicated than necessary.

Huh? If by abstraction you refer to the item struct, then I'm not sure how that removes a check.

It also doesn't seem very unreasonable for a MotionSequenceRequest to contain items, which themselves contain information about each item respectively. From that nested structure it's very clear that the information in those items belongs together.

While if there were just two plain arrays at the level of the request, interpretation of the message is actually less clear as the relationship between the information in the first array and the second array would only be apparent from and encoded in the code interpreting the message (ie: comes from an external and essentially unconnected source).

There are many messages that just use a convention for array lengths (i.e. Marker, RobotTrajectory..).

that may be, but this seems a bit like a tu-quoque: the fact that this is done in other messages does not necessarily make it a good pattern to apply here.

6 changes: 3 additions & 3 deletions srv/GetMotionSequence.srv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# A list of motion commands
MotionSequenceRequest commands
# Planning request witha list of motion commands
ct2034 marked this conversation as resolved.
Show resolved Hide resolved
MotionSequenceRequest request

---

# Response to the planning request
MotionSequenceResponse response