On Wed, Mar 13, 2013 at 11:52 AM, Dan Smith <dsmith@danplanet.com> wrote:
Wow, this is a lot of work, thanks for doing it!

> You can select multiple plans because, for example, you might want
> both an IARU plan and a country plan.

So, from reading part of the code, I got the impression that perhaps
the plans are/should be hierarchical. Meaning, that the NA plan is a
subset of IARU 2. Is that the case?

Yes, however a North American Marine band plan and a North American Amateur band plan would both be under an ITU region 2 band plan, and you might want to use both.
 
I ask because I'm wondering if it
makes more sense to let folks choose IARU 2 or NA (which would inherit
from IARU, and perhaps override some bits) instead of this
mix-and-match behavior. I don't think it would make much sense to have
NA and IARU 1 enabled at the same time, for example, but maybe I'm
wrong. Of course, that has me thinking that we should be utilizing
inheritance...

You're right.  I'll try it with objects and see how it looks.  I expect that will make it easier to calculate and claim the repeater input blocks too (with opposite offsets).
 
> The format in bandplan_au.py is the format I prefer.  The format in
> bandplan_na.py is carried over from chirp_common.py and should be
> replaced by the new format once we agree on how it should look.

It's very close to what I pulled out of thin air earlier, and overall I
think I'm fine with it. I do wonder if we should be using objects
instead of just primitives, but I'm not sure.

> While testing this I noticed that there are multiple instances of the
> config class, with different and overlapping lifetimes.  This means
> that sometimes configuration changes are applied and sometimes they
> are not. That is a separate issue.

Hmm? Config is a singleton, and the various proxy objects should just be
views into it. I've not seen any such issues, but if you find them, we
should definitely get them resolved.

I've filed bug #683
 
> +  "bands": {
> +    (28.000e6, 29.700e6): {

I don't like the specification of these as floats. Can we use ints,
perhaps with easy shortcut helpers such as:

  (k(28000), k(28700))

and

  (m(146), m(148))

Sounds good to me.
 
> +def _walk_bandplan(freq, bandplan, defaults):
> +    # Override defaults with values from this level.
> +    for name in defaults:
> +        defaults[name] = bandplan.get(name, defaults[name])
> +
> +    # Go more specific if the frequency fits within an available
> sublevel.
> +    for key, vals in bandplan.items():
> +        if key in defaults:
> +            pass
> +        elif isinstance(key, tuple):
> +            if key[0] > key[1]:
> +                raise Exception("Invalid range %s - %s" % key)
> +            if int(freq) >= key[0] and int(freq) < key[1]:
> +                _walk_bandplan(freq, vals, defaults)
> +                break
> +        else:
> +            raise Exception("Invalid key %s" % key)

This just screams at me for the use of an object structure instead of
primitives. Is there a reason you decided against it? (other than me
firing off a half-baked concept earlier, of course :P)

I'll try what you suggest.
 
> +      <menu action="bandplan" name="bandplan">
> +        <menuitem action="north_america"/>
> +        <menuitem action="australia"/>
> +      </menu>

This is fine, of course, and we don't have many here right now, but we
can automate the generation of the submenu so that adding a new
bandplan is a little less involved.

> +        # Migrate old "automatic repeater offset" setting to

Thanks for this!

Otherwise, I think it looks fine. Again, thanks a lot for taking on
this non-trivial thing :)

No worries.

--
Sean Burford <sburford@google.com>