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

+@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-Guid...

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@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers

----- Original Message -----

My opinion is that multi-device radio drivers tend to be a little buggy-er than others, and it's annoyingly hacky in the tests to make it work right. If you feel strongly, then that's fine, but especially since I expect nobody to have any real reason to use those channels anymore, it's probably better to opt for simplicity. Your call.

Yes, I noticed it's a bit buggy :) but it works for me. I tried to rewrite the code to use special channels also for TV, but it's a bit complicated. Is there a way how to force users not to set e.g. certain modulations for such special channels? Or is the only way to let them to select whatever they want from the listbox and do the check in the set_memory method and if Memory items aren't valid, change them to some sane values? I think this could be confusing for users.

...

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.

It's a CHIRP requirement that the driver not return a memory with trailing whitespace. It's annoying to edit that way, and tends to propagate through import and copy/paste operations.

Thanks, it works now.

...

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.

Put a big fat comment over it, at the very least. Explain what it used to do and what your rationale is for doing it this way. Otherwise it'll be hard to suss out later without blame.

Added.

...

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.

I think it's a marketing thing. The -A variants are usually for North America, -E is for Europe and without either means something else. Not all models have all those variants. In other cases where necessary, we support the A and the E, but I have never heard of an A not working for the un-suffixed variant.

Thanks, it seems you are right. The IC-T90 manual just exists in Japan language (I wasn't able to get it in the English language). And according comparison of these two manuals, it seems the radios are nearly identical - just minor different configuration for Asia, similarly as with the IC-E90. So, I added alias just for IC-T90. Or maybe I could name the radio IC-T90 and alias as IC-E90.

Regarding the clone status check, I spent some time on digging into the USB sniffs and it seems there is a bug in the Chirp code. After adding:

--- a/chirp/drivers/icf.py +++ b/chirp/drivers/icf.py @@ -183,6 +183,7 @@ def send_clone_frame(radio, cmd, data, raw=False, checksum=False): pass

radio.pipe.write(frame) + get_clone_resp(radio.pipe)

return frame

I.e. after calling already defined, but never used function, it started to work even with the clone status check enabled. In the original Icom SW sniffs the clone status check response is clearly visible. It seems the icf driver doesn't read the radio responses during the clone process and when it tries to read the last response code, there is already buffer full of previous data which it cannot correctly process.

But there is a problem with this patch, it breaks the tests so it waits forever on the pipe:

$ ./run_tests.py -d 'Icom_IC-E90' Icom IC-E90 Detect PASSED: All tests Icom IC-E90 Settings PASSED: All tests ^CTraceback (most recent call last): File "./run_tests.py", line 1300, in <module> failed = tr.run_one(options.driver) File "./run_tests.py", line 1232, in run_one "%s.img" % drv_name)) File "./run_tests.py", line 1210, in run_rclass_image failed += self.run_rclass_image(dev.__class__, image, dst=dev) File "./run_tests.py", line 1213, in run_rclass_image return self._run_one(rclass, image, dst=dst) File "./run_tests.py", line 1167, in _run_one failures = tc.run() File "./run_tests.py", line 984, in run self._run(self.SerialNone()) File "./run_tests.py", line 947, in _run radio.sync_in() File "../chirp/drivers/icx90.py", line 598, in sync_in icf.IcomCloneModeRadio.sync_in(self) File "../chirp/drivers/icf.py", line 627, in sync_in self._mmap = clone_from_radio(self) File "../chirp/drivers/icf.py", line 289, in clone_from_radio return _clone_from_radio(radio) File "../chirp/drivers/icf.py", line 237, in _clone_from_radio md = get_model_data(radio) File "../chirp/drivers/icf.py", line 140, in get_model_data send_clone_frame(radio, 0xe0, mdata, raw=True) File "../chirp/drivers/icf.py", line 186, in send_clone_frame get_clone_resp(radio.pipe) File "../chirp/drivers/icf.py", line 162, in get_clone_resp while not exit_criteria(resp, length): File "../chirp/drivers/icf.py", line 157, in exit_criteria return buf.endswith("\xfd") KeyboardInterrupt

So this needs to be addressed somehow

thanks & regards

Jaroslav

----- Original Message -----

Sorry I meant to reply to this earlier.

...

Regarding the clone status check, I spent some time on digging into the USB sniffs and it seems there is a bug in the Chirp code. After adding:

--- a/chirp/drivers/icf.py +++ b/chirp/drivers/icf.py @@ -183,6 +183,7 @@ def send_clone_frame(radio, cmd, data, raw=False, checksum=False): pass

radio.pipe.write(frame)

• get_clone_resp(radio.pipe)

  return frame

I.e. after calling already defined, but never used function, it started to work even with the clone status check enabled. In the original Icom SW sniffs the clone status check response is clearly visible. It seems the icf driver doesn't read the radio responses during the clone process and when it tries to read the last response code, there is already buffer full of previous data which it cannot correctly process.

But there is a problem with this patch, it breaks the tests so it waits forever on the pipe:

$ ./run_tests.py -d 'Icom_IC-E90' Icom IC-E90 Detect PASSED: All tests Icom IC-E90 Settings PASSED: All tests ^CTraceback (most recent call last): File "./run_tests.py", line 1300, in <module> failed = tr.run_one(options.driver) File "./run_tests.py", line 1232, in run_one "%s.img" % drv_name)) File "./run_tests.py", line 1210, in run_rclass_image failed += self.run_rclass_image(dev.__class__, image, dst=dev) File "./run_tests.py", line 1213, in run_rclass_image return self._run_one(rclass, image, dst=dst) File "./run_tests.py", line 1167, in _run_one failures = tc.run() File "./run_tests.py", line 984, in run self._run(self.SerialNone()) File "./run_tests.py", line 947, in _run radio.sync_in() File "../chirp/drivers/icx90.py", line 598, in sync_in icf.IcomCloneModeRadio.sync_in(self) File "../chirp/drivers/icf.py", line 627, in sync_in self._mmap = clone_from_radio(self) File "../chirp/drivers/icf.py", line 289, in clone_from_radio return _clone_from_radio(radio) File "../chirp/drivers/icf.py", line 237, in _clone_from_radio md = get_model_data(radio) File "../chirp/drivers/icf.py", line 140, in get_model_data send_clone_frame(radio, 0xe0, mdata, raw=True) File "../chirp/drivers/icf.py", line 186, in send_clone_frame get_clone_resp(radio.pipe) File "../chirp/drivers/icf.py", line 162, in get_clone_resp while not exit_criteria(resp, length): File "../chirp/drivers/icf.py", line 157, in exit_criteria return buf.endswith("\xfd") KeyboardInterrupt

So this needs to be addressed somehow

Yeah, so I'm surprised we're not doing this already and it definitely seems like the right thing to do is make the change you've proposed above. The reason it fails is one of the clone edge condition tests provides an endless stream of garbage, as if you selected the wrong serial port to do a download from, in an attempt to make sure the driver is robust enough to notice and fail. Since get_clone_resp() is pretty dumb, it ... fails.

So what do you propose here? Could we add e.g. some counter which will abort get_clone_resp() after some number of characters are received (e.g. twice the clone frame)?

On the one hand I'd really like to get that change in there, but it's also been (from looking at the history) not checking the response to each frame for the entire lifetime of that code (so ~10 years). I would hate to introduce it now and cause some instability for Icom users, which have always enjoyed pretty stable support. I'm not sure what to do here.

However, to get you out of jail here, maybe we could do something to just affect your model and then potentially enable it per-driver on other models as we're able to test them. Some some radio.MUNCH_CLONE_RESP=True flag that we check in the icf code to know whether or not to do that? What do you think?

OK, NP with that. I can rewrite the code.

Jaroslav

----- Original Message -----

----- Original Message -----

...

----- Original Message -----

...

Sorry I meant to reply to this earlier.

...

Regarding the clone status check, I spent some time on digging into the USB sniffs and it seems there is a bug in the Chirp code. After adding:

--- a/chirp/drivers/icf.py +++ b/chirp/drivers/icf.py @@ -183,6 +183,7 @@ def send_clone_frame(radio, cmd, data, raw=False, checksum=False): pass

radio.pipe.write(frame)

• get_clone_resp(radio.pipe)

  return frame

I.e. after calling already defined, but never used function, it started to work even with the clone status check enabled. In the original Icom SW sniffs the clone status check response is clearly visible. It seems the icf driver doesn't read the radio responses during the clone process and when it tries to read the last response code, there is already buffer full of previous data which it cannot correctly process.

But there is a problem with this patch, it breaks the tests so it waits forever on the pipe:

$ ./run_tests.py -d 'Icom_IC-E90' Icom IC-E90 Detect PASSED: All tests Icom IC-E90 Settings PASSED: All tests ^CTraceback (most recent call last): File "./run_tests.py", line 1300, in <module> failed = tr.run_one(options.driver) File "./run_tests.py", line 1232, in run_one "%s.img" % drv_name)) File "./run_tests.py", line 1210, in run_rclass_image failed += self.run_rclass_image(dev.__class__, image, dst=dev) File "./run_tests.py", line 1213, in run_rclass_image return self._run_one(rclass, image, dst=dst) File "./run_tests.py", line 1167, in _run_one failures = tc.run() File "./run_tests.py", line 984, in run self._run(self.SerialNone()) File "./run_tests.py", line 947, in _run radio.sync_in() File "../chirp/drivers/icx90.py", line 598, in sync_in icf.IcomCloneModeRadio.sync_in(self) File "../chirp/drivers/icf.py", line 627, in sync_in self._mmap = clone_from_radio(self) File "../chirp/drivers/icf.py", line 289, in clone_from_radio return _clone_from_radio(radio) File "../chirp/drivers/icf.py", line 237, in _clone_from_radio md = get_model_data(radio) File "../chirp/drivers/icf.py", line 140, in get_model_data send_clone_frame(radio, 0xe0, mdata, raw=True) File "../chirp/drivers/icf.py", line 186, in send_clone_frame get_clone_resp(radio.pipe) File "../chirp/drivers/icf.py", line 162, in get_clone_resp while not exit_criteria(resp, length): File "../chirp/drivers/icf.py", line 157, in exit_criteria return buf.endswith("\xfd") KeyboardInterrupt

So this needs to be addressed somehow

Yeah, so I'm surprised we're not doing this already and it definitely seems like the right thing to do is make the change you've proposed above. The reason it fails is one of the clone edge condition tests provides an endless stream of garbage, as if you selected the wrong serial port to do a download from, in an attempt to make sure the driver is robust enough to notice and fail. Since get_clone_resp() is pretty dumb, it ... fails.

So what do you propose here? Could we add e.g. some counter which will abort get_clone_resp() after some number of characters are received (e.g. twice the clone frame)?

...

On the one hand I'd really like to get that change in there, but it's also been (from looking at the history) not checking the response to each frame for the entire lifetime of that code (so ~10 years). I would hate to introduce it now and cause some instability for Icom users, which have always enjoyed pretty stable support. I'm not sure what to do here.

However, to get you out of jail here, maybe we could do something to just affect your model and then potentially enable it per-driver on other models as we're able to test them. Some some radio.MUNCH_CLONE_RESP=True flag that we check in the icf code to know whether or not to do that? What do you think?

OK, NP with that. I can rewrite the code.

Attached patch, it's not perfect, but works for me.

Another thing, the final clone result check could be probably rewritten. In the IcfFrame class the cmd code 0xE6 for final clone result check is already defined (and I can see it in the USB sniffs), but it's never used in the code. Instead the current code blindly reads the garbage and checks whether the final byte is 0x00. I think it should check for the cmd 0xE6 and read the following result code.

thanks & regards

Jaroslav

Hi Dan,

are you OK with the proposed patch? Or do you have different patch?

thanks & regards

Jaroslav