Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 5929] New protocol dissector for "CIP Motion"

[bugzilla-daemon@xxxxxxxxxxxxx]

https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5929

Michael Mann <mmann78@xxxxxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #6616|0                           |1
        is obsolete|                            |
   Attachment #6981|                            |review_for_checkin?
               Flag|                            |

--- Comment #7 from Michael Mann <mmann78@xxxxxxxxxxxx> 2011-09-10 21:29:29 PDT ---
Created an attachment (id=6981)
 --> (https://bugs.wireshark.org/bugzilla/attachment.cgi?id=6981)
Updated patch

Addressed all issues except #2 & #9.
2) Didn't know when to use "extended" value string vs. "regular" value string
9) The values pulled off the packet are guint8, but multipliers are applied to
the values to use elsewhere in the dissector.  They could probably be guint16s,
but most compilers are optimized for 32-bit values, so I left them alone.

A few additional comments about the dissector, but nothing that I feel should
prevent the patch from being committed:

1. The timestamp fields (hf_cip_cont_time_stamp, etc) could probalbly be
"timestamp" datatypes instead of FT_UINT64.  

2. The bit fields could be better enumerated.  I switched the ones I noticed to
FT_BOOLEAN with the "enum" of tfs_true_false.  I suspect some of the other
buildin boolean enums make work better (enable/disable?) or some could be
created within the dissector specific to it.

3. More integration/code reuse with the cip dissector.  They share commonality
and that's what the packet-cip.h header file is for.

-- 
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
You are watching all bug changes.