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
>
