Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ft2800.py - Add support for extended TX version of radio #729

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

aaknitt
Copy link
Contributor

@aaknitt aaknitt commented Jul 16, 2023

Add new class for radios with extended transmit modification (MARS/CAP) that uses a different ID_BLOCK value.

https://chirp.danplanet.com/issues/3919

Succesfully bench tested upload and download with an FT-2800M with TX mod.

@aaknitt aaknitt force-pushed the patch-1 branch 4 times, most recently from 4bc7d3f to 2c35db2 Compare July 17, 2023 02:47
@kk7ds
Copy link
Owner

kk7ds commented Jul 17, 2023

Thanks for this, and thanks for mirroring the other 1900 example directly. However, I hate that we ever did it that way and have regretted it ever since :)

I wonder if you would be willing to try to do this differently, or at least entertain an attempt? I'm thinking that we could, on download, wait for any IDBLOCK-looking thing and when we get one make sure it's either the normal or modified one, and then record that. Then we can either tack that onto the end of the image, or store it in the image metadata chunk. The driver can then look at that to know which limits to expose, and when you go to upload, use that to identify itself to the radio. That would allow us to have one driver entry for this radio (which is more appropriate) but still keep the radio happy. Should work right? The Icom ID-5100 driver supports three firmware revs of the radio this way, and some Kenwood and Yaesu drivers support persisting file format details in the metadata blob. What do you think?

Also, please put a bug reference in the commit (i.e. "Fixes: #3919") so the ticketing system will correlate the fix to the bug.

@aaknitt
Copy link
Contributor Author

aaknitt commented Jul 17, 2023

I wonder if you would be willing to try to do this differently, or at least entertain an attempt? I'm thinking that we could, on download, wait for any IDBLOCK-looking thing and when we get one make sure it's either the normal or modified one, and then record that. Then we can either tack that onto the end of the image, or store it in the image metadata chunk. The driver can then look at that to know which limits to expose, and when you go to upload, use that to identify itself to the radio. That would allow us to have one driver entry for this radio (which is more appropriate) but still keep the radio happy. Should work right? The Icom ID-5100 driver supports three firmware revs of the radio this way, and some Kenwood and Yaesu drivers support persisting file format details in the metadata blob. What do you think?

Sure, I'll need to educate myself on the image format and metadata chunk and look at some of the other examples you referenced. I'm still early on the learning curve of the overall software architecture and functionality. Would storing this data in the image then prevent using an image from one variant to write to a different variant? For example, if I have an unmodified radio that I read and save to an image...if it's pulling the IDBLOCK from that image, then it won't be able to write that same data to a modified radio, I think? Is that a problem we need to solve for, or just use Export/Import to manage that? I guess that same issue already exists with FT-1900 method, so maybe it's ok, but with today's approach you at least know you've selected a variant vs having it kind of occur under the hood in the image with no user visibility to it. Wondering if it could lead to confusion/complaints. Let me know what you think.

Also, please put a bug reference in the commit (i.e. "Fixes: #3919") so the ticketing system will correlate the fix to the bug.

Will do

@kk7ds
Copy link
Owner

kk7ds commented Jul 17, 2023

Sure, I'll need to educate myself on the image format and metadata chunk and look at some of the other examples you referenced. I'm still early on the learning curve of the overall software architecture and functionality.

Here's an example of a driver that uses it in a simple way:

https://github.com/kk7ds/chirp/blob/master/chirp/drivers/ft4.py#L689

You don't have to implement the getter/setter in the same way there, but you can. That driver basically has to do the same thing, but for geo reasons instead of modification reasons. Hopefully you can follow the tentacles of that in the file there and figure it out, but feel free to ask if you need help.

Would storing this data in the image then prevent using an image from one variant to write to a different variant? For example, if I have an unmodified radio that I read and save to an image...if it's pulling the IDBLOCK from that image, then it won't be able to write that same data to a modified radio, I think? Is that a problem we need to solve for, or just use Export/Import to manage that? I guess that same issue already exists with FT-1900 method, so maybe it's ok, but with today's approach you at least know you've selected a variant vs having it kind of occur under the hood in the image with no user visibility to it. Wondering if it could lead to confusion/complaints. Let me know what you think.

Yeah, it would prevent using a modified image on an unmodified radio and vice versa, but I think that's what Yaesu's intent was by making the radio identify as a different model once modified. That prevents you from doing a straight cable clone between the two, and is also why (IIRC) the radio must be reset post modification. We have cheap chinese radios supported in chirp where we can't tell the difference between one and another radio, and so we recommend people always keep the image tied to the radio and copy/paste memories between them. So I think there's prior art there, and I certainly have no problem fielding questions from users that complain about that. However, as you note, that's already a restriction of the multiple-class model that the 1900 started.

Thanks!

@aaknitt
Copy link
Contributor Author

aaknitt commented Jul 18, 2023

@kk7ds Take a look at this now and see if it's matching what you were thinking. It seems to be working well with a modified radio. I also manually edited the saved .img file to change the stored IDBLOCK value and after doing so it would not upload, which is what I would expect.

@aaknitt aaknitt changed the title Update ft2800.py - Add class for extended TX version of radio Update ft2800.py - Add support for extended TX version of radio Jul 18, 2023
Copy link
Owner

@kk7ds kk7ds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this looks pretty close, just a couple things. Thanks!

chirp/drivers/ft2800.py Outdated Show resolved Hide resolved
chirp/drivers/ft2800.py Outdated Show resolved Hide resolved
@kk7ds
Copy link
Owner

kk7ds commented Jul 18, 2023

Oh one other thing. This is a nit, but could you make the first line of the commit message a short description (like the title of this PR) and then a blank line, and any additional stuff like the bug reference? The first line goes out in the build email summary each night and that kinda needs to be a short single line that makes sense to match everything else.

@kk7ds
Copy link
Owner

kk7ds commented Jul 19, 2023

Cool, looks great now, thanks!

Add support for radios with extended transmit modification (MARS/CAP) that use a different ID_BLOCK value

Fixes: #3919
@kk7ds kk7ds merged commit 848e37b into kk7ds:master Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants