|
|
View previous topic :: View next topic |
Author |
Message |
gjm27n
Joined: 15 Jun 2016 Posts: 23
|
|
Posted: Mon Jul 04, 2016 3:10 pm |
|
|
Yep, yours works. Upgrading to 8.92 now since that appears to be the only difference. |
|
|
gjm27n
Joined: 15 Jun 2016 Posts: 23
|
|
Posted: Mon Jul 04, 2016 3:23 pm |
|
|
WOO HOO!!!! Upgraded to 8.92 and my hex file matches yours and works.
Now to get my original program running again. |
|
|
gjm27n
Joined: 15 Jun 2016 Posts: 23
|
|
Posted: Mon Jul 04, 2016 3:45 pm |
|
|
The original program is working again. So, what I have I learned? Lots, but most important,
MPLAB 8.63 and CCS 5.061 BAD!
MPLAB 8.92 and CCS 5.061 GOOD!
But going forward, you guys have helped me immensely, not only in tracking this down but in some debugging techniques I hadn't thought of. |
|
|
Ttelmah
Joined: 11 Mar 2010 Posts: 19608
|
|
Posted: Tue Jul 05, 2016 7:53 am |
|
|
What I would look at, is whether the compile syntax is exactly the same.
I'm slightly suspicious that the problem is actually created by using opt=0 through the compiler.
Most people don't change the optimisation. Have to wonder why you do?. Historically (perhaps ten years ago...), turning down the optimisation was worthwhile, as sometimes the optimiser did some silly things, but for the last few hundred compilers, the default optimisation has worked well.
If I wanted to change the optimisation, I wouldn't do it via MPLAB. I'd instead simply put the line:
#OPT 0
in the file to be compiled.
Now I note that the command line syntax listed for setting the optimisation, is given on the current compilers as:
+Y0
not
+Y=0
So I'm wondering if the old syntax is causing problems with the new compiler, and the later MPLAB (and corresponding plug-in), fixes this.... |
|
|
gjm27n
Joined: 15 Jun 2016 Posts: 23
|
|
Posted: Tue Jul 05, 2016 9:10 am |
|
|
I use these chips in embedded boards I have designed to run industrial equipment. When running in automatic mode, I use a state machine to sequence the operations. Each output requires a specific input prior to going to the next state. When optimization is default (9), the compiler optimizes out the waits on each steps inputs causing the machine to run incorrectly. I haven't tested levels between 0 and 9, just know 9 optimizes out important steps and 0 doesn't. One day soon I'll test the other levels more thoroughly and report what I find. I have verified even with 5.061, optimization 9 doesn't work for me.
Otherwise my compile lines looks the same. |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1362
|
|
Posted: Tue Jul 05, 2016 10:22 am |
|
|
gjm27n wrote: | I use these chips in embedded boards I have designed to run industrial equipment. When running in automatic mode, I use a state machine to sequence the operations. Each output requires a specific input prior to going to the next state. When optimization is default (9), the compiler optimizes out the waits on each steps inputs causing the machine to run incorrectly. I haven't tested levels between 0 and 9, just know 9 optimizes out important steps and 0 doesn't. One day soon I'll test the other levels more thoroughly and report what I find. I have verified even with 5.061, optimization 9 doesn't work for me.
Otherwise my compile lines looks the same. |
Normally correct use of the volatile keyword can prevent some of that. Can you give a small example of what code gets optimized out?
I'll be honest, the CCS compiler isn't usually aggressive enough to do that, but I have seen 2 occasions where I needed to make the appropriate variables volatile so they wouldn't be optimized out. |
|
|
gjm27n
Joined: 15 Jun 2016 Posts: 23
|
|
Posted: Tue Jul 05, 2016 9:16 pm |
|
|
Here are relevant snippets of code. On C_CLAMP_FWD_IN, the bit test gets optimized out so it immediately advances to the next step before the CLMP_FWD_IN prox switch is made. This happens with every revision of the compiler.
Code: |
ulTableAddr = label_address(STATE_TABLE_CYCLE);
...
goto_address(ulTableAddr + (ucCycleIdx * 4));
...
STATE_TABLE_CYCLE:
goto C_CLAMP_FWD_OUT;
goto C_CLAMP_FWD_IN;
goto C_PDCLAMP_FWD_OUT;
goto C_PDCLAMP_FWD_IN;
goto C_MANDREL_FWD_OUT;
goto C_MANDREL_FWD_IN;
goto C_CYCLE_DELAY;
...
C_CLAMP_FWD_OUT:
Set_Dsp_Addr(STAT_ADDR);
LCD_Dsp(CLAMP_FWD_DSP);
bit_set(CLMP_REV_OUT);
bit_clear(CLMP_FWD_OUT);
if (stBend.ucLube)
bit_clear(LUBE_ON_OUT);
ucCycleIdx++;
continue;
C_CLAMP_FWD_IN:
if (bit_test(CLMP_FWD_IN))
continue;
delay_ms(ucClampFwdDelay * 8);
bit_set(LUBE_ON_OUT);
Clr_Stat();
ucCycleIdx++;
continue;
|
|
|
|
PCM programmer
Joined: 06 Sep 2003 Posts: 21708
|
|
Posted: Tue Jul 05, 2016 11:21 pm |
|
|
We can't do much with this. Can you make it into a compilable test program ? |
|
|
Ttelmah
Joined: 11 Mar 2010 Posts: 19608
|
|
Posted: Wed Jul 06, 2016 1:31 am |
|
|
I have to say, use C!....
Goto, is an operation that should only ever be used in very rare circumstances.
K&R says:
"With a few exceptions like those cited here, code that relies on goto statements is generally harder to understand and to maintain than code without gotos. Although we are not dogmatic the matter it does seem that goto statements should be used rarely, if at all".
In fact Dennis Ritchie was against it's inclusion at all, but was persuaded that there were circumstances for severe error recovery, where it might be used.
As shown there is no normal C code route by which any of the function parts after C_CLAMP_FWD_OUT can actually be reached. Understand that 'goto_address', is not a normal C instruction, it exists purely for use in things like writing interrupt handlers, not normal program flow. Not surprising the optimiser will get rid of the code that cannot be reached. It should not be used as you show.
The manual entry for 'goto_address', says:
"This is not a normally used function except in very special situations.".
Says it all.....
What you show, should be done simply as:
Code: |
//assuming the table is inside some 'while' or 'do':
while(condition)
{
switch(STATE_TABLE_CYCLE) {
case 0:
Set_Dsp_Addr(STAT_ADDR);
LCD_Dsp(CLAMP_FWD_DSP);
bit_set(CLMP_REV_OUT);
bit_clear(CLMP_FWD_OUT);
if (stBend.ucLube)
bit_clear(LUBE_ON_OUT);
ucCycleIdx++;
break; //this will re-execute the 'while'.
case 1:
if (bit_test(CLMP_FWD_IN))
break;
delay_ms(ucClampFwdDelay * 8);
bit_set(LUBE_ON_OUT);
Clr_Stat();
ucCycleIdx++;
break;
//etc..
}
}
|
In fact this will be more efficient, since currently you are performing an array lookup, to jump to an address in a table, which is then performing another goto.
If you want to do your own table lookup (the switch will code as a table lookup already), then just have an array of function addresses, and do a function call directly to these. So:
Code: |
void C_CLAMP_FWD_OUT(void)
{
Set_Dsp_Addr(STAT_ADDR);
LCD_Dsp(CLAMP_FWD_DSP);
bit_set(CLMP_REV_OUT);
bit_clear(CLMP_FWD_OUT);
if (stBend.ucLube)
bit_clear(LUBE_ON_OUT);
ucCycleIdx++;
}
void C_CLAMP_FWD_IN(void)
{
if (bit_test(CLMP_FWD_IN))
return;
delay_ms(ucClampFwdDelay * 8);
bit_set(LUBE_ON_OUT);
Clr_Stat();
ucCycleIdx++;
}
//then
typedef void (*fp)(void);
fp functions[7]= { C_CLAMP_FWD_OUT, \
C_CLAMP_FWD_IN, \
C_PDCLAMP_FWD_OUT, \
C_PDCLAMP_FWD_IN, \
C_MANDREL_FWD_OUT, \
C_MANDREL_FWD_IN, \
C_CYCLE_DELAY };
//then whan you want to call the function
(*functions[STATE_TABLE_CYCLE])();
|
I'm very much 'not surprised' that this doesn't work with any optimisation at all....
As another 'minor comment', there is a standard in C, to reserve 'ALL CAPITALS', for things that are #defined, as a reminder that this is the case. |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1362
|
|
Posted: Wed Jul 06, 2016 7:10 am |
|
|
gjm27n wrote: | Here are relevant snippets of code. On C_CLAMP_FWD_IN, the bit test gets optimized out so it immediately advances to the next step before the CLMP_FWD_IN prox switch is made. This happens with every revision of the compiler.
Code: |
ulTableAddr = label_address(STATE_TABLE_CYCLE);
...
goto_address(ulTableAddr + (ucCycleIdx * 4));
...
STATE_TABLE_CYCLE:
goto C_CLAMP_FWD_OUT;
goto C_CLAMP_FWD_IN;
goto C_PDCLAMP_FWD_OUT;
goto C_PDCLAMP_FWD_IN;
goto C_MANDREL_FWD_OUT;
goto C_MANDREL_FWD_IN;
goto C_CYCLE_DELAY;
...
C_CLAMP_FWD_OUT:
Set_Dsp_Addr(STAT_ADDR);
LCD_Dsp(CLAMP_FWD_DSP);
bit_set(CLMP_REV_OUT);
bit_clear(CLMP_FWD_OUT);
if (stBend.ucLube)
bit_clear(LUBE_ON_OUT);
ucCycleIdx++;
continue;
C_CLAMP_FWD_IN:
if (bit_test(CLMP_FWD_IN))
continue;
delay_ms(ucClampFwdDelay * 8);
bit_set(LUBE_ON_OUT);
Clr_Stat();
ucCycleIdx++;
continue;
|
|
if you declare the CLMP_FWD_IN as volatile it shouldn't optimize it out (this is what the volatile word is for).
I don't see your declaration of CLMP_FWD_IN but as an example:
Code: | volatile unsigned int8 CLMP_FWD_IN; |
The same would apply to your other similar global variables. Here is a good write up on Volatile
http://www.barrgroup.com/Embedded-Systems/How-To/C-Volatile-Keyword
EDIT: removed a potentially confusing part. |
|
|
gjm27n
Joined: 15 Jun 2016 Posts: 23
|
|
Posted: Wed Jul 06, 2016 10:02 am |
|
|
Thanks for you input.
I agree with K&R and this is the only exception I ever use one in (In my day job, I'm a C++ engineer for the Aerospace industry and we have very strict coding standards). I'm old enough to remember the days (80's) when K&R came out and Ritchie's aversion to it and we avoided it like the plague. I only started using them in assembly for state tables. State tables work great in this case and this is the best way I know to implement it. This state table design was actually ported over from an assembly project I wrote almost 20 years ago for the PIC. Somehow, I knew I'd get comments on it. Over the years, I've considered switching to a switch/case statements but didn't since this has many advantages, I can easily add or remove operations as processes or machines change (or even re-arrange). Just need to insert them in order into the state table. No need to renumber the case statements or change anything else.
C_CLAMP_FWD_OUT is a label, not a declaration which is why I capitalized it (I treat labels the same as defines). |
|
|
Ttelmah
Joined: 11 Mar 2010 Posts: 19608
|
|
Posted: Wed Jul 06, 2016 12:39 pm |
|
|
The key point is that switch, in CCS, if you don't have a default, codes as a jump table. Hence just as efficient, and then the compiler will know this code is being executed.
Because goto_address is normally expected to be to a static address, and nowhere are you putting the target addresses into a array, the compiler does not know that the code it ever reached, so will optimise it away. You can trick this by using volatile - CCS implement 'half' of volatile - volatile has two meanings. The first is 'this variable can be changed from somewhere else (hence forcing the compiler to assume the code is required) - the second is that on some compilers this also forces the code to either disable interrupts round the fetch, or do repeated fetches (assuming the variable may change if multi-byte), but CCS don't do this. |
|
|
gjm27n
Joined: 15 Jun 2016 Posts: 23
|
|
Posted: Wed Jul 06, 2016 12:50 pm |
|
|
Thanks, I plan on looking into restructuring the code when I have time. I'm already at 85% of ROM usage on the 18F452 so at some point I'll need to address it. |
|
|
jeremiah
Joined: 20 Jul 2010 Posts: 1362
|
|
Posted: Wed Jul 06, 2016 2:24 pm |
|
|
Ttelmah wrote: | - the second is that on some compilers this also forces the code to either disable interrupts round the fetch, or do repeated fetches (assuming the variable may change if multi-byte), but CCS don't do this. |
Just as a side note:
This is not part of the C standard. This would be a compiler extension. The volatile keyword does not guarantee atomic access. This is not to say some compilers don't do that. I've only got experience with GCC, MSVC++, and a few random embedded ones. None of those do, but others may. Though I would wonder if changing the effect of the volatile keyword has any bearing on their ANSI compliance. |
|
|
Ttelmah
Joined: 11 Mar 2010 Posts: 19608
|
|
Posted: Thu Jul 07, 2016 3:22 am |
|
|
Yes.
Some C's, use the Java meaning of volatile.
In Java, volatile implies that every thread accessing a volatile variable will read it's current value before proceeding, and ensures that multi-byte variables will be read in a thread safe manner (so atomic access).
As standard, volatile does not guarantee thread safe access, and this (correctly), is the way that CCS handles it, just ensuring that the optimiser does not interfere in any way with the access.
MS C# (not C) does do this. It treats read and write to a volatile variable as shortcuts to Thread.VolatileRead, and Thread.VolatileWrite.
In MSVC, use of volatile, actually overloads the write/read, allowing _you_ to decide whether to use a thread safe fetch (which I think is actually the nicest way to handle this).
This latter way of handling things, is actually a very clean way of doing things, making it totally your decision.
I haven't checked recently, whether CCS is doing this. If so, it'd be a very good way of working. |
|
|
|
|
You cannot post new topics in this forum You cannot reply to topics in this forum You cannot edit your posts in this forum You cannot delete your posts in this forum You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|