Structure passed to function [SOLVED]

[benoitstjean]

Compiler: 5.026
Device: PIC24EP512GP806

Another trivial question which should be obvious but I am really not sure what I am doing wrong here.... Note that below is just an example to simplify the code. The interrupt works because it is used by other timers.

The working code is the following:

in .h file:

bool MyTimerActive = FALSE;
unsigned int MyTimerCount = 0;


To start the timer, I do this:

main( )
{
MyTimerActive = TRUE;
}

In timer interrupt function:

#INT_TIMER1
void TIMER1_isr( void )
{
if( MyTimerActive == TRUE )
{
MyTimerCount ++;

if( MyTimerCount == 100 )
{
MyTimerCount = 0;
}
}
}


Above code works just fine. Now, I have many timers and I'd like to clean-up the code a bit so I decided to create a structure which will contain the 'count' and 'IsActive' flags:


I have this structure in my .h file:

typedef struct
{
bool IsActive;
unsigned int Count;

} STIMERBASE;


STIMERBASE MyTimer;


Then I have these two functions:

void TIMER_Start( STIMERBASE TimerID )
{
TimerID.Count = 0;
TimerID.IsActive = TRUE;
fprintf( MONITOR_SERIAL, "\n\r--- Timer started ---" );
}

void TIMER_Stop( STIMERBASE TimerID )
{
TimerID.Count = 0;
TimerID.IsActive = FALSE;
fprintf( MONITOR_SERIAL, "\n\r--- Timer stopped ---" );
}


And when I want to start MyTimer, I *thought* I'd only need to do this:


main()
{
TIMER_Start( Mytimer );
}


And in my Timer interrupt, this is what I have:

#INT_TIMER1
void TIMER1_isr( void )
{
if( MyTimer.IsActive == TRUE )
{
MyTimer.Count +;

if( MyTimer.Count == 100 )
{
Timer_Stop( MyTimer );
}
}
}



PROBLEM: Timer interrupt works but timer never gets stopped therefore I don't even think it gets started.

What am I doing wrong with the structures?


Thanks,

Ben

[PCM programmer]

Your Timer_Start and Timer_Stop functions are writing to local copies
of the structure elements, but your ISR is looking at the global structure.

You can see this in the .SYM file:

[benoitstjean]

Argh!

So now this works below:

1) I added a '*' in the function;
2) I access the parameters via -> rather than . (dot);

void TIMER_Start( STIMERBASE *TimerID )
{
TimerID->Count = 0;
TimerID->IsActive = TRUE;
fprintf( MONITOR_SERIAL, "\n\r--- Timer started ---" );
}

Ben

[Ttelmah]

There are several errors in what you type, which suggests the problem may be caused by other things:

1) No enable for the interrupt shown.
2) only one '+' in the structure version increment line...
With this it'll never get stopped.
3) having the fprintf in the timer, with the code as shown, would have the interrupt disabled.
4) Code as shown will 'drop off the end', so chip will go to sleep.

As posted there is another problem, with alignment. The bool, will be having to use 16bits to then allow the counter to be correctly aligned.

[benoitstjean]

Hi PCM_Programmer,

I guess we posted at the same time.

Sorry, the stuff you posted doesn't mean much to me unfortunately.

However, is my assumption correct to use an -> rather than a dot?

Thanks,

Ben

[benoitstjean]

Hi TTelmah,

I posted at the same time as you as well!

The reason there are a few typos is mainly because I somewhat re-wrote in the post from scratch given that my code had other significant names...

1) As indicated, it is a simplified version of the code as you can see therefore I didn't put the enable_interrupt function (code has 40K+ lines so I put only what I figured was relevant);

2) Yes, there is a '++' not only '+';

3) All right for that part but now it works;

4) See no.1


As for the bool part, I use an int8. I had tried an int1 but the code crashed. With int8, it works.

But again, now that I modified the internal part of the function to use an arrow rather than a dot, it works.

Ben

[Ttelmah]

Yes. That makes you set the value in the original variable (if you pass a pointer to it), instead of changing the value in a local copy (which isn't going to do much!...).

It's worth keeping alignment 'in mind' in the future. For instance:

[benoitstjean]

Oh, I wasn't aware of that. Thanks, I will definitely look at how I have stuff outlined.

I guess it's the same idea if I have character strings?

So I would have:

[Ttelmah]

Strings/chars/int8 are all byte aligned.
Problem is that any larger variable has to be word aligned. So in the 'bad' example, the int8 starts on a word boundary. Then the int16 also has to be put on a word boundary, so a byte is skipped. Then a single byte value again leaves another byte potentially unused...
You can use the __packed__ attribute to store things tightly, but then a standard word access can't be used to fetch word based variables. Potential crash if you try to access a byte aligned word variable using a pointer for example.