[chirp_devel] [PATCH] [icomciv] Restructuring of IcomCIVRadio radio classes; breakout each radio into its own module in preparetion for simplifying support for upcomming CIV feature #4547

[Dan Smith]

>> 1) Changing the way probe_model() works to enumerate the global driver list instead of the local model list. This would have been a trivial change in and of itself, and is unrelated to the refactoring. Presumably this change is somehow necessitated by your approach to #4547, but it's not at all clear that it's a meaningful change. If it's needed at all, I would think it might make more sense as a part of the patch for #4547, and not as part of an unrelated refactoring.

> This change is precipitated by the requirement to support configurable civ's (#4547) such that we are no longer able to hard code these values into the code base. While I agree that it would be ideal to not include it as part of the refactoring effort, so as to reduce its scope, the refactoring itself would introduce a circular dependency on the hard coded classes and thus makes it a requirement of this change.  The directory method fwiw, is a fairly common practice in the codebase as it stands.

Yeah, I'm with Martin on this - the probe_model() change can and should come before the refactor. When I saw this patch yesterday, before the other comments, I had spotted functional changes and knew that it would need more investigation to make sure as little change came with the refactor as possible. I definitely don't want to mix the two.

> The intent of this change is to reduce potential conflicts amongst developers working on similar radio models.  Currently there are multiple implementations of icom live mode radios being introduced into the code base - this change should minimize the scope of those changes making it i) easier to develop and ii) reducing merge conflicts and iii) easier to test.

While there are multiple things going on right now, we've gone years where that hasn't been true. There's a lot of benefit of having them all in one file for contextual relativity, as well as consistency. It's a lot more likely that things will remain consistent in one file, and the drivers are so small that it seems a little silly to split them out, to me. I totally don't get how this split will improve testing. A compromise would be to merge your probe changes, which will allow for new models to be built in their own file instead of having to be in icomciv.py for the probe routine to work. Further, splitting each model one at a time would also make the manual verification a lot easier to do. Like Martin said, I don't have all of the models we support here available for testing, so I'd want to be extra sure we're not going to break any of them in the process.

Independent of whether or not we're going to merge the split, I'm not going to consider it until after Rick updates his 7300 patch. I asked him not to move things around in that file, so I'm definitely not going to move *everything* underneath him in the meantime :)

> Another outcome of this change is to allow for a clear isolation of radio specific implementation details from the base class, thus reducing the potential for knock on effects from a developer introducing a new radio model while unknowingly breaking another.

I can appreciate this argument, but as simple as these are, I'm not really sure I see this as a strong one.

> The root of the impending changes to support #4547 stems from the current implementation of how CIV values are handled, in that they are currently implemented as static variables and will need to become class variables.  As this is a significant deviation to the implementation of the base IcomCIVRadio class, this change will allow for the development of specific radio models mentioned above to continue unimpeded by that work.
> 
>> Most importantly, this change means the loss of history for all of the drivers currently in icomciv.py insofar as someone would have to know to look at the history of that module in order to find any history at all for all of the drivers being split out from here.
> 
> This particular situation arises anytime new source files are introduced into the code base; there is however precedence for this type of change on this code base, for example:
> 
> https://chirp.danplanet.com/projects/chirp/repository/revisions/d135e492dfa3. 
> 
> In our particular situation the history is not lost as in the previous revision. In fact, the icomciv.py file is retained in the repo along with its history.

Yeah the history is there, but it's really annoying to look beyond that point. That reorganization was a holistic restructuring at a single point in time. It stemmed from an original not-very-modular history where CHIRP supported like six icom VHF radios which clearly didn't scale to hundreds of models and vendors, and was required to get on a cleaner path. You could argue the same for icomciv.py I guess, but the cost/benefit doesn't strike me as worth it.

On the topic of the configurable address feature, I'll be honest and say I'm not sure why this is important. I've got several Icom CIV radios and have never needed to change the address on any of them, for any reason or because software required it. I feel like in the past when there was more purpose-built hardware that interfaced with these radios, it may have been important, but I'm not sure why it is now. I'm interested in what the proposed solution(s) are, because I'm sure I will have an opinion. I think probably the only thing I'd really be in favor of is a hidden config option to override the address used for extreme circumstances, where the user really needs to be able to control it. I don't think it's important enough to justify exposing through the driver model and into the UI. If there are some real use-cases, I'd like to hear them.

Anyway, as I mentioned, I'm not going to apply the reorg part of the patch underneath Rick while he's working to rebase his stuff. If you want to submit the probe change now to facilitate future drivers being outside of the main module (and presumably the configurable address thing), then that's fine.

--Dan