[chirp_devel] Adding TYT TH-7800 Support. Fixes #3477

[Nathan Crapo]

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 at 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 at 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 at tomh.us>
> > +# Copyright 2014 Jens Jensen <af5mi at yahoo.com>
> > +# Copyright 2014 James Lee N1DDK <jml at jmlzone.com>
> > +# Copyright 2016 Nathan Crapo <nathan at 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 at intrepid.danplanet.com
> http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
> Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20160608/6b2d5af5/attachment-0001.html