Crazy Optimization

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

This is absolutely killing my program!

 

Environment: M328P, AS7 with reasonably up-to-date avr-gcc (can't figure out where to find the installed version number) on Win10. Programmer is Atmel Dragon, if it matters, though it should not.

 

I have the usual "endless loop" in main(). There is an initialization section at the top of main, followed by the loop. When I single step from the end of the initialization section to where it lands in the endless loop, it jumps directly to an FatFs f_write() function where NONE of the arguments are properly initialized and the file system is NOT even open! This is the firmware equivalent to murder! Not even time to shout "Et tu Brutus?"

 

When I look at the disassembled code, the first thing it does after the end if the initialization is to call that f_write() function. There is NOTHING in the c code that requests that as a task for initialization! It just seems to do it, as if it thinks that might be funny to try.

 

THEN, it jumps to a block toward the end of the run loop where it prepares for sleep. At that point, some of the wake-up events are NOT ready because earlier parts of the run-loop, on which those depend, have not been executed. There goes the second stab!

 

Is there any way to suggest/direct/force the transition from initialization to the run-block at a better/more desirable/preferred jump target point? I suppose that I could put the body of that run-loop into a function and make that function call as the only element of the loop. Would that force a particular order? If I were to do that, there would be copious references to global variables that are external to the function, and lint (which I am not yet using) would probably love that.

 

Thanks

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

Last Edited: Wed. Dec 5, 2018 - 12:10 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I've done things like:

   int main() __attribute__((noinline))

ie - make the sections of code that you're particularly interested in debugging separate functions that will be called explicitly.

I guess it's sort of a manual equivalent (and more focuses) of what "-Og" is supposed to do.

 

Your description seems unlikely, though.  Probably something else is going on.

 

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

Can't quite follow without seeing some code.

 

Though I do recall decades ago spending hours figuring out

what was wrong with this code:

 

int main ()
{
    ...

    if (something_false);
    {
        do_something();  // WTF why is this geting called???
        ...
    }
}

 

--Mike

 

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

I would be happy if something else is going on, provided that I can identify it. However, I watch it in disassembly view, and it runs exactly parallel to the trace in source  code. So, it seems to be doing it for some reason that the compiler thinks is  "good". The one saving grace, if there is one, is that without initialized call arguments, the f_write() fails very early in its execution, and seems to do relatively little damage. It seems counter-intuitive to me that we should be  evaluating compiler optimizations on the basis of whether damage has occurred or not!

 

I understand that a code example is desirable, but that loop is 450 lines! I have a hard time picturing how to extract a meaningful snippet.

 

I think that I will try the function approach and see if it is any more faithful to the original intent.

 

Thanks

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

Last Edited: Wed. Dec 5, 2018 - 12:26 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Update -

 

The contents of the run-loop was moved into a function with no specified attributes and it now transitions from initialization to run-loop as expected. Of course, the compiler MAY re-order things within that function but this now seems "under control". Hope that does not prove to be an illusion. Takes a few more bytes in my 95.5% full flash memory.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

Did this all follow some modification of the code?  I gather this is not all new code (especially when it's using 95% of flash!).  Is there any obvious point where working code turned into broken code?

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

It changed when I changed from an "open-write-close" of the  file system on a per-sample basis to one where the file is left open. 

 

The optimizer has "always" used an odd entry point to the run-loop since the beginning of the  code creation several years ago, but it has mostly been pretty benign. Normally, it would start out with the sleep at the end of the loop, and continue from there. Would not mind that, at all. But this call to FatFs f_write() is quite out of character, compared to earlier iterations.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

ka7ehk wrote:
Hope that does not prove to be an illusion
It may be.   Or it may at the very least be brittle.  Possibly a side-effect of inhibiting code-inlining.

 

Sequence point?

 

https://en.wikipedia.org/wiki/Sequence_point

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Wed. Dec 5, 2018 - 04:19 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I once had a problem because some of the initialisation code was missing. It was simply not linked with the rest of the program.

That drove me crasy for week until another AVRfreak suggested the right solution.

 

When having strange problems, an often used tactic is to reduce the amount of code.

Code that isn't there can not cause problems, (except for the extremely rare case I just mentioned).

 

So why is there fatfs code toto jump to?

 

You can get the version of avr-gcc with:

avr-gcc --version

Paul van der Hoeven.
Bunch of old projects with AVR's:
http://www.hoevendesign.com

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

You mention the idea that code that is not there won't cause a problem. I have not considered test removing code in this section of the program because every time I have changed code in the past, the entry point (from initialization to the run-loop) changes, and when I restore that code, the entry point returns to what it was earlier. So, removing code on a test basis, here, just proves that it changes things.

 

The code in question is driven by a state machine. At the time in question (immediately after initialization), the state is "IDLE" (verified) and, in this state, the process of writing data to the the file system is totally skipped. That is what makes the behavior in question so bizarre. The state IS initialized, it does NOT pass through the data write process, which is a substantial block of code, yet this one function in the middle of this skipped block is called between initialization and the "real" entry point into the loop. 

 

As I indicated, the consequences are moderately benign since f_write() fails very early due to the absence of an open file system to reference. However, it seems more of a joke than conscientious optimization, as if the optimizer is "thinking": lets see what we can do that won't do too much damage and will really drive the programmer totally batty. Yes, I know that this is dangerous personification, but that is what it seems to this human who is pretending to be a programmer.

 

Putting the entire guts of the run-loop into a called function seems to result in the loop being entered where I would really prefer and without any of the obtuse function calls described above. Of course, there is absolutely nothing to keep the optimizer from rearranging the order of executed code inside this new function, but for now, it seems to be behaving.

 

Best wishes

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

Jim,
You have a choice: either zip up your AS7 project or post a minimal code.
.
This means that readers can duplicate your problem and offer practical solutions.
If it reveals a bug in the tools you have got evidence from multiple readers.
And it certainly raises a lot of interest.
.
From my own experience most problems are generated by me.
Foreign eyes can spot the errors that I am oblivious to.
.
In the old days it was tedious to put all the project components together in a post.
Now you can just zip up the project directory and attach to a message.
.
David.

Last Edited: Wed. Dec 5, 2018 - 07:16 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ooops, at the moment, I only have the version that does not show the described behavior. I will go back and retrieve the earlier version and post it tomorrow afternoon, U.S. Pacific time. 

 

Jim

 

 

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

This behavior would really haunt me, and I don't think I would rest until I had figured it out.  I wonder if it has anything to do with the fact that you are so close to filling up memory (why that would matter, I don't know).  This might be too much work, but I would at least look into trying to compile for a 64k AVR and see if the problem remains or disappears.  I would also start stubbing out sections of the code to see if the problem goes away as the program gets smaller.

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

jim,

 

have you tried a clean build ( as7  rebuild all) ?

I have seen in the past that sometimes when changing a C file or a H file (can not recall which one of the 2 it actually was) that then in some cases these changes would not be included in the rebuild, so I had to do a full clean rebuild to actually have these changes take effect. As I do not have a JTAG like debugging capability all I could see was that the program would crash or at least not do what I expected it to do at that point.

 

 

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

Did not try that. Thanks for the suggestion. 

 

Working on getting back to the source code state in which that behavior occurred. Will try to see, then, if the same thing happens. My hunch is that it won't.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

Well, this "issue" has appeared again, involving the same f_write().

 

As a refresher, in the first case, the f_write() was buried in the middle of the code that is inside the run-loop of the program. The code would leave the initialization section before the run-loop, jump to the f_write() statement, then drop to the end of the run-loop, where it went to sleep, waiting for the next sensor interrupt. I  moved ALL of the code within the run-loop into a wrapper function, and this particular (unwanted) call to f_write() was eliminated. In this particular case, the call, while superfluous, was relatively benign, because the file system was not even open, so the "file object" had not even been initialized and f_write() exited with a failure result and did not attempt to write.

 

Now, inside the wrapper, the superfluous f_write()  call is back, in a different place. I fear that it might not be so benign, this time, because the file system IS open when it is called. Here is the "evidence", starting with run_main(), the wrapper function that contains the code for the run-loop. Near the middle of this code listing, you will see some annotations at the left edge, "[n]", where "n" is the execution sequence order.

 

//================================== MAIN RUN LOOP ======================================
void run_main( void )
    {

    #define ACCEL_OVERRUN_VAL   4
    static uint8_t AccelOverRunCount = ACCEL_OVERRUN_VAL;   //number of allowed over-runs

    //  - - - - - - - - - - - -  RTC MANAGE +REV11 - - - - - - - - - - - - - - - - -
    //                    global int DISABLED - no ints

    if (TIFR2 & (1<<TOV2))        //service RTC
        {
        TIFR2 |= (1<<TOV2);       //clear TOV2 overflow flag bit
        SecondFlag = 1;         //flag new value, results in no change if already set
        LocalSeconds ++;
        MinuteCount --;         //for Delay and Period
        if (MinuteCount == 0)
            {
            MinuteCount = 59;
            MinuteFlag = 1;
            if ( (MasterState == IDLE) || (MasterState == IDLE_AUTO) )
                {
                adc_flag = 1; //do low batt test every  minute in idle
                }
            } //end if MinuteCount
        } //end if timer overflow

        //  - - - - - - - - - - -  LED FLASH MANAGE - - - - - - - - - - - - - - - -- - -
        //                    global int DISABLED - no ints

        // Flash has 0.25 sec elements. Call LEDManager on EVERY pass.
        // Tolerates fast calls. 

        LEDManager();                   //display.c

        // - - - - - - - - - - - - - MANAGE MASTER STATE - - - - - - - - - - - - - - - -
        //                    global int DISABLED - no ints

        //TEST MODE timeout, AutoStart timeout, DelayStart timeout, burst timing
        DoMasterTiming(SecondFlag, MinuteFlag); //Tolerates off-second calls

        //Some event conditions only evaluated every second
        DoMasterEvents( SecondFlag );   //Tolerates off-second calls
        MasterState = DoMasterState( MasterState , MasterEvents );  // SHOULD allow fast calls but must test.

        if  (MasterState == SAMPLING )          //+REV12 replaces scattered sets and clears
            SampleStatus |= SAMPLE_Record;
        else if ( MasterState == SS_ACTIVE2 )
            SampleStatus |= SAMPLE_Record;
        else
            SampleStatus &= ~SAMPLE_Record;

        //SecondFlag is used below so no clear, here.
        MinuteFlag = 0;     //clear it  

        //  - - - - - - - - - -  MANAGE ADC TIMING +REV11 - - - - - - - - - - - - - - - -
        //                    global int DISABLED - no ints

        if ( SecondFlag )
            {
            if (  BurstOn == BURST_OFF ) //not currently in burst  mode
                {
                adc_sec --;             //count adc interval
                if ( adc_sec == 0 ) {   //interval has timed out

                    if ( SampleStatus & SAMPLE_Record ) //surrogate for MasterState SAMPLING or ACTIVE2
                        {
                        adc_flag = 1;               //signal to start new adc cycle
                        }
                    else if ( MasterState == USB_PROTO )
                        {
                        adc_flag = 1;               //signal to start new adc cycle
                        }
                    adc_sec = ADCSEC_reset;     //default currently 3600 sec = 1 hr
                    }   //end if adc_sec
                } //end if BURST_ON is off
            }  //end if SecondFlag

        // - - - - - - - - - - - - - - - ADC MANAGE - - - - - - - - - - - - - - - - - - -
        //                    global int DISABLED - no ints

        //Triggered by adc_flag
        //Needs to be called more often than once each second to manage the adc state
        if (MasterState != IDLE_LOPWR)   //don't bother in low power state
            {
            adc_state = do_adc_state (adc_state); //tolerates fast calls
            }  //end not IDLE_LOPWR

		// - - - - - - - - - - -  - - MANAGE HARDWARE SAMPLE - - - - - - - - - - - - - - -
        //                    global int DISABLED - no ints
        //   most of this section skipped when USB connected (just reads FIFO)

        //SAMPLE_Time is set to prevent incorrect execution if it reaches here
        //some time other than when a new sample is available. This insures that
        //this block is only executed when a new sample from the accelerometer is present.
        //Can happen if looping with no sleep (USB connected)!

        if ( SampleStatus & SAMPLE_Time )       //time to sample - set in INT2 ISR
            SampleStatus &= ~SAMPLE_Time;       //ONLY used in this block
            {

            //apply temperature value to whole block of accel values
            //actually displayed every 10 seconds on the 10 second mark
            Tvalue = AccelTemp();       // read temperature
            Tsum += Tvalue;             // average sum.
            Tidx ++;                    // count the entry into the average
            if ( Tidx >= NTsamp )
                {
                //32 bit has sufficient range to do this
                Tavg = 10L*(uint32_t)Tsum;
                Tsum = 0;
                Tavg = Tavg / NTsamp;   //
                Tavg = Tavg + 250;      // to convert to 10x temperature
                }

            //MUST read the buffer contents before opening the file. If this does not
            //happen, new samples will arrive during the open/seek operations and the
            //buffer will over-run, loosing samples. This WILL occur as the file access
            //time approaches 250ms.

            //read the ENTIRE contents of the  FIFO, not just 25 but usually will be 25.
            //FIFO_SRC register shows FIFO status
            uint8_t TheVal = AccelReadByte(LIS_FIFO_SRC);   // byte, FIFO status
            TheVal &= 0x1f;                         // byte count only
            uint8_t J=0;                            // average index

            for(uint8_t samcnt = 0; samcnt < TheVal; samcnt++)
                {
                //The actual Accel sample and process
                AccelRead();                // get one set accel values as Xval, Yval, Zval
                //do the average and display if the average is complete
                //J is initialized to zero every pass of the main loop
                SampleWrite = DoAverage();          // do the average and set block write if finished
                if ( SampleWrite )
                    {
                    Xary[J] = Xavg;                 //Nary[J] is buffer for averager output axis N
                    Yary[J] = Yavg;
                    Zary[J] = Yavg;
                    J ++;
                    }
                }  //end if for(samcnt)     

            //every time the LIS3DSH FIFO drops below the watermark, it clears the interrupt
            PCIFR |= (1<<PCIF2);      //+REV11 clear second-edge interrupt

[1]         if ( SampleStatus & SAMPLE_Record ) //multiple states
                {
[2]             res = FR_OK;        //+REV12 for f_write and following

                //walk throught the averager output buffer and write lines to memory
                // J is one larger than the index of the last saved value
[3]  [7]        for(uint8_t K = 0; K < J; K++)
                    {
     [8]            if (res == FR_OK) //inside for() to catch the earliest write error
                        {
     [9]                SampleNumber ++;            // count every sample, whether dropped or not
     [10]               Xavg = Xary[K];
     [11]               Yavg = Yary[K];
     [12]               Yavg = Zary[K];
     [13]               FormatLine();               // form csv line for USB or uSD; buffer pointed by csvstrptr

[4]  [14]               res = f_write(&fil, csvstrptr, strlen(csvstr), &bw); // Write it to the destination file
[5]  [15]               if (bw < MyLen)                                   // bw = bytes written; disk full
                            {
[6]  [16]                   res = FR_DISK_FULL;
                            }
                        } //end if FR_OK
                    }  //end for (K)

                f_sync (&fil);          //+REV12 flushes the write buffer to the uSD card

                LEDNewPat(AXIS_Z, LEDSTDYOFF, 0xff);
                if (res == FR_OK)  //if NOT FR_OK, res has earliest fail value
                    {
                    FileFailCount = FILE_FAIL_VAL;          //reset the counter
                    LEDNewPat(AXIS_Z, LEDSTDYOFF, 0xff);
                    }
                else
                    {
                    LEDNewPat(AXIS_Z, LEDREDFLASH, 1);      //not successful so show error
                    FileFailCount --;                       //count the fail
                    if ( FileFailCount == 0 )               //fail too many times in a row
                        {
                        MasterState = ReturnIdle(MasterEvents);
                        LEDNewPat(AXIS_Z, LEDSTDYOFF, 0xff);
                        }
                    }
                }   //end if SampleStatus & SAMPLE_RECORD
            } //end if SampleStatus & SAMPLE_TIME   

        // - - - - - - - - - - - - - - MANAGE INTERRUPTS - - - - - - - - - - - - - - - -

        ACCEL_ENABLE;   //enable accelerometer
        //serial enable is managed at USB detection
        sei();          //enable interrupt 

        // - - - - - - - - - - - - - PARSE SERIAL MESSAGE - - - - - - - - - - - - - - - -
        //                       global int ENABLED - ints

        //parse received serial command
        if ( SerialFlag )           //easier to debug
            {
            if ( (MasterState == USB_PROTO) )
                {
                ParseMessage();
                SerialFlag = 0;
                } //end MasterState
            }  //end if SerialFlag

        // - - - - - - - - - - - - - CLEAN SAMPLE STATUS - - - - - - - - - - - - - - - -
        //                       global int ENABLED - ints

        if ( SecondFlag && (adc_state == ADC_VALUE) )   //
            {
            adc_state = ADC_IDLE;       //
            }

        SecondFlag = 0; 

        // - - - - - - - - - - - - - COMPLETE SERIAL - - - - - - - - -  - - - - - - - - -
        //                       global int ENABLED - ints

        //If there is a character being transmitted, wait for it to complete

        while (!(TX_READY)) {}  

        // - - - - - - - - - - - - - - COMPLETE ADC - - - - - - - - -  - - - - - - - - -
        //                       global int ENABLED - ints      

        if ( adc_run ) {                    //then ADC is on
            //this test is valid only if adc is on
            while( ADCSRA & (1<<ADSC) ) {}    //wait until measurement is complete
            }

        // - - - - - - - - - - - - - - - SLEEP - - - - - - - - - - - - - - - - - - - - -
        //                       global int ENABLED - ints
        //                  exit global int DISABLED - no ints

        //sleep here if USB is off. Run loop without file access normally takes about 0.7ms
        //with occasional (~2s) one cycle 2.8ms. With file access, the  loop execution time
        //can approach 250ms. When it gets too close, the file has to be changed because
        //it is too long. WAKE_ISR indicates that the accel FIFO filled before getting
        //to sleep. AccelStatus is true if the accel FIFO  has been filled past the  watermark.

        if ( !( MasterEvents & USB_PLUGGEDIN ) )    //do sleep if no USB connected  +REV5
            {
            //test whether watermark exceeded or accel unterrupt has tripped (maybe both)
            if ( AccelStatus() )                //sleep only accel not full otherwise sample NOW
                {
                SampleStatus |= SAMPLE_Time;    // read  sample NOW
                AccelOverRunCount --;
                if ( AccelOverRunCount == 0 )   //too many over-runs in a  row
                    {
                    AccelOverRunCount = ACCEL_OVERRUN_VAL;  //reset it
                    MasterEvents |= FILE_TO;        // raise file change flag
                    FileSec = SetFileSec();         // reset the standard file timer
                    }
                }
            else                                // sleep only accel not full
                {
                AccelOverRunCount = ACCEL_OVERRUN_VAL;  //reset it
                if ( SampleStatus & SAMPLE_Record ) //temporary debug trap, sampling only
                    {
                    //do something that won't be optimized away.
                    YPON;YMOFF;
                    }
                //SleepTime is int16_t so it can be negative.
                SleepTime = TCNT2;
                TheWakeSource = WAKE_NIL;
                WDT_Init();                     // +REV11 init.c - watchdog for sleep 2sec failsafe timeout
                                                // also enables global ints.
                sleep_mode();                   //Sleep=PowerSave, clears and sets and SEI bit, right here.

                asm("nop");         //wait slightly after wake up
                asm("nop");         //
                asm("nop");         //
                asm("nop");         // trap here to view TheWakeSource

                WDT_off();          //+REV11 not needed until next sleep
                ACCEL_DISABLE;      //+REV11 turn off accel int
                cli();              //disable all other ints including serial when USB connected

                if ( SampleStatus & SAMPLE_Record ) //temporary debug trap, sampling only
                    {
                    //do something that won't be optimized away.
                    YPOFF;YMOFF;
                    }

                if (TheWakeSource == WAKE_ISR)  //the normal wakeup
                    {
                    //TCNT2 counts in units of 1sec/256. So 4 counts represents 60ms
                    //whichis about 1/4 of the normal 250ms sleep time.
                    SleepTime = TCNT2 - SleepTime;
                    if (SleepTime < 0)
                        {
                        SleepTime = SleepTime + 256;    //correct roll-over of 8-bit count
                        }
                    if (SleepTime < 4)
                        {
                        MasterEvents |= FILE_TO; //+REV11 raise file change flag
                        FileSec = SetFileSec();  //+REV11 rest the standard file  timer
                        } //end if SleepTime < 4
                    }  //end wake source
                else        //here only if TheWakeSource == WAKE_WDT or WAKE_NIL, latter unlikely
                    {
                    MasterEvents |= FILE_TO;        // raise file change flag
                    FileSec = SetFileSec();         // reset the standard file timer
                    SampleStatus |= SAMPLE_Time;    // read  sample NOW
                    } //end else wake source
                TheWakeSource = WAKE_NIL;
                }   //end if FIFO full
            }  //end if USB plugged in

    }

Here we have a snip of the .lss file, from that center section of the previous code, with the same sequence numbers added

 

[1]			if ( SampleStatus & SAMPLE_Record )	//multiple states
    1226:	80 91 20 02 	lds	r24, 0x0220
    122a:	86 ff       	sbrs	r24, 6
    122c:	a1 c0       	rjmp	.+322    	; 0x1370 <run_main+0x3c6> nb: ACCEL_ENABLE line
[2]    				res = FR_OK;		//+REV12 for f_write and following
    122e:	10 92 1c 06 	sts	0x061C, r1

				//walk throught the averager output buffer and write lines to memory
				// J is one larger than the index of the last saved value
[3]				for(uint8_t K = 0; K < J; K++)
    1232:	dd 23       	and	r29, r29
    1234:	09 f4       	brne	.+2      	; 0x1238 <run_main+0x28e>
    1236:	6b c0       	rjmp	.+214    	; 0x130e <run_main+0x364>
    1238:	00 e0       	ldi	r16, 0x00	; 0
    123a:	10 e0       	ldi	r17, 0x00	; 0
    123c:	c0 e0       	ldi	r28, 0x00	; 0

[4]				res = f_write(&fil, csvstrptr, strlen(csvstr), &bw); // Write it to the destination file
    123e:	0f 2e       	mov	r0, r31
    1240:	f0 e9       	ldi	r31, 0x90	; 144
    1242:	ef 2e       	mov	r14, r31
    1244:	f4 e0       	ldi	r31, 0x04	; 4
    1246:	ff 2e       	mov	r15, r31
    1248:	f0 2d       	mov	r31, r0
[5]						if (bw < MyLen) 									 // bw = bytes written; disk full
							{
[6]							res = FR_DISK_FULL;
    124a:	0f 2e       	mov	r0, r31
    124c:	f4 e1       	ldi	r31, 0x14	; 20
    124e:	df 2e       	mov	r13, r31
    1250:	f0 2d       	mov	r31, r0

				//walk throught the averager output buffer and write lines to memory
				// J is one larger than the index of the last saved value
[7]				for(uint8_t K = 0; K < J; K++)
					{
[8]					if (res == FR_OK) //inside for() to catch the earliest write error
    1252:	80 91 1c 06 	lds	r24, 0x061C
    1256:	81 11       	cpse	r24, r1
    1258:	55 c0       	rjmp	.+170    	; 0x1304 <run_main+0x35a>
						{
[9]						SampleNumber ++;			// count every sample, whether dropped or not
    125a:	80 91 1b 02 	lds	r24, 0x021B
    125e:	90 91 1c 02 	lds	r25, 0x021C
    1262:	a0 91 1d 02 	lds	r26, 0x021D
    1266:	b0 91 1e 02 	lds	r27, 0x021E
    126a:	01 96       	adiw	r24, 0x01	; 1
    126c:	a1 1d       	adc	r26, r1
    126e:	b1 1d       	adc	r27, r1
    1270:	80 93 1b 02 	sts	0x021B, r24
    1274:	90 93 1c 02 	sts	0x021C, r25
    1278:	a0 93 1d 02 	sts	0x021D, r26
    127c:	b0 93 1e 02 	sts	0x021E, r27
    1280:	f8 01       	movw	r30, r16
    1282:	e5 50       	subi	r30, 0x05	; 5
    1284:	fa 4f       	sbci	r31, 0xFA	; 250
[10]					Xavg = Xary[K];
    1286:	80 81       	ld	r24, Z
    1288:	91 81       	ldd	r25, Z+1	; 0x01
    128a:	a2 81       	ldd	r26, Z+2	; 0x02
    128c:	b3 81       	ldd	r27, Z+3	; 0x03
    128e:	80 93 16 02 	sts	0x0216, r24
    1292:	90 93 17 02 	sts	0x0217, r25
    1296:	a0 93 18 02 	sts	0x0218, r26
    129a:	b0 93 19 02 	sts	0x0219, r27
    129e:	f8 01       	movw	r30, r16
    12a0:	e2 5f       	subi	r30, 0xF2	; 242
    12a2:	fa 4f       	sbci	r31, 0xFA	; 250
[11]					Yavg = Yary[K];
[12]					Yavg = Zary[K];
    12a4:	80 81       	ld	r24, Z
    12a6:	91 81       	ldd	r25, Z+1	; 0x01
    12a8:	a2 81       	ldd	r26, Z+2	; 0x02
    12aa:	b3 81       	ldd	r27, Z+3	; 0x03
    12ac:	80 93 12 02 	sts	0x0212, r24
    12b0:	90 93 13 02 	sts	0x0213, r25
    12b4:	a0 93 14 02 	sts	0x0214, r26
    12b8:	b0 93 15 02 	sts	0x0215, r27
[13]					FormatLine();				// form csv line for USB or uSD; buffer pointed by csvstrptr
    12bc:	0e 94 2a 20 	call	0x4054	; 0x4054 <FormatLine>

						TESTON;
    12c0:	42 9a       	sbi	0x08, 2	; 8

[14]					res = f_write(&fil, csvstrptr, strlen(csvstr), &bw); // Write it to the destination file
    12c2:	f7 01       	movw	r30, r14
    12c4:	01 90       	ld	r0, Z+
    12c6:	00 20       	and	r0, r0
    12c8:	e9 f7       	brne	.-6      	; 0x12c4 <run_main+0x31a>
    12ca:	31 97       	sbiw	r30, 0x01	; 1
    12cc:	af 01       	movw	r20, r30
    12ce:	40 59       	subi	r20, 0x90	; 144
    12d0:	54 40       	sbci	r21, 0x04	; 4
    12d2:	60 91 05 01 	lds	r22, 0x0105
    12d6:	70 91 06 01 	lds	r23, 0x0106
    12da:	2c e8       	ldi	r18, 0x8C	; 140
    12dc:	35 e0       	ldi	r19, 0x05	; 5
    12de:	8c e2       	ldi	r24, 0x2C	; 44
    12e0:	96 e0       	ldi	r25, 0x06	; 6
    12e2:	0e 94 c9 1b 	call	0x3792	; 0x3792 <f_write>
    12e6:	80 93 1c 06 	sts	0x061C, r24
[15] 						if (bw < MyLen) 									 // bw = bytes written; disk full
    12ea:	80 91 57 02 	lds	r24, 0x0257
    12ee:	90 e0       	ldi	r25, 0x00	; 0
    12f0:	20 91 8c 05 	lds	r18, 0x058C
    12f4:	30 91 8d 05 	lds	r19, 0x058D
    12f8:	28 17       	cp	r18, r24
    12fa:	39 07       	cpc	r19, r25
    12fc:	10 f4       	brcc	.+4      	; 0x1302 <run_main+0x358>
							{
[16]						res = FR_DISK_FULL;
    12fe:	d0 92 1c 06 	sts	0x061C, r13
							}
			

In this case, the first (unexpected) call to f_write() returns FR_OK (!) while the second returns with FR_DISK_ERR. I have a hunch that the extraneous call to f_write() has something to do with the failure of the second (valid) call to f_write() but I don't have enough information on that to say with any confidence.

 

I am stuck!

 

Thanks for taking time to look at this.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

Last Edited: Thu. Dec 6, 2018 - 07:31 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Is this anomoly at line-99 a result of your editing or does it exist in your "proper" source file ?

 

if (SampleStatus & SAMPLE_Time)   //time to sample - set in INT2 ISR
        SampleStatus &= ~SAMPLE_Time; //ONLY used in this block
    {

 

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

Jim,

 

FatFS has version numbers.   You can post a link to the official FatFS code.

 

You can ZIP up your AS7 project.   Then readers can see your problem for themselves.

There is no limit to the size of ZIP that you can attach to your message.

 

This applies to any "library Release".    Anyone in the world can access the same "Release".

 

"Beta" software is different.    You must quote a Tag or date reference to a GitHub project for readers to get the same as you.

Quite honestly,  you avoid Beta software.    Only use official Releases unless you are a Beta developer.

 

Life is simpler if you can use the "regular" tools like AS7.    Or even MPLABX.

 

David.

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

Winterbottom - THAT is a genuine error! I will correct it. It was present in both problematic versions. Maybe it has something to do with this weirdness.

 

David -

 

   1. FatFs version is R0.09b. Yes, it is old, but it is the same version as used in the earlier product. I had such a terrible time getting it going that I thought that I had it "figured out" and did not want to repeat that process. It is the same version that you or Cliff did an example project with.

 

    2. Not sure why you think I am not using "regular tools". This is from a reasonably current version of AS7 and gcc.

 

    3. Thought the form I submitted would be easier to follow. Nobody can build and simulate the code because there is so much hardware dependency. For example, in the initialization code, if the sensor does not respond properly, it drops into an error loop, flashing an LED. To get to the place where it records data, there are several sequential hardware triggers (switch states) that have to occur. I will post a zip of the entire project, though.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

I am sure that R0.09b will be functional.   You just need to say R0.09b

 

AS7 projects tend to have everything under one folder.    You can just ZIP it up and attach.

Even if you reference R0.09b in a foreign directory,  it is easy enough to say.    Then we simply install our own foreign directory from an official R0.09b Release.

 

Yes,   you have either got to create a minimal project.    Or add sufficient "short circuits" to avoid your hardware dependencies.

 

David.

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

N.Winterbottom wrote:

Is this anomoly at line-99 a result of your editing or does it exist in your "proper" source file ?

 

if (SampleStatus & SAMPLE_Time)   //time to sample - set in INT2 ISR
        SampleStatus &= ~SAMPLE_Time; //ONLY used in this block
    {

 

Bingo:

avr-mike wrote:

Though I do recall decades ago spending hours figuring out

what was wrong with this code:

 

int main ()
{
    ...

    if (something_false);
    {
        do_something();  // WTF why is this geting called???
        ...
    }
}

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

joeymorin wrote:

Bingo:

 

Have to admit I'm quite surprised I got it!!  ;-)

 

--Mike

 

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

My hunch is that the culprit is right there. 

 

Recompiling and testing right now.

 

Jim

 

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

That was indeed the problem. And, a genuine "tufer", two-for-one. It also "fixed" another problem that resulted in a failing f_write(), failing with FR_DISK_ERROR!

 

A whole bunch of Freaks SuperPoints for joeymorin for the initial suggestion and a bushel basket of said Freaks SuperPoints for avr_mike for catching the error in live code.

 

This has cost me three days of work! All your help and suggestions are really appreciated. Now, if the lesson can just be speed-burned into the appropriate neurons, and it never happens again, I will be doubly appreciative.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

Last Edited: Fri. Dec 7, 2018 - 02:35 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ka7ehk wrote:

Now, if the lesson can just be speed-burned into the appropriate neurons, and it never happens again....

 

Unfortunately it took two or three more times getting bitten

by this type of error before it finally became a standard thing

to look for.

 

It'd be great if there was a way to coax the compiler to warn

of back-to-back blocks following an if or for, etc. since the

error is entirely legal syntax.  Or an option to require curly

braces.

 

But I've found to best remedy is to write many, many short

functions and avoid loops and branches as much as possible.

With very long functions containing lots of conditional code

the bug could be anywhere.

 

The worst (style-wise) function I ever wrote was almost 2000

lines which was some sort of text parser, and most of that

2000 lines was a state machine implemented as one giant

switch statement!  I shoot for 10 lines or less these days.

 

--Mike

 

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

avr-mike wrote:
It'd be great if there was a way to coax the compiler to warn

of back-to-back blocks following an if or for, etc. since the

error is entirely legal syntax.

that's for likely most any linter.

from PC-lint Plus Online Demo - Gimpel Software - The Leader in Static Analysis for C and C++ with PC-lint Plus

 

Likely that defect is also caught by Microsoft Visual Studio; going to take me a while tomorrow to look through the warnings lists unless one here can give it a go in Visual Studio.

/analyze (Code Analysis) | Microsoft Docs

 

"Dare to be naïve." - Buckminster Fuller

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

A whole bunch of Freaks SuperPoints for joeymorin for the initial suggestion and a bushel basket of said Freaks SuperPoints for avr_mike for catching the error in live code.

Much as I'd like to accept superpoints, it was avr_mike who made the first suggestion, and N.Winterbottom who caught it in your posted code.  I just yelled 'BINGO!' ;-)

 

Glad you got it sorted out though.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Ah, yes, a big batch of Freaks SuperPoints for Winterbottom!

 

He is the one with sharp eyes.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

Congrats to all on figuring this out, but I'm still confused.  I thought the problem was that the code was jumping into the while(1) loop at some point other than the top of the loop.  How would a semicolon / curly brace error cause that to happen?

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

I am a bit confused about that one, the original problem. Its more plausible with the current structure, wherein the run-loop code is in a wrapper function that tends to encourage a specific entry point. With this one, the odd jump was from the point where the incorrect brace was located to the f_write() inside what SHOULD have been a block with access controlled by the malformed control statement. Its still not crystal clear WHY, but, at this point, I accept it.

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

Jim,

 

Looking at the code in #16 I think you seriously need to read about https://en.wikipedia.org/wiki/Cy... - a possibly more easily digestible explanation is:

 

https://www.guru99.com/cyclomati...

 

The idea is that code should never be as complex as you show in #16. If it has that many code paths it will be untestable and unmanageable. When things get that complex you should look at breaking sections out into separate functions that can then be isolated and individually tested. It also means that when there's a problem it's much easier to isolate where you need to look.

 

In theory if you design before your code you should never end up with sequences that complex in the first place as during the design you will already have split things - but even doing that some jobs are so complex they can lead to a cascade of conditional code anyway. In which case you need to look at splitting things at the point of implementation.

 

There are static analysis tools that can perform complexity analysis such as QAC, Klockwork, Lint, etc. In the safety conscious environment where I work we have defined limits on cyclomatic complexity that cannot be exceeded. As the table on my second link above shows:

 

Last Edited: Fri. Dec 7, 2018 - 01:01 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
There are static analysis tools that can perform complexity analysis such as QAC, Klockwork, Lint, etc.
and metrics tools like RSM.

Cyclomatic Complexity - RSM

 

"Dare to be naïve." - Buckminster Fuller

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

avr-mike wrote:
Unfortunately it took two or three more times getting bitten

by this type of error before it finally became a standard thing

to look for.

That rule is a part of one C coding standard :

EXP15-C. Do not place a semicolon on the same line as an if, for, or while statement - SEI CERT C Coding Standard - Confluence

 

"Dare to be naïve." - Buckminster Fuller

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

clawson wrote:
There are static analysis tools that can perform complexity analysis such as QAC, Klockwork, Lint, etc.
by a linter, a follow on to cyclomatic complexity :

C: Cognitive Complexity of functions should not be too high

SonarSource Blog » Cognitive Complexity, Because Testability != Understandability

 

edit: 2nd URL

 

"Dare to be naïve." - Buckminster Fuller

Last Edited: Sat. Dec 8, 2018 - 02:29 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Thanks for that critique, Cliff and others who have commented.  I DO appreciate it. 

 

Most of the rest of the program is more concisely organized. This part has sort of grown in an ad-hoc way as a result of a way-too-sketchy initial design that overlooked or missed features and details that became obvious only later (thats one of the hazards of a "one-man-shop"). For example, in the earlier version of the product, the core state machine was implemented as bare code that was part of the run-loop. Here, that function is broken out and handled. much more as it should be, in a separate testable module. So, its not all that bad and I do agree that the section shown is pretty challenging.

 

I also readily admit that I am not a programmer by training. I did a bunch of 8051 assembler stuff, "learned on the job" so to speak, then after a couple of year gap, made the jump to C and AVR. Took ONE, one-quarter, C-language class that, plus what I have read, learned on this "job", and tutored-by-freaks, is the extent of my knowledge. So, ideas like lint and cyclomatic complexity are still pretty abstract. I will keep plugging away at it, with the excellent help from all of you.

 

Again, thanks

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

Question  on complexity.

 

If you hide some alternate paths inside a function, does that reduce the cyclomatic complexity of the next function, up?

 

Here is an example: suppose that you have a function with high cyclomatic complexity. Then, you take a portion of that function which has a lot of alternate paths, and break it out into a separate function. Does the complexity of the original function then drop, accordingly?

 

Thanks

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

Last Edited: Fri. Dec 7, 2018 - 11:02 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

gchapman wrote:

avr-mike wrote:
Unfortunately it took two or three more times getting bitten

by this type of error before it finally became a standard thing

to look for.

That rule is a part of one C coding standard :

EXP15-C. Do not place a semicolon on the same line as an if, for, or while statement - SEI CERT C Coding Standard - Confluence

 

This bit of history predates the modern internet (1993) when C++

was so new there were significant compiler bugs (gcc) and nothing

like lint available, at least not to us with our small budget.  Also

the semicolon was a mistake, not intended to be there, so not really

a violation of coding standards.  Though if we had coding standards

and some way to check them, then yes this error might have been

uncovered quickly.

 

--Mike

 

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

There's talk of McCabe's Complexity Number but could find the actual score for the posted code - so I ran cccc and got MVC=37 (I don't disagree)

 

There's also much talk of a trailing semicolon on the same line as an if(). That however wasn't the actual bug. It was the transposition of two lines at line-99 of the posted code:

 

    if ( SampleStatus & SAMPLE_Time )       //time to sample - set in INT2 ISR
        SampleStatus &= ~SAMPLE_Time;       //ONLY used in this block
        {

Should have been:

    if ( SampleStatus & SAMPLE_Time )       //time to sample - set in INT2 ISR
        {
        SampleStatus &= ~SAMPLE_Time;       //ONLY used in this block

 

I use cppcheck myself but that didn't find any problem here.

 

I tried to compile this to see if gcc with -Wextra warnings would find any problem so started to throw in #defines, locals and globals where required.

I gave up though because there's just too much to sort out in a reasonable time.

 

BTW: During that failed exercise I found more oddness:

 

At line-134

			if (SampleWrite)
			{
				Xary[J] = Xavg; //Nary[J] is buffer for averager output axis N
				Yary[J] = Yavg;
				Zary[J] = Yavg;
				J++;
			}

Did you mean Zary[J] = Zavg;

 

 

A similar bug at line-156

     [9]                SampleNumber ++;            // count every sample, whether dropped or not
     [10]               Xavg = Xary[K];
     [11]               Yavg = Yary[K];
     [12]               Yavg = Zary[K];

 

Last Edited: Fri. Dec 7, 2018 - 11:10 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

N.Winterbottom wrote:
That however wasn't the actual bug.
That's in a C coding standard.

EXP19-C. Use braces for the body of an if, for, or while statement - SEI CERT C Coding Standard - Confluence

N.Winterbottom wrote:
I use cppcheck myself but that didn't find any problem here.
and the PC-lint Plus interactive demo didn't catch the defect.

"Automated Detection" in EXP19-C lists Klocwork; if Cliff has access to it then may Cliff run that snippet through it?

Also listed is a SonarSource product (SonarQube C/C++ Plugin); their C linter is a Visual Studio extension :

Visual Studio extension | SonarLint

due to SonarCFamily for C/C++ | C/C++ static code analysis | SonarSource

 

edit: last URL

 

"Dare to be naïve." - Buckminster Fuller

Last Edited: Sat. Dec 8, 2018 - 01:22 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

N.Winterbottom wrote:
There's also much talk of a trailing semicolon on the same line as an if(). That however wasn't the actual bug. It was the transposition of two lines at line-99 of the posted code:
The part that stood out to me was the semicolon, because that's one of the things I try to keep an eye out for i.e. single-statement if clauses.  The added evidence of an immediately following brace block suggested to me the OP likely intended the block to be the if clause.  Where the single-statement belonged I didn't contemplate, as I hadn't perused the rest of the posted code.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Winterbottom -

 

Oh, thank you for catching those copy-paste errors. They are certainly not involved in the previously describe "problems" but they sure make a mess of the data!

 

Jim

Jim Wagner Oregon Research Electronics, Consulting Div. Tangent, OR, USA http://www.orelectronics.net

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

gchapman wrote:

avr-mike wrote:

It'd be great if there was a way to coax the compiler to warn

of back-to-back blocks following an if or for, etc.

that's for likely most any linter.

 

I use the Arduino IDE on Mac to compile and upload AVR code

and found that it does catch this error class with the following

warning message:

 

warning: suggest braces around empty body

in an 'if' statement [-Wempty-body]

 

even without enabling verbose compilation mode.  Checking the

gcc man page, this warning is not enabled by -Wall, but requires

-Wextra (which I didn't know about).  So if you use avr-gcc check

whether you have -Wextra in your compile options!

 

--Mike

 

 EDIT: I have the compiler warnings preference set to "All."  The

other choices are None, Default, and More.  A warning is only

produced with All.  Also it does not emit a warning if there is a

valid statement between the if and the curly brace, only if there

is a bare semicolon, so would not have caught OP's mistake.

 

Last Edited: Sat. Dec 8, 2018 - 04:27 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 

and the PC-lint Plus interactive demo didn't catch the defect.

I forgot to enable options.

Defective :