Happy to break the change apart and wait for Rick as well...
Some further comments inline below.
Thanks Dan.
On Sat, May 1, 2021 at 10:18 AM Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
- 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.
In terms of the CIV address change - it really is a question of user experience. First, each model of icom radios are given a unique ci-v address. ICom supports configuration of their CI-V address for the situation where multiple radios are connected to the same cable in order to be able to address each one individually - much like multiple peripherals on an I2C bus. But I suspect you knew that and really doubt that any chirp user is suffering from this limitation... most likely true.
As such, while there is technically nothing wrong with simply supporting the default ci-v address, the scenario which presents itself in chirp is that in cases of communication failures due to a misconfigured ci-v address there is no indication to the user as to which address chirp is utilizing and as such this has led users to believe it to be an issue with their cable and frustration ensues (no surprise there) .
It would also be a fair argument to say that at this point neither of these scenarios are a common occurrence and that how or if this features needs to be exposed into the UI or at all is of course a matter of discussion. I will add however, that laying the groundwork to support configurable ci-v addresses, opens the door for additional per 'radio model' configurable attributes to be utilized in the future; such as baud-rate, etc..
Without actually advocating for anything, here are a couple potential scenarios:
1. Do nothing for the minority of users impacted by this issue and hope they figure out to change their CI-V on their own. 2. Improve the error messaging to present the end-user with additional diagnostic information regarding their failed connection (may not be a bad idea either way). 3. Implement radio model attributes to support ci-v addresses - keeping the hard-coded defaults in place while allowing for them to be overridden by an optional user defined config file. 4. Exposing configurable attributes mentioned above in the UI in some manner that makes sense.
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 _______________________________________________ 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