[chirp_devel] Patch: Icom IC-2730A
Hi,
I've attached a patch file for the Icom IC-2730A (fixes #2745). There is also an image attached to the bug for tests.
Please review and send feedback, especially on any suggested changes to how I've modified icf.py to support this radio, as it differs from previous Icom radios.
73, Rhett Robinson AG7KJ
Hi Rhett,
Sorry this took so long to get to.
Note that if you can inline the patches (like the patchbomb extension for hg will do for you) it's a lot easier for me to review them. It's a minor thing, but...it helps :)
I've attached a patch file for the Icom IC-2730A (fixes #2745). There is also an image attached to the bug for tests.
Please review and send feedback, especially on any suggested changes to how I've modified icf.py to support this radio, as it differs from previous Icom radios.
I'm really surprised to see some of the changes to the icf driver. One of the nicest thing about icom radios is that they all use the same (albeit kinda silly) protocol. Hopefully they're just switching to what you have done and we won't see a wide divergence going forward.
Would it be possible to refactor the clone mode driver into a base one that implements the original protocol and then a new subclass "raw" one? That might mean changing some of the module-level things to instance methods so you can override them in the subclass. I think that would be cleaner than passing the flags around. It would also be nice to have that cleanup/refactor as a separate patch from your driver add. Again, the patchbomb extension makes it easy to send a stack of patches. Info here:
https://chirp.danplanet.com/projects/chirp/wiki/DevelopersProcess#Sending-a-...
That said, the driver add looks reasonable.
Let me know what you think about the refactor and then we can go forward (and I'll try not to be so tardy in handling it).
Thanks!
--Dan
Hi Dan, thanks for the feedback. I was also surprised at the differences (especially for the first time I've done this!), so like you said, hopefully this is a one-time pretty standard change going forward.
I've refactored the changes into two patches: one that adds raw support for icf.py, and one that adds the driver. I also got my hg setup to do the patchbomb, so I'll send the patches that way to the mailing list in just a minute. Note I also found a bug in my code doing this, so, yay! Perhaps not too surprisingly, in this new raw mode, the radio escapes control bytes sent to the computer just as we're expected to escape control bytes sent to the radio.
Cheers, Rhett
On Sun, Jan 14, 2018 at 11:36 AM, Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
Hi Rhett,
Sorry this took so long to get to.
Note that if you can inline the patches (like the patchbomb extension for hg will do for you) it's a lot easier for me to review them. It's a minor thing, but...it helps :)
I've attached a patch file for the Icom IC-2730A (fixes #2745). There is
also an image attached to the bug for tests.
Please review and send feedback, especially on any suggested changes to
how I've modified icf.py to support this radio, as it differs from previous Icom radios.
I'm really surprised to see some of the changes to the icf driver. One of the nicest thing about icom radios is that they all use the same (albeit kinda silly) protocol. Hopefully they're just switching to what you have done and we won't see a wide divergence going forward.
Would it be possible to refactor the clone mode driver into a base one that implements the original protocol and then a new subclass "raw" one? That might mean changing some of the module-level things to instance methods so you can override them in the subclass. I think that would be cleaner than passing the flags around. It would also be nice to have that cleanup/refactor as a separate patch from your driver add. Again, the patchbomb extension makes it easy to send a stack of patches. Info here:
https://chirp.danplanet.com/projects/chirp/wiki/ DevelopersProcess#Sending-a-change
That said, the driver add looks reasonable.
Let me know what you think about the refactor and then we can go forward (and I'll try not to be so tardy in handling it).
Thanks!
--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
Alright, I also realized that hg email tip only does one patch at a time, so I sent this out as two patches. Let me know if there's a better way to send these out together. Thanks again!
On Sun, Jan 14, 2018 at 2:58 PM, Rhett Robinson rrhett@gmail.com wrote:
Hi Dan, thanks for the feedback. I was also surprised at the differences (especially for the first time I've done this!), so like you said, hopefully this is a one-time pretty standard change going forward.
I've refactored the changes into two patches: one that adds raw support for icf.py, and one that adds the driver. I also got my hg setup to do the patchbomb, so I'll send the patches that way to the mailing list in just a minute. Note I also found a bug in my code doing this, so, yay! Perhaps not too surprisingly, in this new raw mode, the radio escapes control bytes sent to the computer just as we're expected to escape control bytes sent to the radio.
Cheers, Rhett
On Sun, Jan 14, 2018 at 11:36 AM, Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
Hi Rhett,
Sorry this took so long to get to.
Note that if you can inline the patches (like the patchbomb extension for hg will do for you) it's a lot easier for me to review them. It's a minor thing, but...it helps :)
I've attached a patch file for the Icom IC-2730A (fixes #2745). There
is also an image attached to the bug for tests.
Please review and send feedback, especially on any suggested changes to
how I've modified icf.py to support this radio, as it differs from previous Icom radios.
I'm really surprised to see some of the changes to the icf driver. One of the nicest thing about icom radios is that they all use the same (albeit kinda silly) protocol. Hopefully they're just switching to what you have done and we won't see a wide divergence going forward.
Would it be possible to refactor the clone mode driver into a base one that implements the original protocol and then a new subclass "raw" one? That might mean changing some of the module-level things to instance methods so you can override them in the subclass. I think that would be cleaner than passing the flags around. It would also be nice to have that cleanup/refactor as a separate patch from your driver add. Again, the patchbomb extension makes it easy to send a stack of patches. Info here:
https://chirp.danplanet.com/projects/chirp/wiki/DevelopersPr ocess#Sending-a-change
That said, the driver add looks reasonable.
Let me know what you think about the refactor and then we can go forward (and I'll try not to be so tardy in handling it).
Thanks!
--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
Alright, I also realized that hg email tip only does one patch at a time, so I sent this out as two patches. Let me know if there's a better way to send these out together. Thanks again!
I only see the one driver patch, but I think what you're looking for is something like:
hg email tip~1..
--Dan
I sent both the update to icf.py and the new ic2730.py file. Note that I forgot to update the description (and mentioned that in a second email) so it might look like just one patch, but there are two hidden in the same subject.
This one updates the icf.py: http://intrepid.danplanet.com/pipermail/chirp_devel/2018-January/004897.html And this one adds the new driver: http://intrepid.danplanet.com/pipermail/chirp_devel/2018-January/004899.html
Thanks, Rhett
On Sun, Jan 14, 2018 at 3:20 PM, Dan Smith dsmith@danplanet.com wrote:
Alright, I also realized that hg email tip only does one patch at a
time, so I sent this out as two patches. Let me know if there's a better way to send these out together. Thanks again!
I only see the one driver patch, but I think what you're looking for is something like:
hg email tip~1..
--Dan
I sent both the update to icf.py and the new ic2730.py file. Note that I forgot to update the description (and mentioned that in a second email) so it might look like just one patch, but there are two hidden in the same subject.
Oh, yep, I read too fast and thought you said you were re-sending to correct something but didn't look at the body of the patch.
So, I can hand-edit it as I apply, but it'll change the hash and everything. Can you re-send just the icf patch with the updated description just so I can cleanly apply it without editing? It seems like you're around and can do that quick. If I don't see it in the next hour or two I'll just do it myself and slam it in.
Then I can apply the 2730 patch atop that and be good to go.
Thanks!
--Dan
Done!
On Sun, Jan 14, 2018 at 3:34 PM, Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
I sent both the update to icf.py and the new ic2730.py file. Note that I
forgot to update the description (and mentioned that in a second email) so it might look like just one patch, but there are two hidden in the same subject.
Oh, yep, I read too fast and thought you said you were re-sending to correct something but didn't look at the body of the patch.
So, I can hand-edit it as I apply, but it'll change the hash and everything. Can you re-send just the icf patch with the updated description just so I can cleanly apply it without editing? It seems like you're around and can do that quick. If I don't see it in the next hour or two I'll just do it myself and slam it in.
Then I can apply the 2730 patch atop that and be good to go.
Thanks!
--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
Done!
Awesome, thanks. I just pushed all that up.
Did you happen to have an older Icom to test the ICF changes with just to make sure that still works? If not, we should try to get someone on the ML to do a quick sniff test with something once the new build is out tomorrow just to be sure.
Thanks!
--Dan
Alas, I do not, else I definitely would have manually tested them along with the change.
On Sun, Jan 14, 2018 at 4:12 PM, Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
Done!
Awesome, thanks. I just pushed all that up.
Did you happen to have an older Icom to test the ICF changes with just to make sure that still works? If not, we should try to get someone on the ML to do a quick sniff test with something once the new build is out tomorrow just to be sure.
Thanks!
--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
participants (2)
-
Dan Smith -
Rhett Robinson