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

Soapyremote as a wrapper. #365

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alexander-sholohov
Copy link

With these changes soapyremote can act as a wrapper for all soapy-based sources.

@jketterl
Copy link
Owner

jketterl commented Jan 3, 2024

Once again, sorry for the late review.

This one probably still needs some more work. I see the basic idea, but this doesn't work out in the big picture.

As I said, this will need to integrate with the feature detection. SoapyRemote is an optional component, so offering the input without any checks if it is actually present will result in error. I'm not sure how this would really integrate properly since combining this with other sources breaks the way the current integration works. Adding a feature check before adding the field would be a first step, but then you'd still need some additional handling for when the config already contains the field, but the soapyremote feature is unavailable. It's probably not an easy modification to perform, but the other option is to make soapyremote a required feature, which appears to be kind of overkill to me.

The only other remark i have is that once this is integrated directly into the existing sources, the dedicated soapyremote source has no further use and can probably deleted. There is the option to do a config migration for any setup that has existing instances of soapyremote devices. it's probably best to just transparently update them to the new scheme instead of keeping the old source around.

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.

None yet

2 participants