Re: play.c

2005-11-29 Thread Hollis Blanchard

On Nov 29, 2005, at 4:05 PM, Vincent Pelletier wrote:

+  status = inb (SPEAKER);
+  outb (SPEAKER, status & ~(SPEAKER_TMR2 | SPEAKER_DATA));


You changed the style here...


+  status = inb (SPEAKER) | SPEAKER_TMR2 | SPEAKER_DATA;
+  outb (SPEAKER, status);


... but not here.


+  grub_dprintf ("play","tempo = %d\n", tempo);

...

+  grub_dprintf ("play","pitch = %d, duration = %d\n", buf.pitch,
+buf.duration);


Space after the first comma, please.


+  to = grub_get_rtc () + 120 * buf.duration / tempo;


What is 120? Should be a #define, and probably commented.


+  file = grub_file_open (args[0]);
+  if (! file)
+return grub_error (GRUB_ERR_FILE_NOT_FOUND, "file not found");
+
+  if (grub_file_read (file, (void *) &tempo, sizeof(tempo)) != 
sizeof(tempo))

+return grub_error (GRUB_ERR_FILE_READ_ERROR,
+   "file doesn't even contains a full tempo 
record");


You must grub_file_close(file) before returning here.

Other than these minor things, looks good to me.

-Hollis



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-29 Thread Vincent Pelletier
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

New version of my patch.
I think I followed all the given advices.

Vincent Pelletier
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.2 (GNU/Linux)

iD8DBQFDjNCnibN+MXHkAogRAtK8AJ4rInXMoWqirdCWUwkrchz1hhAWXACfetNg
t9XlC2XS4zLj08Tf6A2HHzyIPwMFAUOM0KcURCgpFDKO1REC0rwAnifHHB66WIvU
wL5TGVG7yAyONtADAKC02lDTu8C178IVqV2ejfWdsAi4WQ==
=4UJg
-END PGP SIGNATURE-
Index: commands/i386/pc/play.c
===
RCS file: commands/i386/pc/play.c
diff -N commands/i386/pc/play.c
--- /dev/null	1 Jan 1970 00:00:00 -
+++ commands/i386/pc/play.c	29 Nov 2005 22:03:58 -
@@ -0,0 +1,228 @@
+/* play.c - command to play a tune  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2005  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* Lots of this file is borrowed from GNU/Hurd generic-speaker driver.  */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Read a byte from a port.  */
+static inline unsigned char
+inb (unsigned short port)
+{
+  unsigned char value;
+  asm volatile ("inb%w1, %0" : "=a" (value) : "Nd" (port));
+  return value;
+}
+
+/* Write a byte to a port.  */
+static inline void
+outb (unsigned short port, unsigned char value)
+{
+  asm volatile ("outb   %b0, %w1" : : "a" (value), "Nd" (port));
+}
+
+/* The speaker port.  */
+#define SPEAKER			0x61
+
+/* If 0, follow state of SPEAKER_DATA bit, otherwise enable output
+   from timer 2.  */
+#define SPEAKER_TMR2		0x01
+
+/* If SPEAKER_TMR2 is not set, this provides direct input into the
+   speaker.  Otherwise, this enables or disables the output from the
+   timer.  */
+#define SPEAKER_DATA		0x02
+
+/* The PIT channel value ports.  You can write to and read from them.
+   Do not mess with timer 0 or 1.  */
+#define PIT_COUNTER_0		0x40
+#define PIT_COUNTER_1		0x41
+#define PIT_COUNTER_2		0x42
+
+/* The frequency of the PIT clock.  */
+#define PIT_FREQUENCY		0x1234dd
+
+/* The PIT control port.  You can only write to it.  Do not mess with
+   timer 0 or 1.  */
+#define PIT_CTRL		0x43
+#define PIT_CTRL_SELECT_MASK	0xc0
+#define PIT_CTRL_SELECT_0	0x00
+#define PIT_CTRL_SELECT_1	0x40
+#define PIT_CTRL_SELECT_2	0x80
+
+/* Read and load control.  */
+#define PIT_CTRL_READLOAD_MASK	0x30
+#define PIT_CTRL_COUNTER_LATCH	0x00	/* Hold timer value until read.  */
+#define PIT_CTRL_READLOAD_LSB	0x10	/* Read/load the LSB.  */
+#define PIT_CTRL_READLOAD_MSB	0x20	/* Read/load the MSB.  */
+#define PIT_CTRL_READLOAD_WORD	0x30	/* Read/load the LSB then the MSB.  */
+
+/* Mode control.  */
+#define PIT_CTRL_MODE_MASK	0x0e
+
+/* Interrupt on terminal count.  Setting the mode sets output to low.
+   When counter is set and terminated, output is set to high.  */
+#define PIT_CTRL_INTR_ON_TERM	0x00
+
+/* Programmable one-shot.  When loading counter, output is set to
+   high.  When counter terminated, output is set to low.  Can be
+   triggered again from that point on by setting the gate pin to
+   high.  */
+#define PIT_CTRL_PROGR_ONE_SHOT	0x02
+
+/* Rate generator.  Output is low for one period of the counter, and
+   high for the other.  */
+#define PIT_CTRL_RATE_GEN	0x04
+
+/* Square wave generator.  Output is low for one half of the period,
+   and high for the other half.  */
+#define PIT_CTRL_SQUAREWAVE_GEN	0x06
+
+/* Software triggered strobe.  Setting the mode sets output to high.
+   When counter is set and terminated, output is set to low.  */
+#define PIT_CTRL_SOFTSTROBE	0x08
+
+/* Hardware triggered strobe.  Like software triggered strobe, but
+   only starts the counter when the gate pin is set to high.  */
+#define PIT_CTRL_HARDSTROBE	0x0a
+
+/* Count mode.  */
+#define PIT_CTRL_COUNT_MASK	0x01
+#define PIT_CTRL_COUNT_BINARY	0x00	/* 16-bit binary counter.  */
+#define PIT_CTRL_COUNT_BCD	0x01	/* 4-decade BCD counter.  */
+
+#define T_REST			((short) 0)
+#define T_FINE			((short) -1)
+
+struct note
+{
+  short pitch;
+  short duration;
+};
+
+static void
+beep_off (void)
+{
+  unsigned char status;
+
+  status = inb (SPEAKER);
+  outb (SPEAKER, status & ~(SPEAKER_TMR2 | SPEAKER_DATA));
+}
+
+static void
+beep_on (short pitch)
+{
+  unsigned char status;
+  unsigned int counter;
+
+  if (pitch < 20)
+pitch = 20;
+  else

Re: play.c

2005-11-06 Thread Dennis Clarke
On 11/6/05, Marco Gerards <[EMAIL PROTECTED]> wrote:
> Hollis Blanchard <[EMAIL PROTECTED]> writes:
> > On Nov 5, 2005, at 2:48 PM, Dennis Clarke wrote:
> >> On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:
> >>> On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:
>  Here is the play command, along with some songs.
> >>>
> >>> I point out some stylish mistakes.
> >>
> >> Personally I was thrilled with the idea of sound and really, these

Okay okay okay .. I am the "new kid" and clearly not up to speed on
anybody or anything.

[ please picture the John Cleese scene from A Fish Called Wanda in which
  he is hanging upside down out a window ]

I apologize completely and without reservation andmy comments were
from way out in left field and .. man .. I just didn't know what I was
thinking ..

On that note I will ensure that I run anything I do through cb before
hand with the K&R C options for indents and braces etc etc.

Then I will look again.

Dennis
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-06 Thread Marco Gerards
Hollis Blanchard <[EMAIL PROTECTED]> writes:

> On Nov 5, 2005, at 2:48 PM, Dennis Clarke wrote:
>
>> On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:
>>> On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:
 Here is the play command, along with some songs.
>>>
>>> I point out some stylish mistakes.
>>
>> Personally I was thrilled with the idea of sound and really, these
>> little "style" things are so trivial.  Its like someone hanging a
>> painting and then commenting on color of the frame.
>>
>> Actually I am sure it is just a language barrier issue and what you
>> meant to say was "wow, thanks for this cool submission!"
>
> Please don't confuse constructive criticism for disinterest or a lack
> of appreciation. The way code approaches perfection is through peer
> review, and you will notice that almost all patches from all
> contributors are commented on. In fact, most people actively seek out
> criticism for their patches! That is why Vincent posted it to the list
> for comment rather than just committing it himself.

The Hurd just works the other way around sometimes.  People send in
simple patches and they are ignored very often.  There is nothing more
frustrating than that.  It might seem a bit harsh to get some critique
on a patch, but it is the best in the long term for everyone.

Which reminds me of all the patches for GRUB I still have to
review... ;-/

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-06 Thread Marco Gerards
Dennis Clarke <[EMAIL PROTECTED]> writes:

> On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:
>> On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:
>> > Here is the play command, along with some songs.
>>
>> I point out some stylish mistakes.
>
> Personally I was thrilled with the idea of sound and really, these
> little "style" things are so trivial.  Its like someone hanging a
> painting and then commenting on color of the frame.

Style is not trivial at all.  I keep realizing that when I read code
that others wrote and without a consistent coding style.

Besides that, Vincent already knew we value his contribution, at least
he should. :-)

--
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-06 Thread Marco Gerards
Vincent Pelletier <[EMAIL PROTECTED]> writes:

Hi Vincent,

> Here is the play command, along with some songs.

Cool, thanks for this neat patch. :-)

> Vincent Pelletier
> Index: commands/i386/pc/play.c
> ===
> RCS file: commands/i386/pc/play.c
> diff -N commands/i386/pc/play.c
> --- /dev/null 1 Jan 1970 00:00:00 -
> +++ commands/i386/pc/play.c   5 Nov 2005 14:06:00 -
> @@ -0,0 +1,248 @@
> +/* play.c - command to play a tune  */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2003  Free Software Foundation, Inc.

Shouldn't it be (C) 2003, 2005 or so?

> +static grub_err_t
> +grub_cmd_play (struct grub_arg_list *state __attribute__ ((unused)),
> +   int argc, char **args)
> +{
> +  grub_file_t file;
> +  struct note buf;
> +  int tempo;
> +  unsigned int to;
> +
> +  if (argc != 1)
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, "file name required");
> +
> +  file = grub_file_open (args[0]);
> +  if (! file)
> +return 0;
> +
> +  if (grub_file_read (file, (void *) &tempo, sizeof(tempo)) != sizeof(tempo))
> +return 0;

You have to close the file here.


> +#ifdef GRUB_UTIL
> +void
> +grub_play_init (void)
> +{
> +  grub_register_command ("play", grub_cmd_play, GRUB_COMMAND_FLAG_BOTH,
> +  "play FILE", "Play a tune", 0);
> +}
> +
> +void
> +grub_play_fini (void)
> +{
> +  grub_unregister_command ("play");
> +}
> +#else /* ! GRUB_UTIL */

This code is quite low level and not needed in GRUB_UTIL.  So you can
just leave this away.  Perhaps the same needs to be done for some
commands that are already in CVS, I'll look at that.

Thanks,
Marco



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Vincent Pelletier
On Sat, 5 Nov 2005 23:23:25 +0100
Samuel Thibault <[EMAIL PROTECTED]> wrote:
> Hollis Blanchard, le Sat 05 Nov 2005 16:17:33 -0600, a écrit :

BTW, thank for your comments, I'll send another version tomorrow.

> > Can you describe how the PIT is related to the speaker?
> That's the way one can make the speaker beep without having to keep
> cpu busy with typically-440-Hz outs. One just asks the PIT to do it
> for us in the background.

To beep, a square generator is set to the right frequency, and then a
bit allows the pulses to go to the actual buzzer.

> > I'm wondering if this code could mostly work on PPC platforms that
> > have PC-like peripherals.
> It depends on whether the PIT is linked to the speaker like on PC.

I think there is really little chance it works (ie. without
writing a sound-card driver).

Vincent Pelletier

PS: trying sylpheed claws gtk2





___ 
Appel audio GRATUIT partout dans le monde avec le nouveau Yahoo! Messenger 
T�l�chargez cette version sur http://fr.messenger.yahoo.com



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Samuel Thibault
Hollis Blanchard, le Sat 05 Nov 2005 16:17:33 -0600, a écrit :
> On Nov 5, 2005, at 8:17 AM, Vincent Pelletier wrote:
> >Here is the play command, along with some songs.
> 
> Can you describe how the PIT is related to the speaker?

That's the way one can make the speaker beep without having to keep cpu
busy with typically-440-Hz outs. One just asks the PIT to do it for us
in the background.

> I'm wondering if this code could mostly work on PPC platforms that
> have PC-like peripherals.

It depends on whether the PIT is linked to the speaker like on PC.

Regards,
Samuel


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Hollis Blanchard

On Nov 5, 2005, at 8:17 AM, Vincent Pelletier wrote:


Here is the play command, along with some songs.


Can you describe how the PIT is related to the speaker? I'm wondering 
if this code could mostly work on PPC platforms that have PC-like 
peripherals.



--- /dev/null   1 Jan 1970 00:00:00 -
+++ commands/i386/pc/play.c 5 Nov 2005 14:06:00 -
@@ -0,0 +1,248 @@

...
+/* Lots of this file is borowed from GNU/Hurd generic-speaker driver 
*/


"borrowed"


+/* Read a byte from a port.  */
+static inline unsigned char
+inb (unsigned short port)
+{
+  unsigned char value;
+
+  asm volatile ("inb%w1, %0" : "=a" (value) : "Nd" (port));
+  asm volatile ("outb   %%al, $0x80" : : );
+
+  return value;
+}


What is the purpose of outb to 0x80?

Please combine these into a single asm statement. The compiler can and 
will put code in between these statements...



+/* Write a byte to a port.  */
+static inline void
+outb (unsigned short port, unsigned char value)
+{
+  asm volatile ("outb   %b0, %w1" : : "a" (value), "Nd" (port));
+  asm volatile ("outb   %%al, $0x80" : : );
+}


Again, combine the asm statements please.

If this code could be usable on other architectures, we will need to 
move inb/outb into architecture-specific code. That's probably not a 
bad idea anyways...



+static void
+beep_off (void)
+{
+  unsigned char status;
+
+  status = inb (SPEAKER) & ~(SPEAKER_TMR2 | SPEAKER_DATA);
+  outb (SPEAKER, status);
+}


A minor issue, but this code makes more sense to me:
status = inb (SPEAKER);
outb (SPEAKER, status & ~(SPEAKER_TMR2 | SPEAKER_DATA));


+static grub_err_t
+grub_cmd_play (struct grub_arg_list *state __attribute__ ((unused)),
+ int argc, char **args)
+{
+  grub_file_t file;
+  struct note buf;
+  int tempo;
+  unsigned int to;
+
+  if (argc != 1)
+return grub_error (GRUB_ERR_BAD_ARGUMENT, "file name required");
+
+  file = grub_file_open (args[0]);
+  if (! file)
+return 0;
+
+  if (grub_file_read (file, (void *) &tempo, sizeof(tempo)) != 
sizeof(tempo))

+return 0;


Shouldn't these error conditions return non-0 error codes?


+  grub_printf ("Now playing %s...\n", args[0]);


Probably not needed.


+  grub_dprintf ("play","tempo = %d\n", tempo);
+
+  while (grub_file_read (file, (void *) &buf, sizeof(struct note)) == 
sizeof(struct note) &&

+ buf.pitch != T_FINE &&
+ grub_checkkey () == -1)
+{
+
+  grub_dprintf ("play","pitch = %d, duration = %d\n", buf.pitch, 
buf.duration);

+
+  switch (buf.pitch)
+{
+  case T_REST:
+beep_off ();
+break;
+
+  default:
+beep_on (buf.pitch);
+break;
+}
+
+  to = grub_get_rtc () + 120*buf.duration/tempo;
+  while ((unsigned int) grub_get_rtc () <= to && grub_checkkey () 
== -1)


I would add some parentheses. Also, I would prefer "grub_checkkey () < 
0", because on i386 that is a BIOS call, and who knows if all BIOS 
return exactly -1...



+   ;
+}
+
+  beep_off ();
+
+  grub_printf ("Done\n");


Again, a little too verbose.


+  grub_file_close (file);
+
+  while (grub_checkkey () != -1)


Same comment as above.


+grub_getkey ();
+
+  return 0;
+}


Also, did you have some tool for creating those song files?

I would expect it's not entirely legal to ship versions of copyrighted 
songs.


-Hollis



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Hollis Blanchard

On Nov 5, 2005, at 3:25 PM, Dennis Clarke wrote:


I was sitting on some changes and was thinking about how to submit
them and then thought, oh great, I have no idea what style is needed
or wanted.  So then after seeing this, an inside joke or something
like that, I then felt "why bother".


Formatting code acceptably for any new project is indeed a barrier to 
entry. You can see my frustration with the ChangeLog in particular in 
the mailing list archives. :) I still think ChangeLog is stupid, but it 
doesn't get in my way much now, so I live with it.


The Linux kernel has other (non-code-related) quirks, such as the 
"Signed-off-by" line, and figuring out the appropriate person to send a 
particular patch to. All these things take some adjusting for new 
contributors.


Btw, if you haven't seen it already, http://grub.enbug.org/CodingStyle 
offers some formatting guidance.



Hollis was probably dead on the mark.  Open source works just fine and
constructive criticism is a good thing.  I guess thats the whole
point.  On that note I'm going to pull the latest CVS and then have
another look at a few things.


Glad to hear it! :) By the way, have you arranged to assign your 
copyright to the FSF? This is a legal barrier to entry...


-Hollis



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Dennis Clarke
On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:
> On Saturday 05 November 2005 10:12 pm, Hollis Blanchard wrote:
> > On Nov 5, 2005, at 2:48 PM, Dennis Clarke wrote:
> > > On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:
> > >> On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:
> > >>> Here is the play command, along with some songs.
> > >>
> > >> I point out some stylish mistakes.
> > >
> Dennis wanted to say such a thing... If my attitude looks silly, Subdino has
> no problem with saying it to me directly without his help.
>

I was sitting on some changes and was thinking about how to submit
them and then thought, oh great, I have no idea what style is needed
or wanted.  So then after seeing this, an inside joke or something
like that, I then felt "why bother".

Hollis was probably dead on the mark.  Open source works just fine and
constructive criticism is a good thing.  I guess thats the whole
point.  On that note I'm going to pull the latest CVS and then have
another look at a few things.

dc
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Yoshinori K. Okuji
On Saturday 05 November 2005 10:12 pm, Hollis Blanchard wrote:
> On Nov 5, 2005, at 2:48 PM, Dennis Clarke wrote:
> > On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:
> >> On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:
> >>> Here is the play command, along with some songs.
> >>
> >> I point out some stylish mistakes.
> >
> > Personally I was thrilled with the idea of sound and really, these
> > little "style" things are so trivial.  Its like someone hanging a
> > painting and then commenting on color of the frame.
> >
> > Actually I am sure it is just a language barrier issue and what you
> > meant to say was "wow, thanks for this cool submission!"
>
> Please don't confuse constructive criticism for disinterest or a lack
> of appreciation. The way code approaches perfection is through peer
> review, and you will notice that almost all patches from all
> contributors are commented on. In fact, most people actively seek out
> criticism for their patches! That is why Vincent posted it to the list
> for comment rather than just committing it himself.
>
> This is how open source works, and I'm sure you'd agree it seems to be
> working pretty well... ;)

It is not much related to Free Software (don't use Open Source in this list, 
please). Dennis is confused, because he simply does not know the background 
or our relationship (between me and Subdino). But I don't understand why 
Dennis wanted to say such a thing... If my attitude looks silly, Subdino has 
no problem with saying it to me directly without his help.

Okuji


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Hollis Blanchard

On Nov 5, 2005, at 2:48 PM, Dennis Clarke wrote:


On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:

On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:

Here is the play command, along with some songs.


I point out some stylish mistakes.


Personally I was thrilled with the idea of sound and really, these
little "style" things are so trivial.  Its like someone hanging a
painting and then commenting on color of the frame.

Actually I am sure it is just a language barrier issue and what you
meant to say was "wow, thanks for this cool submission!"


Please don't confuse constructive criticism for disinterest or a lack 
of appreciation. The way code approaches perfection is through peer 
review, and you will notice that almost all patches from all 
contributors are commented on. In fact, most people actively seek out 
criticism for their patches! That is why Vincent posted it to the list 
for comment rather than just committing it himself.


This is how open source works, and I'm sure you'd agree it seems to be 
working pretty well... ;)


-Hollis



___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Dennis Clarke
On 11/5/05, Yoshinori K. Okuji <[EMAIL PROTECTED]> wrote:
> On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:
> > Here is the play command, along with some songs.
>
> I point out some stylish mistakes.

Personally I was thrilled with the idea of sound and really, these
little "style" things are so trivial.  Its like someone hanging a
painting and then commenting on color of the frame.

Actually I am sure it is just a language barrier issue and what you
meant to say was "wow, thanks for this cool submission!"

Dennis
___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel


Re: play.c

2005-11-05 Thread Vincent Pelletier
Yoshinori K. Okuji wrote:
> I point out some stylish mistakes.

For once that I have the Changelog entry right :) .

> You must put the starting brace character on next line:

Please don't waste your time giving an example when it's that trivial,
or please do it in the diff file directly...

> The lines are folded incorrectly.

I also corrected the lines length where needed.

Vincent Pelletier
Index: commands/i386/pc/play.c
===
RCS file: commands/i386/pc/play.c
diff -N commands/i386/pc/play.c
--- /dev/null   1 Jan 1970 00:00:00 -
+++ commands/i386/pc/play.c 5 Nov 2005 14:06:00 -
@@ -0,0 +1,251 @@
+/* play.c - command to play a tune  */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2005  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+/* Lots of this file is borowed from GNU/Hurd generic-speaker driver */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Read a byte from a port.  */
+static inline unsigned char
+inb (unsigned short port)
+{
+  unsigned char value;
+
+  asm volatile ("inb%w1, %0" : "=a" (value) : "Nd" (port));
+  asm volatile ("outb   %%al, $0x80" : : );
+  
+  return value;
+}
+
+/* Write a byte to a port.  */
+static inline void
+outb (unsigned short port, unsigned char value)
+{
+  asm volatile ("outb   %b0, %w1" : : "a" (value), "Nd" (port));
+  asm volatile ("outb   %%al, $0x80" : : );
+}
+
+/* The speaker port.  */
+#define SPEAKER0x61
+
+/* If 0, follow state of SPEAKER_DATA bit, otherwise enable output
+   from timer 2.  */
+#define SPEAKER_TMR2   0x01
+
+/* If SPEAKER_TMR2 is not set, this provides direct input into the
+   speaker.  Otherwise, this enables or disables the output from the
+   timer.  */
+#define SPEAKER_DATA   0x02
+
+/* The PIT channel value ports.  You can write to and read from them.
+   Do not mess with timer 0 or 1.  */
+#define PIT_COUNTER_0  0x40
+#define PIT_COUNTER_1  0x41
+#define PIT_COUNTER_2  0x42
+
+/* The frequency of the PIT clock.  */
+#define PIT_FREQUENCY  0x1234dd
+
+/* The PIT control port.  You can only write to it.  Do not mess with
+   timer 0 or 1.  */
+#define PIT_CTRL   0x43
+#define PIT_CTRL_SELECT_MASK   0xc0
+#define PIT_CTRL_SELECT_0  0x00
+#define PIT_CTRL_SELECT_1  0x40
+#define PIT_CTRL_SELECT_2  0x80
+
+/* Read and load control.  */
+#define PIT_CTRL_READLOAD_MASK 0x30
+#define PIT_CTRL_COUNTER_LATCH 0x00/* Hold timer value until read.  */
+#define PIT_CTRL_READLOAD_LSB  0x10/* Read/load the LSB.  */
+#define PIT_CTRL_READLOAD_MSB  0x20/* Read/load the MSB.  */
+#define PIT_CTRL_READLOAD_WORD 0x30/* Read/load the LSB then the MSB.  */
+
+/* Mode control.  */
+#define PIT_CTRL_MODE_MASK 0x0e
+
+/* Interrupt on terminal count.  Setting the mode sets output to low.
+   When counter is set and terminated, output is set to high.  */
+#define PIT_CTRL_INTR_ON_TERM  0x00
+
+/* Programmable one-shot.  When loading counter, output is set to
+   high.  When counter terminated, output is set to low.  Can be
+   triggered again from that point on by setting the gate pin to
+   high.  */
+#define PIT_CTRL_PROGR_ONE_SHOT0x02
+
+/* Rate generator.  Output is low for one period of the counter, and
+   high for the other.  */
+#define PIT_CTRL_RATE_GEN  0x04
+
+/* Square wave generator.  Output is low for one half of the period,
+   and high for the other half.  */
+#define PIT_CTRL_SQUAREWAVE_GEN0x06
+
+/* Software triggered strobe.  Setting the mode sets output to high.
+   When counter is set and terminated, output is set to low.  */
+#define PIT_CTRL_SOFTSTROBE0x08
+
+/* Hardware triggered strobe.  Like software triggered strobe, but
+   only starts the counter when the gate pin is set to high.  */
+#define PIT_CTRL_HARDSTROBE0x0a
+
+/* Count mode.  */
+#define PIT_CTRL_COUNT_MASK0x01
+#define PIT_CTRL_COUNT_BINARY  0x00/* 16-bit binary counter.  */
+#define PIT_CTRL_COUNT_BCD 0x01/* 4-decade BCD counter.  */
+
+#define T_REST ((short) 0)
+#define T_FINE ((short) -1)
+
+struct note
+{
+  short pitch;
+  short duration;
+};
+
+static void
+b

Re: play.c

2005-11-05 Thread Yoshinori K. Okuji
On Saturday 05 November 2005 03:17 pm, Vincent Pelletier wrote:
> Here is the play command, along with some songs.

I point out some stylish mistakes.

> +struct note {
> +  short pitch;
> +  short duration;
> +};

You must put the starting brace character on next line:

struct note
{
  short pitch;
  short duration;
};

> +  while (grub_file_read (file, (void *) &buf, sizeof(struct note)) == 
sizeof(struct note) &&
> +         buf.pitch != T_FINE &&
> +         grub_checkkey () == -1)

The lines are folded incorrectly. You must insert new lines before logical 
operations. Also, please add a space character after sizeof:

while (grub_file_read (file, (void *) &buf, sizeof (struct note)) == sizeof 
(struct note)
  && buf.pitch != T_FINE
  && grub_checkkey () == -1)

> +      to = grub_get_rtc () + 120*buf.duration/tempo;

Insert space characters.

Okuji


___
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel