[chirp_devel] Review: IC-T7H support

[Dan Smith]

> Alright, updated patch with passing tests. Turns out it does support
> fractional frequencies, but using a shifted BCD. I also brought in the
> band edges to what this thing supports when you do the software switch
> to wide RX.

Cool, thanks!

> I checked the tuning steps. This Icom doesn't store the step, as
> verified by diffing memory dumps and trying it out on the radio.

Hmm, strange, but okay :)

> +        mem.freq = int(_mem.freq) * 100000
> +        mem.freq += _mem.lastfreq * 10000
> +        mem.freq += int((_mem.fraction / 2.0) * 1000)
> +
> +        mem.offset = int(_mem.offset) * 10000
> +        mem.rtone = chirp_common.TONES[_mem.rtone-1]
> +        mem.ctone = chirp_common.TONES[_mem.ctone-1]

I really hate to pick these nits, but since I'm going to make you
resubmit one more time, I will. PEP8 says that operators have one space
around them. You do it correctly in the first block, but there are
several instances elsewhere that aren't. I'm certainly not perfect here,
but if you wouldn't mind tweaking them, I'd appreciate it.

chirp/ict7h.py:43:6: E221 multiple spaces before operator
chirp/ict7h.py:44:8: E222 multiple spaces after operator
chirp/ict7h.py:94:50: E225 missing whitespace around operator

> diff -r 4a3dbf10d64b -r 3deda8456d93 tests/images/Icom_IC-T7H.img
> Binary file tests/images/Icom_IC-T7H.img has changed

This won't apply properly and I'll commit your image separately. So, if
you could pull this null hunk out, that'd be good.

> diff -r 4a3dbf10d64b -r 3deda8456d93 tests/run_tests
> --- a/tests/run_tests	Fri Jul 13 16:43:41 2012 -0700
> +++ b/tests/run_tests	Tue Jul 17 08:58:28 2012 -0700
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/env python
>  #
>  # Copyright 2011 Dan Smith <dsmith at danplanet.com>
>  #

This shouldn't be in the patch.

Other than those three minor things, I think this is ready to apply.
Thanks a lot for doing this. It's always nice to have someone just drop
in and write a driver :)

>      send_clone_frame(radio.pipe, CMD_CLONE_END, radio.get_endframe(), raw=True)
> +    time.sleep(3.5)

I'm not sure if you intended to include this patch as well, but I don't
want to apply this as-is. If you want to try to try to poll for the end
result for a certain amount of time, then that'd be fine. It doesn't
work, for example, on my 2820, presumably because the radio spends more
than 3.5 seconds validating the (much larger) image.

Lets get the T7H driver applied first and then we can experiment with
the clone result thing.

Thanks a bunch Eric!

-- 
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/20120718/fd82b62e/attachment-0001.bin