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

Add additional AOS and LOS functionality #192

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

srcejon
Copy link

@srcejon srcejon commented Feb 13, 2020

This patch adds the ability for a user to define a ; separated list of rigctrld commands to be sent to the radio on AOS or LOS. E.g. set_powerstat 0/1 to turn on/off the radio.

It also adds the ability to send a user-defined command when tuning to a transponder. This allows the appropriate mode, bandwidth and antenna to be set automatically. E.g. set_mode FM, 10000 ; set_ant 0

The AOS/LOS commands can be defined in the GUI. Currently, the transponder commands need to be defined in the .trsp file, with the COMMAND key. E.g:

33591.trsp
[APT Downlink]
DOWN_LOW=137100000
MODE=APT
COMMAND=set_mode WFM, 40000;set_ant 0

Eventually this should be added to the GUI, but I thought I'd see if this patch was agreed to first.

This feature was requested in Issue #182

Add support for sending user-defined rigctrl commands when transponder
is set. (Part of Issue csete#182)
Allow elevation for AOS and LOS to be set to something other than 0 degrees. Issue 167.
Signal LOS when autotrack is enabled. Issue 193.
Update Windows makefile to work with latest version of msys2. Issue 110.
@srcejon
Copy link
Author

srcejon commented Feb 18, 2020

I've updated the patch to include:

Support playing audio files on AOS and LOS. Issue #187.
Allow elevation for AOS and LOS to be set to something other than 0 degrees. Issue #167.
Signal LOS when autotrack is enabled. Issue #193.
Update Windows makefile to work with latest version of msys2. Issue #110.

@srcejon srcejon changed the title Add support for sending user-defined rigctrld commands on AOS, LOS or transponder tuning Add additional AOS and LOS functionality Feb 18, 2020
@srcejon srcejon requested a review from csete February 18, 2020 14:52
@srcejon
Copy link
Author

srcejon commented Feb 21, 2020

Another update allows applications (shell commands) to be run on AOS and LOS as well (perhaps to run a decoder, or control some other h/w), as requested on the forum.

Note, on Windows, if you want the command to run asynchronously, prefix it with start. On linux, add & at the end.

@csete
Copy link
Owner

csete commented Feb 24, 2020

Thanks for the patch and sorry for the late reply. Unfortunately, I'm busy with other things at the moment but I will take a look at it as soon as I can. However, there is a large backlog of critical bugs that will need my attention before adding new features.

I took a very brief glance and I can see that you have pushed 4 different feature additions into one pull request, resulting in a patch that adds 943 lines of code. That's not good! Please submit separate PRs for each new feature and bug fix so that they can be reviewed and tested individually.

Thanks, and again sorry for not being able to process these at the moment.

Alex

@srcejon
Copy link
Author

srcejon commented Feb 24, 2020

Hi Alex.

These are all on one branch because they all touch the same bits of code, but are addressed seperately in each commit, so you can see (and merge) individual diffs here: https://github.com/csete/gpredict/pull/192/commits - You'd get merge conflicts if they were each on a separate branch. (The other changes I've made (for ground tracks etc) are on a separate branch, as they are independent.)

There are already existing PRs for what is covered, referenced above. #167, #182, #187, #193. I've also just added #198.

@csete
Copy link
Owner

csete commented May 16, 2020

I know very well how git and github works, and I also know that even if I did as you suggest it wouldn't change the fact that commit d391ce9 addresses four separate issues, three bugfixes and a new feature that plays audio. By the way, that last part is a show stopper for the whole thing because I am not going to accept GStreamer as a dependency for playing audio files.

Sorry, but I can not review this in its present form.

@srcejon
Copy link
Author

srcejon commented May 16, 2020

gstreamer is added as an optional package - so it can be built without it. Is there a better cross platform library? gstreamer built for both Linux and windows successfully.

@csete
Copy link
Owner

csete commented May 17, 2020

My primary concern with gstreamer as a dependency is that it is not just a simple library but a whole framework for audio and video processing and therefore it feels like overkill for just playing audio. As far as I recall, the library only provides the framework, then it needs plugins for pretty much everything like audio interfacing, file loading, etc. And all those plugins probably have their own dependencies and every single dependency adds complexity when it comes to distribution, testing, and maintenance.

Anyhow, gpredict is now soon 20 years old and there is a lot of bug fixing, refactoring, and finishing partially implemented features that need to be done before new features can have any priority. The next couple of releases will most likely be maintenance releases that address these issues.

spsvihla pushed a commit to lasp/gpredict that referenced this pull request Nov 29, 2022
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