[beep] Add -f and -l option

[tyama501]

Hi @ghaerr and @floriangit ,

I added -f option for frequency and -l option for duration.

I could play beginning of Happy Birthday referring to
https://github.com/ShaneMcC/beeps/blob/master/happybirthday.sh

ELKS_PC98_beep_happy_NP21W.mp4

-n is not supported, so we need to write "beep" in each line of script.
Also, the frequency needs to be integer.

Enjoy.
Thank you!

[tyama501]

Oops, Line 76 should be ==.
I will fix it.

[ghaerr]

Since duration is declared long, this should probably use atol here. A similar issue could apply with freq, which is only valid to 32767. So beep and freq might want to be declared beep(unsigned int freq).

In order to keep the code file quite small, one might want to use atol in both places, and truncate the long value into an unsigned int for freq.

[ghaerr]

Now I notice that freq is used as following in beep:

d = 2457600 / freq;

which means freq will have to be converted to long anyways. Thus freq should probably be declared long in both beep and the main code, and also have the above rewritten to:

d = 2457600L / freq;

The reason for this is that the ia16-elf-gcc compiler sometimes does the wrong thing with int constants that are > 65535.

[floriangit]

The reason for this is that the ia16-elf-gcc compiler sometimes does the wrong thing with int constants that are bigger than 65535.

Hmm, are you sure about this bigger than 65535? I don't remember now exactly, but in main.c I suffixed L to the duration hardcoded value and to the hardcoded value in usleep. And IIRC there was a necessity to it I did not understand at that time (I thought "oh wait 16bit, that's why....?"), both are well below 65535...sorry I cannot verify my theory right now.

[ghaerr]

In C, types that are different sizes (e.g. char, int, long) or are different signedness (e.g. int vs unsigned int) are automatically promoted to 1) the size of the largest sized element in the expression, and 2) to unsigned if any element is unsigned. Types that are passed to functions are also automatically promoted to the defined argument signedness and size.

For "integer" constants, the question is: what is an integer? For ia16-elf-gcc, it is int, which is 16-bits. This can mean that the compiler silently truncates a long to an int if there are no other elements in the expression, or if it is not being passed to a pre-defined function. This also occurs with signedness: 65535 requires a long, even though an unsigned int could take that value. So is it signed or unsigned? IIRC, the compiler assumes int unless otherwise expressed, which would mean that the value gets truncated to 16 bits. This may or may not be what is desired, as 65535 still fits in 16-bits if the value is interpreted as unsigned. However, if an additional element is type int and not long, then the arithmetic won't be done in 32 bits.

For these reasons, as well as clarity, in the ELKS kernel, most all literal constants > 32767 are suffixed with L so as to ensure proper internal and external computation. In order to know for sure, one can look at the ASM output using something like:

$ ia16-elf-objdump -D -r -Mi8086 beep.o

[floriangit]

Thanks @ghaerr !

[floriangit]

freq, which is only valid to 32767

Indeed dogs are able to hear those frequencies (I cannot). Of course ELKS users could use beep for their dogs, so 👍 to better fix it! :-)

[tyama501]

Thank you!

[ghaerr]

-n is not supported, so we need to write "beep" in each line of script.

Why not support -n so that the sound scripts can be run unmodified? :)

[ghaerr]

Why not support -n so that the sound scripts can be run unmodified? :)

Oops - I see - they use floating point numbers. Well, the float portion could be ignored, and I think atol will properly return just the integer portion of the floating point number.

[tyama501]

Hi @ghaerr and @floriangit

I have added -n option.
Now, I could play the happy birthday without modification.

Here is the sound from PC-9801UV21.

ELKS_PC98_beep_happy_PC-9801UV21.mp4

Thank you.

[floriangit]

Great stuff @tyama501. We could add in beep.c:2 your contributions if you want. Also beep.c:4 could have an update regarding the supported cmdline options. A small "-h" command line option "usage" with an example to Happy Birthday in main() maybe as well? I can do those cleanups if you want after Greg merged your changes?

Ouh and Happy Birthday NEC-PC98!

[ghaerr]

Looks great @tyama501! Let me know if you'd like to merge now, or add anything @floriangit suggested.

[tyama501]

Thank you @floriangit

That would be nice to add option usage.

The happy birthday, I just runned sh script from the web ./happybirth.

@ghaerr , please merge it.

Thanks

[tyama501]

I forgot to mention that it seems that the size gets tripled if printf is used.
Are there small printf for usage?

[ghaerr]

I forgot to mention that it seems that the size gets tripled if printf is used.
Are there small printf for usage?

Thanks for bringing that up - we don't want to use printf unless really necessary. There's two ways we can write tiny commands for ELKS, one uses a macro to display messages to stderr using write like this (see elkcmd/file_utils/mkdir.c for example):

#define errmsg(str) write(STDERR_FILENO, str, sizeof(str) - 1)
#define errstr(str) write(STDERR_FILENO, str, strlen(str))
...
errmsg("usage: mkdir [-p] directory [...]\n");
// or
errstr(argv[i]); 

If printf is required for ease of display of formatted output using format strings, then a tiny printf can be used (see elkscmd/lib/tiny_vprintf.c), by adding the following to the link line (see sys_utils/Makefile for example):

chmem: chmem.o $(TINYPRINTF)
    $(LD) $(LDFLAGS) -o chmem chmem.o $(TINYPRINTF) $(LDLIBS)

I would suggest using errmsg it at all possible to keep the executable size very small. In particular, it is nice to keep an executable size <= 1024 bytes, so that only one MINIX block is allocated for the application.

[floriangit]

In particular, it is nice to keep an executable size <= 1024 bytes

The binary exe is now 1040 bytes. When trimming it down to omit the SIGKILL handler (which is handled by the kernel as per POSIX and has no meaning in an application really, since it can never be caught), I can bring it down to 1024 bytes, yaaay.

Thus I'll rather update the "-h" in form of a comments update only in the .c file to play happy birthday and general guidance and remove the SIGKILL stuff.

Unless you object to?

[ghaerr]

The binary exe is now 1040 bytes. When trimming it down to omit the SIGKILL handler

Wow, that did get big in a hurry... the first version was quite a bit smaller. I'm kind of wondering what is taking up so much space?

omit the SIGKILL handler (which is handled by the kernel as per POSIX and has no meaning in an application really, since it can never be caught),

Agreed - remove it, as SIGKILL always causes the executable to cease termination, without a signal callback.

Thus I'll rather update the "-h" in form of a comments update only in the .c file to play happy birthday and general guidance and remove the SIGKILL stuff.

Go ahead. It might be worth taking a look at what's using up the space... this can be done by generating a map file by adding the following to the link line: -Wl,-Map=beep.map. beep_signal can probably have the switch removed and just call silent and exit.