Page MenuHomeFreeBSD

beep(1): Initial version of utility to create terminal beep via soundcard.
ClosedPublic

Authored by hselasky on Oct 26 2021, 5:15 PM.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

hselasky created this revision.
hselasky added a reviewer: emaste.

Waveform can be adjusted with the WAVE_POWER macro. 0.5 means pure sinewave.

usr.sbin/beep/beep.c
63 ↗(On Diff #97493)

We have bool now

156 ↗(On Diff #97493)

B missing

hselasky marked 2 inline comments as done.

Add support and documentation for -B option.

Make gain logarithmic - suggested by @emaste .

Add missing free() - makes valgrind happy.

Advanced usage:

beep -B && beep -B -F 660 && beep -B -F 330

Excuse me, but I only have stylistic questions.

usr.sbin/beep/beep.c
27–39 ↗(On Diff #97497)

Sorry, I'm a bit too tired to double check against style(9), but these seem to be in wrong order.

67 ↗(On Diff #97497)

Why the ugly underscores?

90 ↗(On Diff #97497)

Is this Gray encoding, after Frank Gray?

164 ↗(On Diff #97497)

I may be wrong, but I think we want to avoid using atoi() in new code.

usr.sbin/beep/beep.8
3 ↗(On Diff #97497)

Aren't we dropping this from wherever we can legally?

usr.sbin/beep/beep.c
193 ↗(On Diff #97497)

is this daemon(3)? That returns an int, which would mandate an explicit comparison.

usr.sbin/beep/beep.c
164 ↗(On Diff #97497)

Indeed atoi(3) says

The atoi() function has been deprecated by strtol() and should not be
used in new code.
usr.sbin/beep/beep.8
3 ↗(On Diff #97497)

Yeah the standard copyright template for new code no longer has this. It's not a big deal but serves no purpose so if there's no other reason to include it (e.g. your employer requires it) then I'd leave it out.

Thanks for all comments. I'll address them later today.

usr.sbin/beep/beep.c
90 ↗(On Diff #97497)

Yes, I'll fix the spelling.

usr.sbin/beep/beep.c
67 ↗(On Diff #97497)

I'll fix it.

hselasky added inline comments.
usr.sbin/beep/beep.8
3 ↗(On Diff #97497)

Fixed.

usr.sbin/beep/beep.c
27–39 ↗(On Diff #97497)

All includes are now sorted by name.

164 ↗(On Diff #97497)

Fixed.

hselasky marked 2 inline comments as done.

Handle comments.

Very minor nits, ok to ignore.

usr.sbin/beep/beep.c
38 ↗(On Diff #97527)

"Kernel includes come first"

97 ↗(On Diff #97527)

Subjective note, but this looks awkward to me, especially with the quotes. I would just call it as everyone else: "apply Gray code".

usr.sbin/beep/beep.c
38 ↗(On Diff #97527)

clang-format says:

include <sys/soundcard.h>

#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <math.h>
#include <paths.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

Will handle the comments tomorrow.

After some IRC discussion, should be /usr/bin/beep, beep(1) perhaps?

hselasky marked an inline comment as done.
hselasky retitled this revision from beep(8): Initial version of utility to create terminal beep via soundcard. to beep(1): Initial version of utility to create terminal beep via soundcard..

Address all comments so far.

usr.bin/beep/beep.1
50

I don't have the authority to say for sure, but I think we should use space to separate the numbers from the units here and below.

pstef added inline comments.
usr.bin/beep/beep.1
61

Typo, inclusively.

This revision is now accepted and ready to land.Oct 29 2021, 5:48 PM
This revision now requires review to proceed.Oct 29 2021, 5:51 PM

Are we good to go for the initial version of beep(1) ? Please accept!

This revision is now accepted and ready to land.Nov 3 2021, 10:31 PM

LGTM, a couple of minor comments on the man page that you can consider

usr.bin/beep/beep.1
31

play a beep (as you use in the text below) reads nicer to me -- "play beep on soundcard" or even just "play a beep sound"?

44

s/to the/on the/ maybe