[chirp_devel] [PATCH] A module that provides Chirp functionalities to custom python scripts #5859

[Dan Smith]

Hi Terence,

> On Jun 4, 2018, at 6:35 PM, Terence Paddack via chirp_devel <chirp_devel at intrepid.danplanet.com> wrote:
> 
> # HG changeset patch
> # User Terence Paddack <terence.paddack at gmail.com>
> # Date 1528162132 18000
> #      Mon Jun 04 20:28:52 2018 -0500
> # Node ID af8ce534bb949d2289c6f9b66650b333ea6e7ac5
> # Parent  0cbedd969bad9e0635e58d138266add5edbf2779
> A module that provides Chirp functionalities to custom python scripts #5859

This is cool, and thanks for doing the work. Do you have any plans to refactor the UI to use the utility functions here, so that we don't duplicate the behavior between this and the UI?

> diff -r 0cbedd969bad -r af8ce534bb94 README.chirpAPI
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/README.chirpAPI	Mon Jun 04 20:28:52 2018 -0500

This is great, but it should probably be mostly in docstrings in the code (more on that below), and in ReST or Markdown format so that we can render it to something more useful for people.

> diff -r 0cbedd969bad -r af8ce534bb94 chirpAPI.py
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/chirpAPI.py	Mon Jun 04 20:28:52 2018 -0500

This should be in the chirp/ module itself. Calling this just "API" might be a bit confusing -- how about we come up with some other name for it, maybe using the words "high level" or "unified" or something?

Also, I think that since this intends to be developer-focused and readable, it would probably be good to be *extra* strict about pep8 compliance. Also, unit tests would be good, like what we have in tests/unit.

> +#The following list contains all of the attributes of a memory channel that will be listed by list_mem()
> +mem_attr = ['number', 'freq', 'name', 'tmode', 'rtone', 'ctone', 'dtcs', 'rx_dtcs', 'dtcs_polarity', 'cross_mode', 'duplex', 'offset', 'mode', 'power', 'skip', 'empty']

This should be wrapped and either prefixed with _ if this is not intended to be used externally, or docstring'd if it is.

> +#Global Variables
> +rclass = ''
> +radio = ''
> +rf = ''

I think this should be implemented as a class so that you don't have to have these globals. Keep the state in the instance of the class, otherwise this can't be used to manipulate multiple radio images at the same time (which is one of the first scenarios I'd think of where I might want to use this).

> +class search_result():

We should follow proper naming conventions for this, and make these new-style classes. Yes, much of chirp is very old-school python, but for new stuff we should do the modern thing and make these inherit from object.

> +#This class is used by search_settings & print_search_results

Externally visible things should have docstrings (and be indented per pep8). This would be a good example to convert so that it's discoverable and self-documenting. Everything else in the file needs this same treatment, so I'll avoid mentioning it in every instance.

> +    def __init__(self):
> +        self.query = ''
> +        self.group = []
> +        self.option = []
> +        self.setting = []
> +        self.group_num = []
> +        self.option_num = []
> +
> +def setup_global_vars(map_file):
> +#Assigns values to some variables that are used by multiple functions in this library
> +#Used by other functions in this library; not recommended for use by the end-user
> +#Required arguments:
> +#    1: map_file    (string containing the name of the memory map file to which the changes will be written ex. 'QYT_KT8900D.img')
> +#Example Usage: setup_global_vars('QYT_KT8900D.img', mem, channel)
> +#Example Usage: setup_global_vars('Baofeng_UV-5R.img', mem, channel)
> +#Example Usage: setup_global_vars('Baofeng_BF-888.img', mem, channel)
> +    ret_val = 1
> +    if not type(map_file) is str:
> +        print "Type mismatch: map_file must be a string. You entered a '" + str(type(map_file).__name__) + "'"
> +    else:
> +        global rclass
> +        global radio
> +        global rf

These should be instance-level variables so you don't have to depend on the globals.

> +        rclass = directory.get_radio_by_image(map_file).__class__
> +        radio = rclass(map_file)
> +        rf = radio.get_features()
> +        ret_val = 0
> +    return ret_val
> +
> +def write_memory(map_file, mem, channel):
> +#Writes the contents of the memory channel object that it is passed into the memory map; 
> +#Used by other functions in this library; not recommended for use by the end-user
> +#Required arguments:
> +#    1: map_file    (string containing the name of the memory map file to which the changes will be written ex. 'QYT_KT8900D.img')
> +#    2: mem         (mem class object containing all channel information to be written to the memory map)
> +#    3: channel	    (integer corresponding to the channel whose contents will be edited)
> +#Example Usage: write_memory('TYT_TH-9800.img', mem, channel)
> +#Example Usage: write_memory('QYT_KT8900D.img', mem, channel)
> +#Example Usage: write_memory('Baofeng_UV-5R.img', mem, channel)
> +#Example Usage: write_memory('Baofeng_BF-888.img', mem, channel)
> +#Comments:
> +#    1: WARNING: This function, which is used by most of the memory channel-editing functions, is NOT programmed to validate memory map contents!
> +    ret_val = 1
> +    if not type(map_file) is str:
> +        print "Type mismatch: map_file must be a string. You entered a '" + str(type(map_file).__name__) + "'"
> +    elif not isinstance(mem, chirp_common.Memory):
> +        print "Type mismatch: mem must be a chirp memory object. You entered a '" + str(type(mem).__name__) + "'"
> +    elif not type(channel) is int:
> +        print "Type mismatch: channel must be an int. You entered a '" + str(type(channel).__name__) + "'"
> +    else:        
> +        setup_global_vars(map_file)
> +        if mem.empty == True:
> +            mem.empty = False
> +        radio.set_memory(mem)
> +        radio.save_mmap(map_file)

Same here, if this was a method on a class, then you'd have self.radio to operate on. I think you probably want to have a top-level class that you start with, maybe RadioFile or ChirpClient or something like that. Do the initialization of the driver and such in the __init__ for that, and make most of the operations you have below be instance methods on that class.

> +        print_mem(map_file, channel)

Generally it's a bad idea to print things in random places of an API implementation that is intended to be used inside other programs. If I was using this to generate something on stdout, this would confuse the desired output with this debug. We have python logging mostly installed everywhere else, so it makes sense to install a logging handler for this (for chirp.whateveryoucallthis) an that way people can control the log level of that sub-logger in their own application if they want, so that logging output goes where they want, at the level they want.

I've got a few other comments, but those are the biggest at the moment. To summarize, in order of importance:

0. Move it into the chirp/ module
1. Make it a class and ditch the globals
2. Fix the wrapping, indentation and PEP8 compliance
3. Docstrings for almost everything you have in comments
4. Convert print statements into logging calls
5. Unit tests
6. Convert the readme to something render-able

Also maybe noting which of these functions are likely to be usable by the UI to avoid duplication would be super.

Thanks a lot for doing this. It was clearly a lot fo work and would be cool if we could get it into shape and in the tree!

--Dan