Skip Nav
Home » Forums » SystemC Forum

Icon - KMLM List KMLM List

View email archives for the history of this mailing list.

List Home All Archives Dates Threads Authors Subjects
systemc-forum - Re: [systemc-forum] Errors in the attached files Message Thread: Previous | Next
  • To: yehuda.singer@xxxxxxxxx
  • From: "Philipp A. Hartmann" <philipp.hartmann@xxxxxxxx>
  • Date: Sun, 04 Mar 2012 14:03:24 +0100
  • Cc: systemc-forum@xxxxxxxxxxxxxxxxxxx
Send Email to systemc-forum@lists.accellera.org:
Send new message
Reply to this message
Yehuda,

On 04/03/12 09:20, Dr. Yehuda Singer wrote:
> 
> Who can help me with this simple code?

This depends on what problem you actually have.
First of all, you should fix the compiler errors, I guess.

I did a quick review anyhow.  Maybe it helps.

Greetings from Oldenburg,
  Philipp

---- my_func.h:

Include guards missing, required include files missing.

> #define MAX_THREADS 2

It's C++, don't use defines for numeric constants.

> struct my_func : sc_module { 

Why do you use SC_CTOR, but not the SC_MODULE macro?
I'd recommend not to mix styles arbitrarily.

>               sc_out<DWORD>           dwTime_sig      ;       // output data

What's a DWORD?  Are you sure, you want to use Windows-specific types
for your system model?

>               sc_in_clk                       CLK                     ;

sc_in_clk is deprecated.  Use a plain sc_in<bool> instead.

Secondly, I would re-order the ports to have the input ports first.
Since you don't give explicit names to the ports, this helps debugging a
bit (if, e.g. port_0 is always the clock port).

> 
>               int my_func_id ; 
> void my_func_id_init( int id ) 
>    {
>          my_func_id = id ; 
>    }
>   //Constructor 
> 
>    SC_CTOR(my_func) {

my_func_id is not initialised in the constructor.  This is a Bad Thing.

>          SC_CTHREAD(entry_my_func, CLK.pos())};

Semicolon missing after "SC_CTHREAD" call.  Superfluous (and strictly
speaking invalid) semicolon after the definition of the constructor.

>   // Process functionality in member function below
>   void entry_my_func();
>   
> };

---- my_func.cpp:

Now, that file is quite weird.
I really don't know what you try to achieve.

> #include <cstdlib>
> #include <limits.h>
> #include <time.h>
> #include "systemc.h"
> #include "my_func.h"

It's a good idea to have implementation-specific includes _after_ the
my_func.h header.  But this qould require, that my_func.h already
includes the headers it depends on.

In bigger SystemC models (or C++ programs in general) it gets really
messy and tedious to maintain otherwise.

> #define WAIT_FUNC 1000        // 1000 ms

Again, don't use defines for integral constants.  And maybe use more
descriptive names.  WAIT_FUNC is clearly not a function?

> int random_wait_func () ; 
> void busy_wait(DWORD milli_seconds) ; 

These functions are locally used only.  You should consider to define
them as 'static'.  Why do you need the separate declaration?

> int random_wait_func()
> {
>       static int m = 1;
>       srand(GetTickCount()*(m++));

Why do you call srand() over and over again?

Use something like:
  static bool srand_called=false;
  if( !srand_called ) {
    srand( /*whatever */ );
    srand_called = true;
  }

I'm quite sure that this would give better randomized results.

>       return 10 * (rand() % 1000);
> }
> 
> void busy_wait(DWORD milli_seconds)
> {
>       DWORD dwTicks = GetTickCount();
>       while (GetTickCount() - dwTicks < milli_seconds);

I know it's a Windows function and the uptime of such systems is not
that high, but you do not guard against wraparounds of GetTickCount
here.  But It's not clear, why you use these functions at all.

> }
> 
> void entry_my_func ()

Is this supposed to be the definition of the member function in my_func?
 Then you need to fully qualify the function name when defining the
function outside of the class:

void my_func::entry_my_func()

> {
>       DWORD dwTime_start ;
>       DWORD dwTime_end ;

This variable is unused.

>       
>       int j;
>       
>       dwTime_start = GetTickCount();

Why don't you initialise the variables directly?

>       for (j = 0; j < MAX_THREADS; j++) 

Why do you need the loop+if here?

>       {
>               if (j ==my_func_id)

It will always match exactly one entry.  Just drop both.

>               {
> 
>                       busy_wait(WAIT_FUNC);
>                       dwTime_sig.write ( GetTickCount()-dwTime_start )  ;

And the body is even independent of my_func_id (and j).

>               }
>       }
>       return ;

Here your process ends.  And you have never called a single SystemC
wait(), so all of your processes will only run once (on the first clock
edge seen at clk.pos()).  What's the point?

> }

---- main.cpp:

> #include <cstdlib>
> #include <limits.h>
> #include <time.h>
> #include "systemc.h"
> #include "my_func.h"
> 
> sc_signal<DWORD> sig_dwTime[MAX_THREADS]; 

Why don't you instantiate the signals in the body of sc_main?

MAX_THREADS is defined in my_func.  And it is not really used there
(except for the superfluous loop).  Move it to main.cpp, since here it's
used as parameter for the number of modules.  Oh, and make it a proper
constant.

> int sc_main(int argc, char **argv)
> {     int i ; 
>       my_func *my_func_ptr_array [MAX_THREADS] ;

I would suggest to use an sc_vector (new in SystemC 2.3) here:

   sc_vector< my_func > my_func_array( "my_func", MAX_THREADS );

Preferably also for the signal arrays above:

   sc_vector<sc_signal<DWORD> > sig_dwTime ("sig_dwTime", MAX_THREADS);

>       char temp_name[100];
>       sc_clock clk("Clock", 1, SC_NS, 0.5, 0.0, SC_NS);
> 
>       for ( i=0; i<MAX_THREADS; i++)
>       {
>               sprintf(temp_name, "my_func_%d", i);
>               my_func_ptr_array[i] = new my_func(temp_name); 
>               my_func_ptr_array[i]->my_func_id_init( i ); 
>               my_func_ptr_array[i]->dwTime_sig(sig_dwTime[i]) ;
>               my_func_ptr_array[i]->CLK(clk) ;
>       }

When using sc_vectors, you can bind the dwtime port of the whole array
with a single statement:

  sc_assemble_vector( my_func_array, &my_func::dwTime_sig )
    .bind( sig_dwTime );

You'd still need the loop to bind the clock (and initialise the id,
although the latter may not be needed):

  for ( i=0; i<MAX_THREADS; i++)
  {
    my_func_array[i].CLK(clk);
    my_func_array[i]->my_func_id_init( i );
  }

> return 1 ;

This (usually) returns an error to the environment.  You should prefer
to return EXIT_SUCCESS, if your simulation has finished successfully.

You don't start the simulation, so your model won't do much.
Add an sc_start call (before the return, of course):

  sc_start( 100, SC_NS );  // or whatever time you want to simulate
        
> }

-- 
Philipp A. Hartmann
Hardware/Software Design Methodology Group

OFFIS Institute for Information Technology
R&D Division Transportation · FuE-Bereich Verkehr
Escherweg 2 · 26121 Oldenburg · Germany · http://www.offis.de/
Phone/Fax: +49-441-9722-420/282 · PGP: 0x9161A5C0 · Skype: phi.har


By Date: Previous | Next Current Thread By Thread: Previous | Next