Math computation giving wrong answer Studio 7 AVR

Go To Last Post
12 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Help on this please.

seconds computation from Hours mins and secs

tseconds = tenths.

 

My math on computing seconds from 23 Hours 55 mins and 18 seconds is 86118 seconds not 20582.

 

I've split the code into managable lines but still get the same answer...

 

 

 


	
unsigned long event_as_tseconds=0;
unsigned long event_as_seconds=0;
	
	
		eventhour = evarray[0] & 0x1f;
		eventmin = evarray[1];
		eventsecond = evarray[2];
		eventtenths = evarray[9];
		
		
	    event_as_seconds += (unsigned long)(eventhour*3600);
		event_as_seconds += (unsigned long)(eventmin*60);
		event_as_seconds +=  (unsigned long) eventsecond;
		
		event_as_tseconds =  (event_as_seconds * 10 ) + eventtenths;
		if (currenttime_as_tseconds < event_as_tseconds ) // get first 'next' alarm
		{
		    
		    
		    
		}
		
		// Following is from Watch window 
		event_as_tseconds	205825	unsigned long{data}@0x07f8 ([R28]+6)
		event_as_seconds	20582	unsigned long{data}@0x07f4 ([R28]+2)
		eventhour	23	unsigned char{data}@0x05bb
		eventmin	55	unsigned char{data}@0x05e6
		eventsecond	18	unsigned char{data}@0x05b0
		eventtenths	5	unsigned char{data}@0x05b2

		
		
		
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Change your constants to long, i.e.  3600  to  3600UL     as 23 x 3600 > 32k or 64k

Your first calculation is overflowing the implied int max value, even though you cast the result to UL, the calcs default to int unless you express the constants as UL's!

 

Jim

I have learned this lesson so many times my head hurts!!!

 

 

Click Link: Get Free Stock: Retire early! PM for strategy

share.robinhood.com/jamesc3274

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
(unsigned long)(eventhour*3600);

looks a bit suspicious. The cast is only being applied AFTER the calculation is done. Surely you want:

((unsigned long)eventhour * 3600);

so the input is cast up to uint32_t (a better choice than "unsigned long" by the way) BEFORE the calculation is performed. In fact perhaps a possibly cleaner syntax is:

eventhour * 3600UL;

The UL will cast the 3600 up to uint32_t before things start so the whole calculation is performed at that width.

Last Edited: Tue. Jun 19, 2018 - 03:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Same thing for the minutes calculation.  eventmin is a 8-bit variable holding values ranging from 0..59.  The maximum minutes value 59*60  overflows this 8-bit value.  So cast the variable to a larger type first, then multiply against the constant.

 

event_as_seconds += (unsigned long)eventmin * 60;

 

Or multiply against an unsigned long constant to get an implied casting

event_as_seconds += eventmin * 60UL;

 

 

Last Edited: Tue. Jun 19, 2018 - 05:15 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for those hints.

This is what I find strange...

I used a simple uncast line of code in one part of the program and it works in one part of the program .... it fails in another (the part of code I posted earlier)

 

I've now made a function to do the job and it fails also but I tried implementing all the suggestions so what do you think...

 

Here's the code that works,  'currenttime_as_tseconds' is correct from the simple line of code, but the dummy variable from the function fails..


	currenttime_as_tseconds = tenths + ( seconds *10 ) + (mins*600) + (hours*36000); // read time now		
    currenttime_as_tsecondsdummy = convertHMSt_totSecs(hours, mins,seconds,tenths);


// from watch

		currenttime_as_tseconds	846472	unsigned long{data}@0x05ef
		currenttime_as_tsecondsdummy	4294941715	unsigned long{data}@0x05f6
		hours	23	unsigned char{data}@0x0600
		mins	30	unsigned char{data}@0x05f4
		seconds	47	unsigned char{data}@0x05f5

 

Here's the function that should do the convert, but is worse than the single line.

What am I missing?

 


unsigned long convertHMSt_totSecs ( unsigned long H, unsigned long M, unsigned long S, unsigned long t)
{
	return (unsigned long) ( (unsigned long) t+ (unsigned long) (S*10UL)  + (unsigned long)(M*600UL)+ (unsigned long)(H*36000UL) );
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What's the deal here then.. I suspected something because it worked in one file but not another...

I copied the function to the same file as the ' working' single line of code.

Previously the function was in the file where the calculation failed.

 

Now it works ... but only when calling from within the same file.


	currenttime_as_tseconds = tenths + ( seconds *10 ) + (mins*600) + (hours*36000); // read time now		
    currenttime_as_tsecondsdummy =locconvertHMSt_totSecs(hours, mins,seconds,tenths);


}



unsigned long locconvertHMSt_totSecs ( unsigned long H, unsigned long M, unsigned long S, unsigned long t)
{
	return (unsigned long) ( (unsigned long) t+ (unsigned long) (S*10UL)  + (unsigned long)(M*600UL)+ (unsigned long)(H*36000UL) );
}



// from watch

		currenttime_as_tseconds	854681	unsigned long{data}@0x05ef
		currenttime_as_tsecondsdummy	854681	unsigned long{data}@0x05f6
		hours	23	unsigned char{data}@0x0600
		mins	44	unsigned char{data}@0x05f4
		seconds	28	unsigned char{data}@0x05f5
		
		

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

But it's not consistent... . oh my!

 

Now, the only one line works...two fail and have different answers even though the functions are identical but are in different files.

I think the 86xxx is correct

 


	currenttime_as_tseconds = tenths + ( seconds *10 ) + (mins*600) + (hours*36000); // read time now
    currenttime_as_tsecondsdummy =locconvertHMSt_totSecs(hours, mins,seconds,tenths); // function in this file
    currenttime_as_tsecondsdummyext =convertHMSt_totSecs(hours, mins,seconds,tenths); // function in other file

}

unsigned long locconvertHMSt_totSecs ( unsigned long H, unsigned long M, unsigned long S, unsigned long t)
{
	return (unsigned long) ( (unsigned long) t+ (unsigned long) (S*10UL)  + (unsigned long)(M*600UL)+ (unsigned long)(H*36000UL) );
}

 /// .. In external file

unsigned long convertHMSt_totSecs ( unsigned long H, unsigned long M, unsigned long S, unsigned long t)
{
	return (unsigned long) ( (unsigned long) t+ (unsigned long) (S*10UL)  + (unsigned long)(M*600UL)+ (unsigned long)(H*36000UL) );
}

//.. from watch

		currenttime_as_tseconds	797449	unsigned long{data}@0x05f3
		currenttime_as_tsecondsdummy	862985	unsigned long{data}@0x05fa
		currenttime_as_tsecondsdummyext	4294954159	unsigned long{data}@0x05e9

		hours	23	unsigned char{data}@0x0604
		mins	58	unsigned char{data}@0x05f8
		seconds	18	unsigned char{data}@0x05f9

 

Last Edited: Tue. Jun 19, 2018 - 06:53 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Which bit of 3600UL did you not understand?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Clawson

Yes - I missed that.

 

However, I would have thought that the UL parameter would obviate the need for additionally casting it within the function .

 

So I NOW have the  external file with this:

 

unsigned long convertHMSt_totSecs ( unsigned long H, unsigned long M, unsigned long S, unsigned long t)
{
	return (unsigned long) ( (unsigned long) t+ ((unsigned long) S*10UL)  + ((unsigned long)M*600UL)+ ((unsigned long)H*36000UL) );
}

 

The local file has  this:

 

unsigned long locconvertHMSt_totSecs ( unsigned long H, unsigned long M, unsigned long S, unsigned long t)
{
	return (unsigned long) ( (unsigned long) t+ (unsigned long) (S*10UL)  + (unsigned long)(M*600UL)+ (unsigned long)(H*36000UL) );
}

 

The results are:

 

    currenttime_as_tseconds =locconvertHMSt_totSecs(hours, mins,seconds,tenths); // function in this file
    currenttime_as_tsecondsdummyext =convertHMSt_totSecs(hours, mins,seconds,tenths); // function in other file

   // From the watch:

    	currenttime_as_tseconds	860907	unsigned long{data}@0x05f3
		currenttime_as_tsecondsdummyext	6061	unsigned long{data}@0x05e9

 

The local file function works.

The external file function fails --  when the functions are identical AND when the functions are cast as recommended.

 

 

 

** EDIT UPDATE **

 

I now have 3 functions that provide the correct result in both files and also with both casting formats as previously shown ( original and Clawsons correction).

I *think* it was due to the declarations not being consistent.

 

Thanks for clearing my head.

 

 

 

 

Last Edited: Wed. Jun 20, 2018 - 08:59 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think you did not notice that the local file and the remote file are not the same.

have a good look at the code you posted here and count the braces.......

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

I see little merit in trying to pack all that into one line of C:

unsigned long locconvertHMSt_totSecs ( unsigned long H, unsigned long M, unsigned long S, unsigned long t)
{
	return (unsigned long) ( (unsigned long) t+ (unsigned long) (S*10UL)  + (unsigned long)(M*600UL)+ (unsigned long)(H*36000UL) );
}

personally I'd go with:

uint32_t locconvertHMSt_totSecs ( uint32_t H, uint32_t M, uint32_t S, uint32_t t)
{
    t += S * 10;
    t += M * 600;
    t += H * 36000;
    return t;
}

That is the way the compiler will evlauate it anyway so the generated code will be the same. I'd use uint32_t not unsigned long so it's clear to the reader how many bits I think the inputs/outputs have. As I presume this is used as:

t = locconvertHMSt_totSecs( 7, 35, 29, t);

or something like this I would be tempted to simply pass t by reference rather than value with a void return (though I suppose there's no harm in also returning the modified t too?). So something like:

void locconvertHMSt_totSecs ( uint32_t H, uint32_t M, uint32_t S, uint32_t * t)
{
    *t += S * 10;
    *t += M * 600;
    *t += H * 36000;
}

or

uint32_t locconvertHMSt_totSecs ( uint32_t H, uint32_t M, uint32_t S, uint32_t * t)
{
    *t += S * 10;
    *t += M * 600;
    *t += H * 36000;
    return *t;
}

As the function parameters are all uint32_t then there is no need for casts in this. The H/M/S will be cast up to 32 bits in the code that prepares to call this function so the evaluation will all be done in 32 bits anyway. As one input to the multiplications is 32 bit the other input (10, 600, 36000) will be promoted to 32 bit too.

 

EDIT: actually, thinking about this, perhaps the intention is NOT for t to be modified but the function used as:

new_tSecs = locconvertHMSt_totSecs( 7, 35, 29, t);

in which case ignore the pass by reference thing.

Last Edited: Wed. Jun 20, 2018 - 12:24 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Cliff, I think the OP has been confused with the type casting.

 

As I do not do FW all the time (main work is HW development FW is only reading what the FW department has done when things go wrong or if I need something special for testing) I have log periods were I do not do anything.

Then when I start coding again or need to sit together with FW as things are not working as expected (by me them or both of us) I sometimes also get confused about this.

 

Also to the OP, when ever you suspect there is a math failure you can first do a manual calculation ( as you did) but do it the way you have written your code and always take in mind how big a variable can be.

I think the overflow problem has bitten each and every programmer at least once while thinking the outcome of a function could not be bigger than X and X fitted inside an 8 bit variable, but during the multiplications additions subtractions and divisions done it went over the byte value......