Hi Brian,
there is no need to add a text string as blurb, if it is the same as
the title of the field, so for example the ipv4 one would become:
{ &hf_helen_ipv4,
{ "IPv4", "helen.ipv4address", FT_IPv4, BASE_NONE, NULL, 0x0,
NULL, HFILL}},
Also, continuing from Michael's comments there is no need to read the
value of status and use proto_tree_add_uint, since you are not using
status later on. I think that it would be best to change it to:
void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
{
[...]
/* Status: */
if ((fieldsAvail & 1) != 0) {
proto_tree_add_item(helen_sub_tree, hf_helen_gpsstatus,
tvb, offset, 1, FALSE);
offset += 1;
}
[...]
}
Also at dissect_helen, I propose to deal with the packet code in the
same way as the gpsstatus. Add an entry for it in the hf[] array,
define an array of value_string with the code names (i.e. End of
Packet, GPS Extension...) and use proto_tree_add_item to add it to the
tree. Apart from all other advantages, you don't need to explicitly
deal with the unknown code strings.
Best regards,
Gerasimos
2010/2/4 Speck Michael EHWG AVL/GAE <Michael.Speck@xxxxxxx>:
> Hi Brian,
>
> just had a look on your latest preview.
> There is one more thing you can do to reduce the code size even further:
> as Jakub has already suggested earlier, it is a good idea to use
> value_string struct to map values with strings. In your case it is
> especially true for the GPS status.
>
> There are only a few changes necessary to your code. First define a
> value_string typed constant, then slightly change the status field
> definition in function proto_register_helen() and finally shorten the
> status field dissection in function dissect_helen(). Below is shown how
> these code sections could look like afterwards.
>
>
> static const value_string helen_gps_status[] = {
> { 0, "Good" },
> { 1, "No Fix" },
> { 2, "Bad GPS Read" },
> { 0, NULL }
> };
>
>
> void proto_register_helen(void) {
> [...]
> { &hf_helen_gpsstatus,
> { "GPS Status", "helen.gpsStatus", FT_UINT8, BASE_DEC,
> VALS(helen_gps_status), 0x00,
> "GPS Status", HFILL}},
> [...]
> }
>
>
> void dissect_helen(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
> {
> [...]
> /* Status: */
> if ((fieldsAvail & 1) != 0) {
> guint8 status;
> status = tvb_get_guint8(tvb,offset);
> proto_tree_add_uint(helen_sub_tree, hf_helen_gpsstatus,
> tvb, offset, 1, status);
> offset += 1;
> }
> [...]
> }
>
>
>
> Cheers
> Mike
>
>
>
>
> -----Original Message-----
> From: wireshark-dev-bounces@xxxxxxxxxxxxx
> [mailto:wireshark-dev-bounces@xxxxxxxxxxxxx] On Behalf Of Brian Oleksa
> Sent: Mittwoch, 3. Februar 2010 21:21
> To: Developer support list for Wireshark
> Subject: Re: [Wireshark-dev] preliminary code submission
>
> Jakub
>
> Yes...you are right. I am not using alot of those variables. I was
> mislead by what I was doing before I started to use the built in
> routines. :-)
>
> My code base keeps getting smaller and smaller. I do understand that
> code quality / readability is a key factor in this industry. I still
> need to fix my IDE to get the correct formatting.
>
> Perhaps once last quick look..?? Attached is the updated file.
>
> Thanks again for the great feedback..!!
>
> Brian
>
>
>
>
>
> Jakub Zawadzki wrote:
>> Hi,
>>
>> On Wed, Feb 03, 2010 at 01:05:32PM -0500, Brian Oleksa wrote:
>>
>>> Jakub
>>>
>>> Thanks for this feedback. It is always good to have an extra set of
>>> eyes :-)
>>>
>>
>> You missunderstood my comment about check_col()
>>
>> instead of:
>> if (check_col(pinfo->cinfo, COL_PROTOCOL))
>> col_set_str(pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_HELEN);
>
>>
>> if (check_col(pinfo->cinfo, COL_INFO))
>> col_clear(pinfo->cinfo, COL_INFO);
>>
>> Just write:
>> col_set_str(pinfo->cinfo, COL_PROTOCOL, PROTO_TAG_HELEN);
>
>> col_clear(pinfo->cinfo, COL_INFO);
>>
>>
>>> gfloat latitude;
>>> latitude = tvb_get_ntohieee_float(tvb,offset);
>>>
>>> Why do you think this variable is not being used..??
>>>
>>
>> Well because it's not used :) What is used for?
>>
>> And one more thing...
>>
>> You declare: ett_helen_ipv6, ett_helen_nos, ...
>> I believe you need only ett_helen from ett_*
>>
>> Cheers.
>>
> ________________________________________________________________________
> ___
>> Sent via: Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
>> Archives: http://www.wireshark.org/lists/wireshark-dev
>> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>>
>> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>>
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives: http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>