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

Adds support for the extended messages for the REPI0001 according to the... #8

Merged

Conversation

thiagodefreitas
Copy link
Contributor

... current tests with MOTOMAN

@gavanderhoorn
Copy link
Member

Hi, this looks great. Two points:

  1. Could you make a capture with some example traffic available somewhere? Hard to validate these changes without something to actually dissect :)
  2. There are quite some whitespace / formatting errors introduced by this commit: the current source is tab-indented (perhaps not the best choice, but still). It would be very nice if you could change your commit to use tabs as well.

Merging this would close #7.

@thiagodefreitas
Copy link
Contributor Author

@gavanderhoorn
Copy link
Member

Works for me, +1.

gavanderhoorn added a commit that referenced this pull request Jul 6, 2014
Adds support for the extended messages for the REPI0001 according to the...
@gavanderhoorn gavanderhoorn merged commit e11af82 into ros-industrial:groovy-devel Jul 6, 2014
@gavanderhoorn
Copy link
Member

Thanks for the contribution.


i=i+1

until i > 4
Copy link
Member

Choose a reason for hiding this comment

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

I only just noticed this now: is this a hard coded number of groups? Any reason not to use the value of the number of groups field?

@gavanderhoorn
Copy link
Member

Also: are these messages Motoman specific? They are similar to the messages defined in REP-I0001, but have different names, and some fields are missing.

@gavanderhoorn
Copy link
Member

Ping?

@thiagodefreitas
Copy link
Contributor Author

Hello,
sorry for the late reply, but due to personal reasons, I have been offline the last days.
The amount of groups is currently limited on the MotoPlus side to 4 groups, that is why it is hardcoded there.

Concerning the other question, the sequence and name, also come from the current implementation. We have a functional version of the complete chain ROS+Motoplus now and are making final adjustments and should be able to make it public soon, almost confirmed that untill the end of this month. We are trying to make it as compliant as possible to the REP, but there was the need for some decisions there.

As there is already plenty of interest on the driver, I have decided to update the dissector as it was very helpful for me and may be to other "users" once we release the driver, but I can make a MOTOMAN specific section, if this would make more sense for you.

Thanks.

@gavanderhoorn
Copy link
Member

I'm just trying to figure out how to view these changes. I was under the impression that they were basically an implementation of the proposed REP-I0001, but after looking closer at the fieldnames and message structures, it would appear that that is not the case.

If the REP will be updated with the changes you mention were done for the Motoman driver, then the current code would be fine. If they will remain specific to the Motoman driver, I'd like to refactor them (I'd be surprised though: mfg specific msgs are expected to be assigned identifiers from the 1000+ range).

As to the hard coded group limit, I'll recode it to use the information from the appropriate field (even if it is currently limited to 4 in the Motoplus code, I see no reason to not use the field if it is already there. It will just be 4 in your specific case).

@thiagodefreitas
Copy link
Contributor Author

I am currently working on some refinements on the driver. Should be done by the end of the week.

I can send you an email with some more complete information and implementation details on the beginning of next week.

@gavanderhoorn
Copy link
Member

Ping?

@thiagodefreitas
Copy link
Contributor Author

@gavanderhoorn : Sorry for the late reply.

I had to wait for some permissions to make the pull requests. Can you get the infos from there? I can provide any information you need anyway.

I have opened 2 pull requests:
ros-industrial/motoman#42
ros-industrial/industrial_core#82

Thanks. :)

@gavanderhoorn
Copy link
Member

Well, my questions weren't so much about what you exactly did, but more about why.

I've also posted a comment on the PR to industrial_core.

gavanderhoorn added a commit that referenced this pull request May 3, 2015
After merging #8, the Motoman specific messages were relocated to the
vendor specific range for Motoman. This change updates the code to
reflect that change. No changes to any functionality.

Dissector is now in agreement with REP-I0004.
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.

2 participants