[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

Sat May 1 22:33:14 PDT 2021

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 at intrepid.danplanet.com> wrote:

> >> 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.
>
>
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 at intrepid.danplanet.com
> http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
> Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20210501/788003ab/attachment-0001.html 