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

REP on naming ABB resources #16

Merged
merged 6 commits into from
Mar 9, 2017

Conversation

gavanderhoorn
Copy link
Member

@gavanderhoorn gavanderhoorn commented Feb 26, 2017

As per subject.

Refer to ros-industrial/abb#75 for context.

Link to rendered version.

@gavanderhoorn
Copy link
Member Author

@Samsagax

@gavanderhoorn
Copy link
Member Author

Question: should there be a section documenting some alternative templates that have been discussed but did not make it?

@Samsagax
Copy link

@gavanderhoorn I've read it. Itlooks fine to me. regarding your last question, it doesn't seem necessary, we should link to the GH discussion instead.

@gavanderhoorn
Copy link
Member Author

gavanderhoorn commented Feb 28, 2017

Thanks for your comments.

We could link to the discussion, but some REPs include an "design alternatives" section directly in the document itself. That way future readers would not have to refer to external resources (that might disappear).

I'm not opposed to linking to the discussion for that though. ros-industrial/abb#75 is already linked as a reference.

@gavanderhoorn
Copy link
Member Author

🛎 @shaun-edwards @Levi-Armstrong ;)

@gavanderhoorn
Copy link
Member Author

After some additional +1s, the REP should get an Active state and should be assigned a nr.

I think 7 is free.

Copy link
Member

@JeremyZoss JeremyZoss left a comment

Choose a reason for hiding this comment

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

+1

The inclusion of payload is messy (file replication with trivial differences), but maintains compliance with ABB's naming conventions, which is probably a net positive.

My (weak) preference would have been to round payload to the nearest 1kg, but the current 0-prefix is also intuitive. Similarly, I would have preferred to leave off payload/reach except where required to distinguish between current variants. But I can see how the current approach is more future-proof.

Consider adding examples of typical naming/layout of artefacts (e.g. _support packages). It's non-obvious, even though I'm sure your description is complete. Levi's example was helpful in this regard.

Also, some guidance on expectations for how to minimize duplication (e.g. in xacros, launch files, etc.) would be welcome. This might be more suited as a wiki page, though, since it tends to be cross-platform.

Sorry for the late comments. Overall, looks good to me.

@gavanderhoorn
Copy link
Member Author

gavanderhoorn commented Mar 8, 2017

@JeremyZoss wrote:

The inclusion of payload is messy (file replication with trivial differences), but maintains compliance with ABB's naming conventions, which is probably a net positive.

yes, payload was a difficult choice. We discussed this in ros-industrial/abb#75. Reason I included it is indeed because we stick to ABBs conventions (but also for consistency, see below).

And not described in this document, but to avoid unnecessary duplication two variants with the same reach but different payload (like the 2400-12/1.55 and -20/1.55, see the readme) can just use each others urdfs/meshes (ie: use the abb_irb2400_12_155 model for the 20Kg variant). At least until MoveIt or other planners become payload-aware.

My (weak) preference would have been to round payload to the nearest 1kg, but the current 0-prefix is also intuitive. Similarly, I would have preferred to leave off payload/reach except where required to distinguish between current variants. But I can see how the current approach is more future-proof.

We thought about this, but decided against it as it would mean that we'd be less consistent: for example for an abb_irb6600_225 (I just made this up) is 225 the reach or the payload? This might be clear for someone who knows the series well, but I want things to be clear for all users.

I agree that shorter names are always preferable, but in this case I felt that avoiding potential ambiguity was more important.

Consider adding examples of typical naming/layout of artefacts (e.g. _support packages). It's non-obvious, even though I'm sure your description is complete. Levi's example was helpful in this regard.

Which example are you referring to here?

Also, some guidance on expectations for how to minimize duplication (e.g. in xacros, launch files, etc.) would be welcome. This might be more suited as a wiki page, though, since it tends to be cross-platform.

Yes, so initially I wanted to add them to this REP, but thinking about it such rules / guidelines wouldn't be ABB specific at all. So I did not include them, with the idea that a REP on artefact naming and support package layout would be more efficient as it could be 'shared' by all Mfgs we support.

That REP could then point to this one (and others for the different Mfgs) for ABB-specific rules on naming.

Perhaps I should make it clearer that this REP really is only about converting product names. I thought the title would convey that already :).

Sorry for the late comments. Overall, looks good to me.

No problem. Thanks for the review.

I think I will add that Design Alternatives section after all, which would address exactly the points that you raised.

@JeremyZoss
Copy link
Member

I was referring to Levi's comment on Issue #75.

@gavanderhoorn
Copy link
Member Author

Btw: quite some time ago I started on a REP documenting ROS-I support pkg layout: Layout of ROS Industrial manufacturer support repositories. That was (and still is) intended to contain the "examples of typical naming/layout of artefacts".

@Levi-Armstrong
Copy link
Member

👍

@shaun-edwards
Copy link
Member

Don't wait on me guys...pretty overwhelmed these days.

@gavanderhoorn
Copy link
Member Author

I've added a section that discusses the "ignore payload" alternative.

@gavanderhoorn
Copy link
Member Author

I've made this REP 7 and synced creation and posting dates.

If I don't receive any more comments by 8pm CET, I'll mark the REP as active and merge it.

@gavanderhoorn gavanderhoorn changed the title Draft: REP on naming ABB resources REP on naming ABB resources Mar 9, 2017
@gavanderhoorn gavanderhoorn merged commit 69b5fad into ros-industrial:master Mar 9, 2017
@gavanderhoorn gavanderhoorn deleted the abb_naming branch March 9, 2017 21:08
@gavanderhoorn
Copy link
Member Author

Thanks for all the input @Levi-Armstrong, @Samsagax, @shaun-edwards and @JeremyZoss.

Appreciated 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants