From: Robby on
"Ulrich Eckhardt" wrote:

> Robby wrote:
> > #define COMPUTE_SYS_OSC(extCrys, fpllidiv, fpllmul, fpllodiv) \
> > (((float)(extCrys/fpllidiv)*(float)(fpllmul/fpllodiv))*1000000)
>
> Why? Why isn't this a function?

Hello Uli,

I don't know why I didn't make it a function. But why shouldn't it be a
macro?

>
> > extern float curr_sys_osc;
>
> This says: There is a global variable for anyone to access. However, zero
> documentation on how to use it...

So your saying I should add coments!

> > void set_curr_sys_osc(float extCrys, float fpllidiv,
> > float fpllmul, float fpllodiv);
> > int get_curr_sys_osc();
> > float get_osc_period();
>
> All these seem to have something to do with the above global variable.

Yes.

> > // In exactly one .c file.
> > static float curr_sys_osc=0;
>
> This says: Create a variable accessible _only_ in this file. This conflicts
> with the declaration in the header.

Thats the way we do globals... no?

- extern a var in a header file
- In exactly one .c file define or declare the var for that .c file
- and include the header file where the var is externed for every other .c
file that wishes to use the var !!! (I could be wrong! but its what I posted
and discussed about this a month or so)

> > void set_curr_sys_osc(float extCrys, float fpllidiv, float fpllmul, float
> > fpllodiv)
> > {curr_sys_osc = COMPUTE_SYS_OSC(extCrys, fpllidiv, fpllmul, fpllodiv);}
>
> The macro is used here, where else? I guess nowhere, so move it into this
> source file. Further, since even here it seems to only be used once, so you
> don't need it at all. Macro are a simple text substitution mechanism
> without any type checking. Adding all the casts you have in there, you get
> zero checking. This is a recipe for disaster and bad programming.

It might be used else where in the future. But what type of "type checking"
do you mean I should do in this function... Can you give me an example.
Thankyou!

> > int get_curr_sys_osc()
> > {return (int) curr_sys_osc;}
> >
> > float get_osc_period()
> > {return (1/curr_sys_osc);}
>
> Why not get_curr_sys_osc_period()? Why not get_curr_sys_osc_frequency()?
> Also, these in combination with set_curr_sys_osc() show that curr_sys_osc
> should not be global and visible everywhere.

I will need these for other modules. This sample is a far cry from the
project's actual size.

> Mylib.c/h are the same, too many global macros, too much type casting.

Yes, I thought so too. But right now, I have one priority... complete the
port. I did this as an extra to quickly organize things the best I could for
the frequency adaption for the delay function since in the new compiler
doesn't provide these.

Regards
Rob
From: David Wilkinson on
Robby wrote:
>>> // In exactly one .c file.
>>> static float curr_sys_osc=0;
>>
>> This says: Create a variable accessible _only_ in this file. This conflicts
>> with the declaration in the header.
>
> Thats the way we do globals... no?
>
> - extern a var in a header file
> - In exactly one .c file define or declare the var for that .c file
> - and include the header file where the var is externed for every other .c
> file that wishes to use the var !!! (I could be wrong! but its what I posted
> and discussed about this a month or so)

Yes, but not with the static keyword!

--
David Wilkinson
Visual C++ MVP
From: Igor Tandetnik on
Robby <Robby(a)discussions.microsoft.com> wrote:
> "Ulrich Eckhardt" wrote:
>
>> Robby wrote:
>>> #define COMPUTE_SYS_OSC(extCrys, fpllidiv, fpllmul, fpllodiv) \
>>> (((float)(extCrys/fpllidiv)*(float)(fpllmul/fpllodiv))*1000000)
>>
>> Why? Why isn't this a function?
>
> I don't know why I didn't make it a function. But why shouldn't it be
> a macro?

The right question to ask is - does it absolutely have to be a macro? If you can manage without, don't use macros. See also

http://www.gotw.ca/gotw/063.htm

--
With best wishes,
Igor Tandetnik

With sufficient thrust, pigs fly just fine. However, this is not necessarily a good idea. It is hard to be sure where they are going to land, and it could be dangerous sitting under them as they fly overhead. -- RFC 1925

From: Robby on
Hi David,

"David Wilkinson" wrote:

> Robby wrote:
> >>> // In exactly one .c file.
> >>> static float curr_sys_osc=0;
> >>
> >> This says: Create a variable accessible _only_ in this file. This conflicts
> >> with the declaration in the header.
> >
> > Thats the way we do globals... no?
> >
> > - extern a var in a header file
> > - In exactly one .c file define or declare the var for that .c file
> > - and include the header file where the var is externed for every other .c
> > file that wishes to use the var !!! (I could be wrong! but its what I posted
> > and discussed about this a month or so)
>
> Yes, but not with the static keyword!

So, in C, we *can't* have global static variables. I guess it would make
sence though. If a variable is global, why would it need to be static. Its
global! Its contents will never be lost!!!

So I take it as we can't do global static variables in C.

Regards
Rob
From: Robby on
"Igor Tandetnik" wrote:

> Robby <Robby(a)discussions.microsoft.com> wrote:
> > "Ulrich Eckhardt" wrote:
> >
> >> Robby wrote:
> >>> #define COMPUTE_SYS_OSC(extCrys, fpllidiv, fpllmul, fpllodiv) \
> >>> (((float)(extCrys/fpllidiv)*(float)(fpllmul/fpllodiv))*1000000)
> >>
> >> Why? Why isn't this a function?
> >
> > I don't know why I didn't make it a function. But why shouldn't it be
> > a macro?
>
> The right question to ask is - does it absolutely have to be a macro? If you can manage without, don't use macros.

I probably doesn't really have to be a macro.

I don't know about you guys, but the way I see the usefulness of macros
would be lets say we are doing a function, and all of the sudden we need to
do a calculation of some sort. Not to clutter up the code in the function, I
would use a macro... its clean out of the way and does one and only one
particular thing. Okay, now I have breifed the article and I see that macros
are a don't care text substitution operation in your code and so on.... but
so far I never had these problems. And what would we care if the compiler
changes our macro name from sleep() to sleepEX(). Isn't all this transparent
to the programmer?

Regards
Robert