[chirp_devel] Add new radio Icom IC-E90/T90/T90A

[Jaroslav Skarvada]

Hi Dan,

thanks for review.

----- Original Message -----
> > The radio is a bit tricky - it has multiple special channels and
> > TV channels (well analog TV is a bit off-topic today, but
> > I wanted to support all features of this radio). I added
> > TV channels as a subdevice, because I have no better idea
> > how to do it.
> 
> Look at the ft817 (and related) radios. They have oodles of special channels,
> and I'd definitely prefer that it be implemented that way. I see you've got
> some special channels in your patch here already -- why not make the TV
> channels the same?
>
Yes, the IC-X90 has plenty of special channels, I tried to implement
them all. Regarding the TV channels I thought about:

- implementing it as the special channels as you are suggesting:
the problem is that the TV channels are different - they occupy
just 8 bytes instead of the 16 bytes regular/special channels, there
are just modulation AM/WFM, frequency (which is calculated differently),
name (different name size of other channels). And there are
no duplex, no offset, no tone, no tuning step and maybe no other
things. I am currently not sure whether it's possible to hack it
together with the "normal" special channels and not confuse user.

- implementing it in the setting dialog: it would lost the memedit
channels magic, e.g no copy&paste, reordering etc., I didn't want
to reimplement the memedit.

- implementing it as subdevice with different feature list:
I choose this variant because I thought it's the most easy way
and in the end it really was easy.

But if your opinion is different, I can change it.

> > Also I had to modify the icf driver a bit, because it seems
> > the clone status doesn't work with this radio. Maybe I am
> > just missing something here, but I wasn't able to persuade
> > my radio to behave this way. The proposed icf driver change
> > should be harmless for other ICOM radios, because it's adding
> > new option and the memory size calculation should be backward
> > compatible.
> 
> This makes me nervous. Every icom radio under the sun uses the same exact
> protocol module. The only difference is that the newer ones have an extended
> format to be able to address larger memories. It doesn't make sense to me
> that this one would be different.
>
IIRC in the protocol sniffs I did last year from the original SW I didn't
see the "clone status" and even when I was playing with it, it seemed there is
none. I am going to double check this. I will make new protocol sniffs
and I can post it here for others to re-check.
 
> > One test from the testsuite is failing:
> > 
> > ./run_tests.py -d 'Icom_IC-E90'
> >     Icom IC-E90        Detect      PASSED: All tests
> >     Icom IC-E90        Settings    PASSED: All tests
> >     Icom IC-E90        Clone       PASSED: All tests
> >     Icom IC-E90        Edges       FAILED: Field `name' is ` ', expected `'
> 
> This means you're not rstrip()ing the memory name. If you do that, I think
> you'll see this pass.
> 
I am rstriping(0x00) which is used by the radio as a pad for unfilled channels.
The radio supports space in its charset, so I am not rstriping it. What confused
me a bit is that the space was included in both 'expected'/'obtained' reports
of the testsuite.

> >  def clone_to_radio(radio):
> > @@ -400,7 +404,7 @@ def convert_data_line(line):
> >  
> >      line = line.strip()
> >  
> > -    if len(line) == 38:
> > +    if len(line) % 8 == 6:
> 
> Why this change?

This is because this radio uses longer icf lines, but it is still small address,
i.e. it doesn't use the 10 chars prefix and it uses the 6 chars prefix,
but it's detected as the 10 chars prefix, because the detection routine wasn't
flexible enough. This is an attempt which assumes that the length of the data line
(after the prefix) will be multiply of 8 (I thought that sane people would do it
this way :) , so the rest has to be the prefix. This calculation works for the
38 chars line length as was hardcoded before and also for the IC-E90. If there is
a better way how to detect this, I am ready to change it.

> 
> > diff --git a/chirp/drivers/icx90.py b/chirp/drivers/icx90.py
> > new file mode 100644
> > index 0000000..c76496a
> > --- /dev/null
> > +++ b/chirp/drivers/icx90.py
> > @@ -0,0 +1,826 @@
> > +# -*- coding: utf-8 -*-
> > +# Copyright 2018-2019 Jaroslav Škarvada <jskarvad at redhat.com>
> > +# Based on various code from the CHIRP
> > +
> > +# This program is free software: you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation, either version 3 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +ICX90_MEM_FORMAT = """
> 
> Please put the import lines above everything else like all the other files.
> 
NP, I will change it.

> > +class ICT90_Alias(chirp_common.Alias):
> > +    VENDOR = "Icom"
> > +    MODEL = "IC-T90"
> > +
> > +class ICT90A_Alias(chirp_common.Alias):
> > +    VENDOR = "Icom"
> > +    MODEL = "IC-T90A"
> > +
> > +
> > + at directory.register
> > +class ICx90Radio(icf.IcomCloneModeRadio):
> > +    """Icom IC-E/T90"""
> > +    VENDOR = "Icom"
> > +    MODEL = "IC-E90"
> > +
> > +    ALIASES = [ICT90_Alias, ICT90A_Alias]
> 
> I can see adding one alias for T90 instead of E90, but adding T90 and T90A is
> not necessary, IMHO.
> 
To be honest I don't know what's the difference between T90 and T90A, I saw
there are radios around which uses both labels on the case. I also noticed
there are manuals for both. So I will probably do more investigation about
it - i.e. I will probably diff the manuals. Or if by an accident is here
somebody who knows more, please let me know. But if you think it's not
necessary, I will drop it.

> > +    def __init__(self, pipe):
> > +        icf.IcomCloneModeRadio.__init__(self, pipe)
> > +        self.init_special()
> 
> Seems like this could just be done in get_features(), but maybe I'm missing
> somewhere else that you're using self.special...
>
I think it should also work. I am using the self.special also in the
get_memory/set_memory, but I assume both are called after the get_features call.
So NP, I will change it.
 
> Also, this needs a commit message and a bug reference in that in order to be
> applied. You should be okay sending a git formatted patch, but it needs to
> be importable with those piece of info for me to digest.
> 
> https://chirp.danplanet.com/projects/chirp/wiki/DevelopersProcess#Patch-Guidelines
>
Sorry, I missed this, NP, I will post git formatted patch later, probably during
next week after I return from my vacation and probably after we resolve other
problems with this patch. Do you want the binary img file to be included
in the git formatted patch? This was probably the only reason why I didn't
send it originally this way :)

thanks & regards

Jaroslav

> --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