[chirp_devel] [PATCH] Add import support for Kenwood *.hmk files

[Dan Smith]

> diff -r 91be43cc7ac4 -r 1bb3df3d624f chirp/directory.py
> --- a/chirp/directory.py	Tue Apr 03 11:19:10 2012 -0700
> +++ b/chirp/directory.py	Tue Apr 03 16:21:35 2012 -0600
> @@ -91,6 +91,9 @@
>      if image_file.lower().endswith(".csv"):
>          return get_radio("Generic_CSV")(image_file)
>  
> +    if image_file.lower().endswith(".hmk"):
> +        return get_radio("Kenwood_HMK")(image_file)
> +
>      if icf.is_9x_icf(image_file):
>          return get_radio("Icom_IC91_92AD_ICF")(image_file)

This follows the existing pattern, but I think the existing pattern is
ugly. I can say that, since it's mine.

I suggested on IRC that you use match_model(), and then realized when I
saw the above that match_model() isn't used for CSV (or chirp) files,
and can't be for extension-based matching. What was I thinking.

I'll send a patch to the list in a minute that will enable doing so, and
convert the three cases there to use match_model() instead. Let me know
what you think about doing it that way instead.

> +class HMKRadio(chirp_common.CloneModeRadio):

I was thinking you would inherit from CSVRadio and hopefully eliminate
the need to duplicate all that work. Was there some reason this didn't
work out?

> +    def _blank(self):
> +        self.errors = []
> +        self.memories = []
> +        for i in range(0, 1000):
> +            m = chirp_common.Memory()
> +            m.number = i
> +            m.empty = True
> +            self.memories.append(m)

I don't think you need _blank() if you're not going to support editing,
right? If you inherit it from CSVRadio, it's one thing, but I think
it'll confuse a future developer if it's here for no reason.

> +    def __init__(self, pipe):
> +        chirp_common.CloneModeRadio.__init__(self, None)
> +
> +        self._filename = pipe
> +        if self._filename and os.path.exists(self._filename):
> +            self.load()
> +        else:
> +            self._blank()

It's an error if we're instantiated without a filename.

> +    def get_features(self):
> +        rf = chirp_common.RadioFeatures()
> +        rf.has_bank = False
> +        rf.has_dtcs_polarity = False
> +        rf.memory_bounds = (0, len(self.memories))
> +        rf.has_infinite_number = True
> +
> +        rf.valid_modes = list(chirp_common.MODES)
> +        rf.valid_tmodes = list(chirp_common.TONE_MODES)
> +        rf.valid_duplexes = ["", "-", "+", "split"]
> +        rf.valid_tuning_steps = list(chirp_common.TUNING_STEPS)
> +        rf.valid_bands = [(1, 10000000000)]
> +        rf.valid_skips = ["", "S"]
> +        rf.valid_characters = chirp_common.CHARSET_ASCII
> +        rf.valid_name_length = 999
> +
> +        return rf

If you inherit from CSVRadio, then these are fine, but I think they are
probably unnecessary if you're import-only, right?

> +    def _parse_quoted_line(self, line):
> +        line = line.replace("\n", "")
> +        line = line.replace("\r", "")
> +        line = line.replace('"', "")
> +
> +        return line.split(",")

This is dead code from the CSV driver. Lets remove it from there and not
replicate it here :)

> +    def _parse_csv_data_line(self, headers, line):
> +        mem = chirp_common.Memory()
> +        odd_split = False
> +            
> +        for header, (typ, attr) in self.ATTR_MAP.items():
> +            try:
> +                val = self._get_datum_by_header(headers, line, header)
> +                if not val and typ == int:
> +                    val = None
> +                elif attr == "duplex":
> +                    val = typ(self.DUPLEX_MAP[val])
> +                    if val == "split":
> +                        odd_split = True

Hmm, what do the Tx Freq. and Offset columns look like in the two cases?
Seems like we should be able to do better than this.

> +                elif attr == "skip":
> +                    val = typ(self.SKIP_MAP[val])
> +                elif attr == "tmode":
> +                    val = typ(self.TMODE_MAP[val])

Instead of special-casing these, why not use a function like the
frequency parsing case? Something like this:

  ATTR_MAP = {
     ...
      "T/CT/DCS" : (lambda v: self.TMODE_MAP[v], "tmode"),
      "L.Out"    : (lambda v: self.SKIP_MAP[v], "skip"),
     ...
  }

That way you can just cover all the cases with this:

> +                else:
> +                    val = typ(val)

...like the CSV driver does. Plus, if we can figure out something sneaky
for the Offset and Tx Freq. cases, then you could still inherit from
CSVRadio with a different ATTR_MAP :)

> +
> +    def load(self, filename=None):
> +        if filename is None and self._filename is None:
> +            raise errors.RadioError("Need a location to load from")
> +
> +        if filename:
> +            self._filename = filename
> +
> +        self._blank()

I think you can remove this.

> +
> +        f = file(self._filename, "r")
> +        for i in range(0, 10):
> +            f.readline().strip()

Is it always ten lines? Maybe it would be better to probe smartly to
chew up everything until the header row?

> +        #f.seek(0, 0)

Did you intend to leave this in or take it out?

-- 
Dan Smith
www.danplanet.com
KK7DS

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: OpenPGP digital signature
Url : http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20120403/ef1c342b/attachment-0001.bin