https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6155
--- Comment #4 from Tyson Key <tyson.key@xxxxxxxxx> 2011-10-11 01:09:32 PDT ---
(In reply to comment #3)
> Looks good, but a few comments:
>
> 1) Current agreement is that we should not use ENC_NA for FT_*INT8 fields;
> instead the protocol's primary endianism should be used.
>
> 2) Personally I would prefer macros for things like:
> {0x61, "PC_to_RDR_SetParameters"},
> [...]
> case 0x61 /* PC_to_RDR_SetParameters */:
>
> 3) Although it's not strictly necessary at the moment, it's good form to set
> the column information even when !tree. And this short dissector could be made
> even shorter by combining the many col_set_str()'s into one col_add_str(...
> val_to_str(cmd, ...) at the top of the function (above the 'if (tree)').
>
> 4) checkApis had some complaints about duplicate blurbs:
>
> Error: the blurb for field "Message Type" ("ccid.bMessageType") matches the
> field name in /tmp/packet-usb-ccid.c
> Error: the blurb for field "Packet Length" ("ccid.dwLength") matches the field
> name in /tmp/packet-usb-ccid.c
> Error: the blurb for field "Slot" ("ccid.bSlot") matches the field name in
> /tmp/packet-usb-ccid.c
> Error: the blurb for field "Sequence" ("ccid.bSeq") matches the field name in
> /tmp/packet-usb-ccid.c
> Error: the blurb for field "Status" ("ccid.bStatus") matches the field name in
> /tmp/packet-usb-ccid.c
> Error: the blurb for field "Error" ("ccid.bError") matches the field name in
> /tmp/packet-usb-ccid.c
> Error: the blurb for field "Chain Parameter" ("ccid.bChainParameter") matches
> the field name in /tmp/packet-usb-ccid.c
> Error: the blurb for field "Data" ("ccid.abData") matches the field name in
> /tmp/packet-usb-ccid.c
> Error: the blurb for field "Voltage Level" ("ccid.bPowerSelect") matches the
> field name in /tmp/packet-usb-ccid.c
> Error: the blurb for field "Clock Status" ("ccid.bClockStatus") matches the
> field name in /tmp/packet-usb-ccid.c
> Error: the blurb for field "Data Structure Type" ("ccid.bProtocolNum") matches
> the field name in /tmp/packet-usb-ccid.c
> Error: the blurb for field "Block Wait Time Integer" ("ccid.bBWI") matches the
> field name in /tmp/packet-usb-ccid.c
> Error: the blurb for field "Level Parameter" ("ccid.wLevelParameter") matches
> the field name in /tmp/packet-usb-ccid.c
>
> I thought about making these changes myself (at least 1, 3, and 4) but ran out
> of time
Thanks once again for the review, Jeff.
I believe that the protocol is mostly *Big Endian (apart from a few fields that
I've explicitly specified as Little Endian, according to the specs). Since I'm
at university at the moment, I don't have access to my card reader, or to my
copies of the appropriate specifications and source code/toolchain.
* I think that ENC_NA defaults to that, anyway - so we should be OK with
replacing instances of "ENC_NA" with "ENC_BIG_ENDIAN" (or whatever it is).
I'll have another look tonight, when I get home, and report back.
Tyson.
--
Configure bugmail: https://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching all bug changes.