Bug # 10909 FT450 not recognized in FT-450D driver

[WCurlew]

CHIRP PR Checklist

The following must be true before PRs can be merged:

• All tests must be passing.
• Commits should be squashed into logical units.
• Commits should be rebased (or simply rebase-able in the web UI) on current master. Do not put merge commits in a PR.
• Commits in a single PR should be related.
• Major new features or bug fixes should reference a CHIRP issue.
• New drivers should be accompanied by a test image in tests/images (except for thin aliases where the driver is sufficiently tested already).
• All files must be GPLv3 licensed or contain no license verbiage. No additional restrictions can be placed on the usage (i.e. such as noncommercial).

Please also follow these guidelines:

• Keep cleanups in separate commits from functional changes.
• Please write a reasonable commit message, especially if making some change that isn't totally obvious (such as adding a new model, adding a feature, etc).
• Do not add new py2-compatibility code (No new uses of six, future, etc).
• All new drivers should set NEEDS_COMPAT_SERIAL=False and use MemoryMapBytes.
• New drivers and radio models will affect the Python3 test matrix. You should regenerate this file with tox -emakesupported and include it in your commit.

[kk7ds]

The PR checks are failing because you changed some marked-for-translation strings. I think you're on Windows, so just let me fix that for you before we merge as it'll be easier for me than you.

[WCurlew]

I don't understand what the legacy code test is all about. I keep failing it, but I don't understand why.

[kk7ds]

This needs to be @WCurlew so it references properly.

[WCurlew]

I'll make the change as specified in your later email Bill ---------- Original Message ---------- From: Dan Smith ***@***.***> To: kk7ds/chirp ***@***.***> Cc: WCurlew ***@***.***>, Author ***@***.***> Subject: Re: [kk7ds/chirp] Bug # 10909 FT450 not recognized in FT-450D driver (PR #803) Date: Fri, 03 Nov 2023 21:50:10 -0700 @kk7ds commented on this pull request. In tests/py3_driver_testers.txt:

@@ -343,7 +343,8 @@ Yaesu_FT-1D_R,+Yaesu_FT3D_R,30-Dec-2022 Yaesu_FT2D_R,+Yaesu_FT3D_R,30-Dec-2022 Yaesu_FT2D_Rv2,+Yaesu_FT3D_R,30-Dec-2022 ***@***.******@***.***,01-Jun-2023+Yaesu_FT-450,+Yaesu_FT-450D,01-Nov-2023+Yaesu_FT-450D,WCurlew,01-Nov-2023This needs to be @WCurlew so it references properly.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

[WCurlew]

Have corrected testers.py, re-run tox -e makesupported and sent corrected txt and md files up. Bill

---------- Original Message ---------- From: Dan Smith ***@***.***> To: kk7ds/chirp ***@***.***> Cc: WCurlew ***@***.***>, Author ***@***.***> Subject: Re: [kk7ds/chirp] Bug # 10909 FT450 not recognized in FT-450D driver (PR #803) Date: Fri, 03 Nov 2023 21:51:29 -0700 @kk7ds commented on this pull request. In tests/py3_driver_testers.txt:

@@ -343,7 +343,8 @@ Yaesu_FT-1D_R,+Yaesu_FT3D_R,30-Dec-2022 Yaesu_FT2D_R,+Yaesu_FT3D_R,30-Dec-2022 Yaesu_FT2D_Rv2,+Yaesu_FT3D_R,30-Dec-2022 ***@***.******@***.***,01-Jun-2023Are you testing FT-450 and FT-450D? If not, you should not change this line and only add your FT-450 report line. Also, please put it above the FT-450D so it sorts properly :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: ***@***.***>

[kk7ds]

Thanks for this. Looks pretty close, just a few things we need to get cleaned up and then it should be ready for merge. I appreciate your work on this, but if you want me to make these changes for you I can do that, although I'd rather you do it so you can test :)

Also, I'd normally want to see these split into two commits - one to fix the mode stuff and one to add the new model, just to keep those things separate. But, I know you've been fighting git with this so I won't ask you to split them after the fact. I will squash your revision patches though before I commit (per the guidelines), and I'll do the locale re-gen at the same time which will make the last test pass.

Thanks!

[kk7ds]

Couldn't we make this elif self.FT450 and avoid a level of nesting below?

[kk7ds]

This is not the correct way to get a byte string. Just use data += b" " here to avoid the encoding.

[kk7ds]

These should be // 8 instead of / 8. The former returns an int and the latter a float. This is a change from python2 (when this code was originally written), but it would be better to use the proper thing in new code. There is compatibility code in bitwise to paper over this, but it's better if we don't dig the hole deeper.

[kk7ds]

This should be LOG. If we ever hit this case, it will generate an error since python doesn't resolve symbols until the code is actually run.

[kk7ds]

This doesn't look right to me because stx has just the internal memory object name in it, not something for presentation to the user. Also, we should not change the name of a setting based on the state of the memory as it can cause the UI to be out of sync with the driver and makes things less portable. Please leave it the way it was.

[kk7ds]

This would be changing the input parameters on a set operation, which you shouldn't do. This may have side-effects in the UI and should be considered out of bounds. So please remove this else.

[kk7ds]

Since you're adding a new class, just make this return False. For several years now, CHIRP appends a metadata blob to the end of the images to do deterministic detection of models. Drivers from before that need to still detect the old-style images, but no driver/model after that point need to. If you just return False here, you'll be saying "only ever detect an FT-450 image by metadata" which means your driver and images won't be confused for FT-450D ones.

There are lots of other examples of this in the code, but here's one for reference:

https://github.com/kk7ds/chirp/blob/master/chirp/drivers/btech.py#L4065-L4066

[WCurlew]

Please see my comments below. I agree with most, but not all of this. I have made most of the changes and they look OK, didn't seem to perturb anything. Please try loading the original FT450D test image with unmodified Chirp to show the IF width issue discussed below. Will wait on your responses before committing. Bill ---------- Original Message ---------- From: Dan Smith ***@***.***> To: kk7ds/chirp ***@***.***> Cc: WCurlew ***@***.***>, Mention ***@***.***> Subject: Re: [kk7ds/chirp] Bug # 10909 FT450 not recognized in FT-450D driver (PR #803) Date: Sat, 04 Nov 2023 08:07:57 -0700 @kk7ds requested changes on this pull request. Thanks for this. Looks pretty close, just a few things we need to get cleaned up and then it should be ready for merge. I appreciate your work on this, but if you want me to make these changes for you I can do that, although I'd rather you do it so you can test :) Also, I'd normally want to see these split into two commits - one to fix the mode stuff and one to add the new model, just to keep those things separate. But, I know you've been fighting git with this so I won't ask you to split them after the fact. I will squash your revision patches though before I commit (per the guidelines), and I'll do the locale re-gen at the same time which will make the last test pass. Thanks! In chirp/drivers/ft450d.py:

@@ -421,8 +430,27 @@ def _read(self, block, blocknum): # Remove the block number and checksum data = data[1:block + 1] else: # Use this info to decode a new Yaesu modelCouldn't we make this elif self.FT450 and avoid a level of nesting below?

Yes, I will make this change and test In chirp/drivers/ft450d.py:

@@ -421,8 +430,27 @@ def _read(self, block, blocknum): # Remove the block number and checksum data = data[1:block + 1] else: # Use this info to decode a new Yaesu model- raise Exception("Unable to read block %i expected %i got %i"- % (blocknum, block + 2, len(data)))+ # unique FT450 (and 450AT?) vs D, only 2 bytes in first block+ if self.FT450:+ if blocknum == 0 and len(data) == 4:+ checksum = yaesu_clone.YaesuChecksum(1, block - 2)+ if checksum.get_existing(data) != \\\\+ checksum.get_calculated(data):+ raise Exception("Checksum Failed [%02X<>%02X] block %02X" %+ (checksum.get_existing(data),+ checksum.get_calculated(data), blocknum))+ # Remove the block number and checksum+ data = data[1:block - 2 + 1]+ # pad the data to the correct 450d size+ data += bytes(" ", 'ascii')This is not the correct way to get a byte string. Just use data += b" " here to avoid the encoding.

Yes, I will make this change and test In chirp/drivers/ft450d.py:

@@ -593,9 +636,21 @@ def _get_special(self, number): _mem = self._memobj.mtune immutable = ["number", "extd_number", "name", "power"] elif mem.number in self.SPECIAL_PMS.values():- bitindex = (-self.LAST_PMS_INDEX) + mem.number- used = (self._memobj.pmsvisible >> bitindex) & 0x01- valid = (self._memobj.pmsfilled >> bitindex) & 0x01+ # ft450 does NOT use the pmsvisible or pmsfilled fields,+ # it uses the next 4 bits after the 500 normal memories+ # in the main visible and filled settings area.+ if self.FT450:+ pmsnum = 501 + (-self.LAST_PMS_INDEX) + mem.number+ used = (self._memobj.visible[(pmsnum - 1) / 8] >> \\\\These should be // 8 instead of / 8. The former returns an int and the latter a float. This is a change from python2 (when this code was originally written) , but it would be better to use the proper thing in new code. There is compatibility code in bitwise to paper over this, but it's better if we don't dig the hole deeper.

This was code I used from the original code regarding the testing for normal memories used and visible. This is all over the place in this code. Since I am not retesting the 450D model, I really didn't want to make changes to his original code. I will look for these to change them, but I believe we are greatly magnifying the risk to 450D users. In chirp/drivers/ft450d.py:

+ vx = 7 # CWR+ elif vx == 13:+ if _mem.mode2 == 0: # RTTY+ vx = 5 # Set to new "DIG"+ vx_data = 2 # RTTY+ elif _mem.mode2 == 1: # USER-L+ vx = 5 # set to new "DIG"+ vx_data = 0+ elif _mem.mode2 == 2: # USER-U variant+ vx = 5 # Set to new "DIG"+ vx_data = 1 # USER-U with Split?+ else:+ LOG.error("unknown combo of mem.mode 13 and mem mode 2: %s %s",+ vx,_mem.mode2)+ else:+ Log.error("unknown _mem.mode value : %s", vx)This should be LOG. If we ever hit this case, it will generate an error since python doesn't resolve symbols until the code is actually run.

OOPs, surprised this didn't throw a syntax error. I will correct this, searched and did not find any others. In chirp/drivers/ft450d.py:

@@ -842,7 +967,7 @@ def _get_memory(self, mem, _mem): options = ["2.5 kHz", "5.0 kHz"] vx = _mem.fm_width stx = "fm_width"- rs = RadioSetting(stx, "IF Bandpass Filter Width",+ rs = RadioSetting(stx, ("IF Bandpass Filter Width " + stx),This doesn't look right to me because stx has just the internal memory object name in it, not something for presentation to the user. Also, we should not change the name of a setting based on the state of the memory as it can cause the UI to be out of sync with the driver and makes things less portable. Please leave it the way it was.

The original code actually adds multiple columns to the GUI, one for each unique setting of str, (am width, fm width, sb width etc) but the user cannot tell what he is dealing with as they are all titled the same on the gui. I added the str label to the column titles so the user can tell what he is looking at, and understand why the values change when different modes are selected, and why some columns are locked out (disabled) in those cases. I.E. when mode FM is selected, the FM bandwidth opens up, and the others are disabled. This action is the way it worked before I got in here, I just made it look more sensible. I am not willing to try and re-factior his code to olnly have the 1 column that covers all the values, especially since the allowable widths are different for each (SB,FM,AM etc.) In chirp/drivers/ft450d.py:

+ _mem.mode2 = 2+ elif stx == "RTTY":+ if self.FT450:+ _mem.mode = 13+ _mem.mode2 = 0+ else:+ _mem.mode = 5+ _mem.mode2 = 0+ elif stx == "N/A":+ _mem.mode = 5+ _mem.mode2 = 2+ setting.value = "USER-U" # default moving to DIG mode+ else:+ LOG.error("In _set_memory invalid digital data type %s", stx)+ else:+ setting.value = "N/A"This would be changing the input parameters on a set operation, which you shouldn't do. This may have side-effects in the UI and should be considered out of bounds. So please remove this else.

No, I cannot change this. MODE and DATA MODE work together to actually define the MODE and MODE2 areas of radio memory, and cannot be left out of sync with each other. A valid combination must occur. This issue is that if we go from MODE DIG to FM for instance above this with the mode, we need to reset the DATA MODE to N/A. Otherwise, the get_memory will force the MODE back to DIG due to the DATA MODE value. Thus, once you hit DIG mode, you can never get out of it in the gui. It took me quite a while to make this work from either the MODE side or the DATA MODE side. In chirp/drivers/ft450d.py:

@@ -1489,3 +1674,19 @@ def set_settings(self, settings): except Exception: LOG.debug(element.get_name()) ***@***.***+class FT450Radio(FT450DRadio):+ """Yaesu FT-450"""+ FT450 = True+ MODEL = "FT-450"++ @classmethod+ def match_model(cls, filedata, filename):Since you're adding a new class, just make this return False. For several years now, CHIRP appends a metadata blob to the end of the images to do deterministic detection of models. Drivers from before that need to still detect the old-style images, but no driver/model after that point need to. If you just return False here, you'll be saying "only ever detect an FT-450 image by metadata" which means your driver and images won't be confused for FT-450D ones.

There are lots of other examples of this in the code, but here's one for reference: https://github.com/kk7ds/chirp/blob/master/chirp/drivers/btech.py#L4065-L4066 I'll make this change, but I don't understand this. It obviously requires an in depth knowledge of what Chirp code does under the covers. Is there a reference to this information? I originally added the MATCH MODEL class def to the FT450 class element to pass the TOX tests where they insure an image isn't referenced by multiple drivers. The test in the FT450D code it too loose and matches both the FT450 and the FT450D image. Should I be removing the other stuff in the FT450D match model code also? I have tested this with the "False" as you suggested and it does seem to work loading both test images, and the TOX test passed clean. &mdash; Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[kk7ds]

It would surely be easier if you would reply to the comments in the web UI so they maintain context. The email version of this is unreadable and replying to it in bulk in the web UI is very hard because it quotes your entire response as a single paragraph.

The original code actually adds multiple columns to the GUI, one for each unique setting of str, (am width, fm width, sb width etc) but the user cannot tell what he is dealing with as they are all titled the same on the gui. I added the str label to the column titles so the user can tell what he is looking at, and understand why the values change when different modes are selected, and why some columns are locked out (disabled) in those cases. I.E. when mode FM is selected, the FM bandwidth opens up, and the others are disabled. This action is the way it worked before I got in here, I just made it look more sensible. I am not willing to try and re-factior his code to olnly have the 1 column that covers all the values, especially since the allowable widths are different for each (SB,FM,AM etc.)

That appears to be a bug in the UI in the column expansion bit. If you look in right click -> properties, it appears correct. I'll have a look into why the UI is expanding the columns, as that feature/view is fairly new. However it looks terrible to have an unformatted internal memory string added to the label, so we should not do it. "IF Bandpass Filter Width" looks fine, but "IF Bandpass Filter Width fm_width" looks bad.

Especially since it's unrelated to all the other stuff you're doing here, please revert that bit to the way it was and I'll look at why it's displayed incorrectly in the UI. The driver shouldn't be adding multiple settings for that anyway, which is another thing I'll look in to refactoring.

No, I cannot change this. MODE and DATA MODE work together to actually define the MODE and MODE2 areas of radio memory, and cannot be left out of sync with each other. A valid combination must occur. This issue is that if we go from MODE DIG to FM for instance above this with the mode, we need to reset the DATA MODE to N/A. Otherwise, the get_memory will force the MODE back to DIG due to the DATA MODE value. Thus, once you hit DIG mode, you can never get out of it in the gui. It took me quite a while to make this work from either the MODE side or the DATA MODE side.

Okay, but I won't merge it in this state. You're altering internal state in the UI here, which is not okay. We have protections for that on the memory objects but not the settings (for reasons), which is why you're not getting an error here. Right after this is called, the UI also dumps and replaces its state with what gets returned by an equivalent get_memory() right after this. So not only is this not having any affect in what the user sees or controls, it also is out of bounds since this is state the was passed to this method, and should not be modified.

[kk7ds]

Ah, yep, it's right in the log traffic:

WARNING: Memory Memory VFOb-50M: 52.597000-0.100000 FM () r100.0* c100.0  d023 NN [5.00] does not have field extra.sb_width

The UI needs all the settings in each memory to be the same structure in order to expand them into columns (which is why adding and removing them based on other settings is not okay).

All that needs to be done to fix that is to only expose one width with the current value and set it in the right field based on the mode that is selected when it is changed. I'll follow up with that refactor, so please just drop that from your patch and I'll follow up with a proper fix and get the FT-450D owner to verify.

[WCurlew]

I believe the reason the UI is expanding the columns is because the internal name of the column is based on the mode_width. The IF Filter stuff is just a label. It's the value of str that makes the column unique. If you have multiple defined memories that have the 3 different modes (AM, FM, SSB (which counts for DIG too)) then a new column gets added each time a unique memory mode is processed. If you try to edit one that is not for the current "MODE" the gui throws an error as that setting is not available in mem.extra. Trying to use a single column is going to be very tricky, as the filter width options in the drop down box have to be different based on the mode, and it doesn't appear you can do that in a single column based on the memory row you are working on. Perhaps you would need to make all possible widths available in the column, and then edit for a bad choice based on mode in the code? BTW you would also then have to change the "default" if a different mode got selected and the current width option was not valid, and then you are in exactly the same circumstance I am in with MODE and DATA MODE, where a change in one may necessitate a change in the other.

[WCurlew]

You would not allow me to add new modes to chirp.common for this radio. I got around that using the DIG base mode and the DATA MODE new column. Now you won't let me do that either, because a change in mode may cause a change in DATA MODE that the user could not have selected yet. Short of using totally inappropriate fake mode names from the current chirp.common mode set for the data modes, I cannot see a solution for this, so I guess I am done with this effort. Sorry for all the confusion and time I have wasted.

[kk7ds]

Bill, I think you're misunderstanding what I'm saying. I'm definitely happy with the approach to add the new digital mode column, that's not the problem. Its just the modification of the UI's object in set_memory() that is not okay. It's also not having any effect, so it should be no problem to remove it. Please don't give up :)

[WCurlew]

I pulled the else clause setting N/A from the set_memory, I see that it was not required to insure the GUI was in sync, and it is covered in Get_memory, or at least it tested OK.

BIll

[kk7ds]

Thanks for those changes Bill. The new test failure is just because you're not rebased on yesterday's changes. Later today I'll pull this down, rebase, fix the locale stuff I previously mentioned and squash it to get ready for merge. If there's anything else I need I'll let you know, but I can probably take it from here.

[kk7ds]

We can ignore the (new) warning in the PR checks test about match_model. What you have here is fine (and what I asked for). Since we just had a build today I'll hold off merging this for a bit to see if other stuff comes around. I also need to rebase my bandwidth fix on this which will take a bit.

But, this is good to go now thanks.