Skip to content

Man edits and more #274

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

Closed
wants to merge 12 commits into from
Closed

Man edits and more #274

wants to merge 12 commits into from

Conversation

N0NB
Copy link
Member

@N0NB N0NB commented Aug 30, 2021

This is related to issue 272 (please do not close it yet!)

There are some script fixes and a simple implementation of Escape only stops TX on the first press which likely needs more refinement.

I'm requesting review and discussion.

N0NB added 8 commits August 21, 2021 20:54
Edit for recommended practices of formatting manual pages.

Added formatting tips and documentation references at the top of the
file.

Completed up through the USAGE section.
Presumably this was a typo and should just be an alteration test for a
local versus the file located in the user's PATH.
As installation places the soundlog script into ${prefix}/bin,
avoid requiring the user to copy it to the ~/tlf/soundlogs directory.

It works just fine from PATH when called from inside the directory.
The -w option on the rec commandline probably dates from the Open Sound
System (OSS) version of rec.  This option is not recognized by the SoX
version of rec which is the only version available on modern Debian.
This command line has been commented out so users can restore it easily
if needed.

Instead add an example command for Sox rec as the default.  The -c 1
option forces single channel recording.  This needs to be documented in
the manual page so users can remove it as two channel recording is the
default for this version of rec.  Two channel recording may be useful
for transceivers that output two channel audio, such as the Elecraft K3.

The -q option makes the rec utility quiet, quelling its normal output.
The opendir() function is not in a context where it understands
the value of $HOME, unlike the system() calls used elsewhere which
operate in a user's shell environment.  To make this work, query the
environment for the value of HOME and concatenate it to the hard coded
directory and pass that to opendir().

Add some error handling so if in the future the directory is exposed to
user configuration, the user won't be completely in the dark if there is
a typo in the entered path.
First try at only stopping TX when Escape is pressed the first time.
This is very simplistic and likely will need more refinement for general
deployment.
@N0NB N0NB requested review from dl1jbe and zcsahok August 30, 2021 01:21
@N0NB N0NB linked an issue Aug 30, 2021 that may be closed by this pull request
char *path = g_strdup_printf("%s%s",
g_getenv("HOME"),
"/tlf/soundlogs");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A point we should think about: Until now no other part of TLF requires a dedicated ~/tlf directory. Do we like to have a central one or keep our old tradition, that 'soundlog' should be a subdirectory of the actual contest directory in use?

Copy link
Member

@zcsahok zcsahok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments and a question.

@@ -258,6 +258,9 @@ int cwstart = 0; /**< number characters after which
int sending_call = 0;
int early_started = 0; /**< 1 if sending call started early,
strlen(hiscall)>cwstart or 'space' */
int escape_pressed = 0; /**< 0. Escape not last key pressed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer using bool for actually boolean values. Also x = true or if (x) {../if (!x) {.. makes code a bit more readable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Originally I'd planned on the values 0-3 for counting all presses until all fields are cleared. Then I decided that isn't necessary.

As stated, this PR is mostly for review and discussion. I say cherry pick anything that's good and improve/discard the rest as needed.

@@ -147,6 +147,7 @@ int callinput(void) {
int j, t, x = 0;
char instring[2] = { '\0', '\0' };
static int lastwindow;
extern int escape_pressed;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add globals to globalvars.h. This avoids having to list all used globals separately.

Copy link
Member Author

@N0NB N0NB Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be useful.

@@ -796,9 +801,15 @@ int callinput(void) {
// <Escape>, clear call input or stop sending.
case ESCAPE: {
if (early_started == 0) {
if (escape_pressed == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the point of using escape_pressed is to stoptx() for the first ESC and cleanup() + clear_display() for the second?

Copy link
Member Author

@N0NB N0NB Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

I find the combination of stopping TX and clearing the exchange field to be rather irritating. Other programs I've used never did that, in fact they only used Escape to stop TX and another key/combination to clear the fields.

For CW this worked very well this past weekend. I gave up on stopping the voice keyer as per Tom and my discussion on the mailing list. I still think that should be implemented but there is likely a better way than the hack I came up with (not part of this PR).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it took some time for me to figure out. :-)
How about putting it more explicitly, something like

if (x==ESC) { ++esc_count } else {esc_count=0}

switch(x) {
  case ESC:
      if (esc_count==1) { stop_tx() }
      else if (esc_count>1) { clear... }

Would there be a way to avoid code duplication in callinput and getexechange? I mean some keys should behave the same, so it could warrant a common code. Not sure how to do it best.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks clean to me.

Sometimes there are subtle differences depending on whether a key is input in the call input or exchange field. I suppose that can be worked out but it would be a big refactor and I'm not sure what the gain would be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factor out the handling of common keys is something we should go for.

For the keys which are handled differently we could even add a state variable telling us the actual input field we are in.

@N0NB
Copy link
Member Author

N0NB commented Sep 9, 2021

Tom, please merge all of the commits except f9be31b, especially the two for the manual page updates.

I have also modified doc/Makefile.am to distribute station-sample.cbr.

@N0NB
Copy link
Member Author

N0NB commented Sep 9, 2021

My apologies for mixing PR topics.

@dl1jbe
Copy link
Member

dl1jbe commented Sep 13, 2021

Seems mail mail from Saturday did not made it as a reply :-(, sorry. Will pick up the commit shortly. I think it is ok to merge related commits (e.g. the man page updates).

@dl1jbe
Copy link
Member

dl1jbe commented Sep 13, 2021

A question came up for ef0696c: The change does not simply allow having 'soundlog' in PATH - instead it is now REQUIRED to be in PATH.

Before I commit it - do we want to keep it so?

@N0NB
Copy link
Member Author

N0NB commented Sep 13, 2021

Tom,
That could be changed in the same way play_vk is handled with a test to see if it is in the working directory and then use the installed version if not.

@dl1jbe
Copy link
Member

dl1jbe commented Sep 14, 2021

Ok, I'll pick up the actual solution and take the extension on the todo list.

@dl1jbe
Copy link
Member

dl1jbe commented Sep 14, 2021

Cherry-picked, squashed and applied all commits (besides f9be31b) as requested.

@dl1jbe
Copy link
Member

dl1jbe commented Sep 21, 2021

Hi Nate, while working on the $HOME problem did you have success with playing the recorded sound log files?
It seems that the command used in the code are also from old OSS play command.

The whole sound handling is in a bad shape. Some parts use hardcoded script names, some use hard coded command strings and some use external command given by logcfg.dat. We should choose a common mode for determining the play and record commands to use.

What makes it more demanding is that we need to record from different sources (microphone or rig) and also have to play to different sinks (radio or speaker). Any idea how to specify that in SoX?

@N0NB
Copy link
Member Author

N0NB commented Sep 21, 2021

Hi Nate, while working on the $HOME problem did you have success with playing the recorded sound log files?
It seems that the command used in the code are also from old OSS play command.

Once I changed things in the code, yes, it worked. I don't think I pushed that stuff out, though.

The whole sound handling is in a bad shape. Some parts use hardcoded script names, some use hard coded command strings and some use external command given by logcfg.dat. We should choose a common mode for determining the play and record commands to use.

I would think that opting for a default that can be overridden by a setting in logcfg.dat would be preferable. Let the commands default to the SoX utilities and the user can change them as needed.

What makes it more demanding is that we need to record from different sources (microphone or rig) and also have to play to different sinks (radio or speaker). Any idea how to specify that in SoX?

That, I don't know. As I run Gnome and PulseAudio and run Tlf in an Xterm, I use the pavucontrol GTK+ utility to direct the sound streams as I need. There are probably ways to write a configuration file for PA and then there will be the cases where PA is not used and the user will need to rely on the ALSA configuration.

@dl1jbe
Copy link
Member

dl1jbe commented Sep 22, 2021

Hi Nate, while working on the $HOME problem did you have success with playing the recorded sound log files?
It seems that the command used in the code are also from old OSS play command.

Once I changed things in the code, yes, it worked. I don't think I pushed that stuff out, though.

Ok, I will look into it.

The whole sound handling is in a bad shape. Some parts use hardcoded script names, some use hard coded command strings and some use external command given by logcfg.dat. We should choose a common mode for determining the play and record commands to use.

I would think that opting for a default that can be overridden by a setting in logcfg.dat would be preferable. Let the commands default to the SoX utilities and the user can change them as needed.

Same preference here. I will go that way.

What makes it more demanding is that we need to record from different sources (microphone or rig) and also have to play to different sinks (radio or speaker). Any idea how to specify that in SoX?

That, I don't know. As I run Gnome and PulseAudio and run Tlf in an Xterm, I use the pavucontrol GTK+ utility to direct the sound streams as I need. There are probably ways to write a configuration file for PA and then there will be the cases where PA is not used and the user will need to rely on the ALSA configuration.

I did some more research and I found some ways to use different sound devices at the same time. Just have
to make some tests which way to go best.

@dl1jbe
Copy link
Member

dl1jbe commented Sep 23, 2021

Just found how to control the device for recording and/playing sound.
You can stat it on SoX command line:

sox <file> -t alsa hw:1,0 or
sox <file> -t pulseaudio devicename........

will do. I will finally add these to the man page.

.B QSO
or
.B DXPED
modes).
.
.TP
.B IGNOREDUPES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just came across this: it's spelled in singular, IGNOREDUPE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

@zcsahok
Copy link
Member

zcsahok commented Feb 26, 2022

Could this be merged? Master is drifting away...

@dl1jbe
Copy link
Member

dl1jbe commented Feb 26, 2022

Could this be merged? Master is drifting away...

Accept the commit mentioned by Nate

Tom, please merge all of the commits except f9be31b, especially the two for the manual page updates.

all got merged in already in commit e26dfde in September last year.

I kept the PR open to kept the discussion about the audio recording around until PR #276 got finished. That one is overdue and I had planned to complete it in next weeks after finishing the move to use qso_array instead of qsos[] (PR #305).

So please let us wait with closing the PR here for some days longer.

@N0NB
Copy link
Member Author

N0NB commented Feb 26, 2022 via email

@dl1jbe
Copy link
Member

dl1jbe commented Feb 27, 2022

Nate, there is an unfinished branch stop_tx in PR #276 which I plan to complete next.

In it I made the soundlog directory configurable by keyword in logcfg.dat. At the moment the default is '~/tlf/soundlogs' but I agree that just './soundlogs' would be better. That can be easily done.

@N0NB
Copy link
Member Author

N0NB commented Feb 27, 2022 via email

@zcsahok
Copy link
Member

zcsahok commented Feb 27, 2022

I was not closely following this PR, sorry.
A local dir ('./soundlog') would fit surely better.

@dl1jbe
Copy link
Member

dl1jbe commented Mar 4, 2022

With PR #317 and #318 in the pipe we can close these PR now. Thanks for all the discussion.

@dl1jbe dl1jbe closed this Mar 4, 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.

Rework sound recorder and playback.
3 participants