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

[Tom Hayward]

On Tue, Apr 3, 2012 at 18:53, Dan Smith <dsmith at danplanet.com> wrote:
> 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.

Looks good. I'll get this patch updated to use it.

>> +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?

I was hoping to inherit CSVRadio, but CSVRadio inherits
IcomDstarSupport and I don't want that. If I inherit from CSVRadio, is
there a way to disable the Dstar stuff?

At first when I supported File > Open, I was getting a Dstar tab I
didn't want. Now that it's import-only, this might not be an issue. I
just don't want to see Dstar columns/features.

> 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.

Indeed. That is leftover copypasta that I meant to remove.

>> +    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.

Added to my list of fixes.

>> +    def get_features(self):
>
> If you inherit from CSVRadio, then these are fine, but I think they are
> probably unnecessary if you're import-only, right?

Ok, are you saying get_features() is not used during import? I can
remove it if it's not used.

>> +    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 :)

Ok

>> +    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.

In this loop we only have access to one column at a time, so I am
saving the vars odd_split and tx_freq to a broader scope and making
the final assignment outside the loop. What exactly are you
suggesting?

>> +                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:

You are strong in the ways of Python.

>> +                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.

Ok

>> +
>> +        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?

I have three example hmk files from three different Kenwood
programming software and they are all 10 lines. But yes, I think I can
do better. The header line always starts with !!Ch, so I can look for
that, back up one line, and send that to the csv reader.

>> +        #f.seek(0, 0)
>
> Did you intend to leave this in or take it out?

This needs to be removed. Seeking back to 0 would obviously make my
10-line skip worthless :)

> --
> Dan Smith
> www.danplanet.com
> KK7DS


Tom KD7LXL