This driver is very similar to the TH9800 driver. The memory map is slightly different, the features differ a bit, and the upload/download sequence is slightly different.

I started discussing a refactor with the TH9800 authors. They seem like a good bunch of guys. Unfortunately I don't have access to a TH7800 radio anymore and I don't have a TH9800. I didn't want to destabilize the TH9800 or break what I had for the TH7800 by refactoring much at this point.

> Hmm, why is this copied into the driver?
>
> > + add_radio_setting(basic, "apo", "Auto Power off (Hours)",
> > + [("Off", 0), ("0.5", 5), ("1.0", 10), ("1.5", 15), ("2.0", 20)],
> > + _settings.apo)

I'm sorry, I don't understand your question. FWIW, APO is a feature that was broken in the TH9800 and I've fixed it in the TH7800. I planned to show the TH7800 authors how I've fixed it.

There are probably a few things we could discuss about the driver. I almost forgot about the comments I added about "common code" that could potentially be moved to a different module for all to use. You're probably the right person to talk to about that.

I can run the pep8 tester - hadn't done that. I'm sure I can break the lines before 80 columns.

The tests pass with the driver.

Regards,

Nathan

On Wed, Jun 8, 2016 at 8:28 AM, Dan Smith via chirp_devel <chirp_devel@intrepid.danplanet.com> wrote:

Hi Nathan,

> This is my first submission to the project. Hopefully I'm following the
> process well enough. I tested this driver fairly thoroughly with the
> radio when I had it, but no longer have access to it.

Cool, thanks!

> Wasn't sure how to include the binary file for unit tests, which I ran.
> I attached it to this email.

Just attaching it here is okay. It would be even better to attach it to
the ticket, because it doesn't get sent to all the subscribers and it's
kind of archived with the request.

> # HG changeset patch
> # User Nathan Crapo <nathan@n4nc3o.com>
> # Date 1465334892 21600
> # Tue Jun 07 15:28:12 2016 -0600
> # Node ID 2f356864c55f674a6faf157d7e13868e5275cf72
> # Parent 333a280ca0c4e856258ebf9dfdb7c547fa9ec90c
> Adding support for TYT TH-7800. Fixes #3477.
>
> diff -r 333a280ca0c4 -r 2f356864c55f chirp/drivers/th7800.py
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/chirp/drivers/th7800.py Tue Jun 07 15:28:12 2016 -0600
> @@ -0,0 +1,780 @@
> +# Copyright 2014 Tom Hayward <tom@tomh.us>
> +# Copyright 2014 Jens Jensen <af5mi@yahoo.com>
> +# Copyright 2014 James Lee N1DDK <jml@jmlzone.com>
> +# Copyright 2016 Nathan Crapo <nathan@n4nc3o.com> (TH-7800 only)

Is this a near copy of another radio? Is it significantly different such
that making them share a common base is not feasible?

> +# --------------- Common Code ---------------
> +
> +# This section should go somewhere common like settings.py. Keep it here for
> +# now until other developers review and accept or reject it.
> +class RadioSettingValueMap(RadioSettingValueList):

Hmm, why is this copied into the driver?

> + add_radio_setting(basic, "apo", "Auto Power off (Hours)",
> + [("Off", 0), ("0.5", 5), ("1.0", 10), ("1.5", 15), ("2.0", 20)],
> + _settings.apo)

Can you break these lines before 80 columns? There is a pep8 tester in
the tree (ideally, just run run_all_tests.sh) to validate the style.

I didn't apply this to run the tests, but I assume they all pass with this?

Thanks!

--Dan
_______________________________________________
chirp_devel mailing list
chirp_devel@intrepid.danplanet.com
http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers