search

ros-industrial / packet-simplemessage

Wireshark Lua dissector for the ROS-Industrial SimpleMessage protocol
8 stars 10 forks source link

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

Closed thiagodefreitas closed 10 years ago

thiagodefreitas commented 10 years ago

... current tests with MOTOMAN

gavanderhoorn commented 10 years ago

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 commented 10 years ago
  1. Should be fixed by: https://github.com/thiagodefreitas/packet-simplemessage/commit/5786466da538efd4a783d68c95459320001e711c
  2. Capture data is available at: https://gist.github.com/thiagodefreitas/95cc9dcc27ccc85460d8#file-bigger_capture
gavanderhoorn commented 10 years ago

Works for me, +1.

gavanderhoorn commented 10 years ago

Thanks for the contribution.

gavanderhoorn commented 10 years ago

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 commented 10 years ago

Ping?

thiagodefreitas commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

Ping?

thiagodefreitas commented 10 years ago

@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: https://github.com/ros-industrial/motoman/pull/42 https://github.com/ros-industrial/industrial_core/pull/82

Thanks. :)

gavanderhoorn commented 10 years ago

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.