Alternative to CASE and nested IF statements

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

Not sure if the title is accurate but I will try and explain myself better here.

 

I am receiving commands through my serial port from the host and storing them in a small buffer in ASCII format.  For basic testing I am sending a simple two byte command.

 

For example:

For Zone 1 I have

Turn on  = '11'  (0x31, 0x31)

Turn off = '10'  (0x31,0x30)

 

Currently I am using a CASE to decipher the first digit, then a couple of IF statements to determine the on or off:

 

  while(1)
    {
        if (rx_buffer_NL0 == 0x01)
        {
			rx_buffer_NL0 = 0;
			rx_counter0=0;
			rx_wr_index0=0;

			switch(rx_buffer0[0])
				{

					case '1':
					{
						if(rx_buffer0[1] == 0x31)
						{
							SPRINKLERS = 0x01;
						}

						else
						{
							if(rx_buffer0[1] == 0x30)
							SPRINKLERS = 0x00;
						}

					}

					break;

					case 0x00:
					SPRINKLERS = 0x00;
					break;

					default:
					break;
				}

		}

		else
		{

		}

    }

THis does work, but I am going to have other issues and the full command will not be two bytes, but more like 5 to 7 bytes where the first two will be the 'device'(zone/pool/light), the third byte will be an on or off, and the last three will be a 0 - 100 value for the lamps.  All terminated with a NULL

 

Currently I am primarily interested in the first three bytes for ON/OFF.  Based on the above code I was thinking I would have to add another IF statement inside the first one to decipher the third byte

 

example:

Buffer holds 'Z,1,1,0,0,0,NULL'  This means that Zone 1 gets turned on.  The three zeros are to be ignored

 

Based on the above posted code I was thinking that I would have to do this:

  while(1)
    {
        if (rx_buffer_NL0 == 0x01)
        {
			rx_buffer_NL0 = 0;
			rx_counter0=0;
			rx_wr_index0=0;

			switch(rx_buffer0[0])
				{

					case 'Z':
					{
						if(rx_buffer0[1] == 0x31)
						{
							if(rx_buffer0[2] == 0x31)
							{
							    SPRINKLERS = 0x01;

							}
						}

						else
						{
							if(rx_buffer0[1] == 0x31)
							{
							    if(rx_buffer0[2] == 0x30)
							    {
							        SPRINKLERS = 0x00;

							    }
							}
						}

					}

					break;

					case 0x00:
					SPRINKLERS = 0x00;
					break;

					default:
					break;
				}

		}

		else
		{

		}

    }

And I ignore teh last three bytes as they are not needed for this as a ZONE is on or off

 

This to me seems wasteful, and I am wondering if there is a better way.  I was looking at SCANF, but that is looking for data from a stream, and is not doing a compare.

 

So my question is could some of you weigh in with some suggestions on what I can look at for performing a comparison of multiple bytes in an array/buffer, or is the format I outlined in teh second code post the way to go.

 

Thanks,

Jim

 

EDIT:

The other command I was thinking of is STRCMP.

 

EDIT:

Keep in mind I have about 30 different commands.  24 of them ignore the last three bytes, the rest will use the last three only after the first three are a match

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

Last Edited: Sun. May 29, 2016 - 01:22 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jgmdesign wrote:
This to me seems wasteful,...

I'm confused.  What are you wasting?  Cycles?  Flash words?  Source lines?  Are they in short supply?  How much of the scarce resource is being used?  What is your target use for that resource?

 

AFAIK LEX/FLEX are still optimal scanners.  But with your extremely tiny language there might be a lot of one-time overhead with the tables.

 

Is this the same situation where I brought up FLEX some months ago?

https://www.avrfreaks.net/forum/s...

 

 

 

 

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

theusch wrote:
Is this the same situation where I brought up FLEX some months ago?

I had forgotten about that thread as the add-on never happened, but good reference.

 

I can see the similarities though of what I asked then and what I am asking now.  I do admit I know nothing of LEX and FLEX so pardon my ignorance of them.

 

Before I came back here and read your post Lee I did some work on my own and for a single Zone I came up with this:

#define Zone1_ON	"Z11"
#define Zone1_OFF	"Z10"

			if(strncmp(rx_buffer0,Zone1_ON,3) == 0)
			{
				SPRINKLERS = 0x01;

			}

			else
			{
				if(strncmp(rx_buffer0,Zone1_OFF,3) == 0)
				{
					SPRINKLERS = 0x00;

				}
			}

This ignores the last three bytes - which I wanted and the port does what I wanted it to do.  My issue is that this seem like a lot of UN-neccessary repetitive IF/ELSE statements as I would have to do this 8 times for the ZONES

 

 

Now this snippet from the other thread:

char commands[COMMAND_NUM][COMMAND_LEN] = {
  "read",
  "dump",
  "open",
  "list",
  "drop",
  "load"
};

.....

  // Match
    for (match=0; match<COMMAND_NUM; match++) {
      if (strncmp(input, commands[match], COMMAND_LEN) == 0) {
        break;
      }
    }

    switch (match) {
      case 0:
        // Action 'read'
        puts("You said'READ'\r\n");
        break;
      case 1:
        // Action 'dump'
        puts("You said'DUMP'\r\n");
        break;
      case 2:
        // Action 'open'
        puts("You said'OPEN'\r\n");
        break;
      case 3:
        // Action 'list'
        puts("You said'LIST'\r\n");
        break;
      case 4:
        // Action 'drop'
        puts("You said'DROP'\r\n");
        break;
      case 5:
        // Action 'load'
        puts("You said'LOAD'\r\n");
        break;
      default:
        // No match
        puts("NO MATCH\r\n");
        break;
    }

Looks to be about the same amount of work per se, but to me appears to be more efficient, and far more readable.

 

Efficient from the FOR loop to find the match. Easier to read using the SWITCH/CASE as there are not so many IF/ELSE.

 

 

Jim

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

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

theusch wrote:
How much of the scarce resource is being used?  What is your target use for that resource?
One such scarce resource is a cyclomatic complexity limit for a function per a guideline or rule.

Use of hierarchical finite state machines is one way to reduce cyclomatic complexity.

Better Embedded System SW: Avoid High Cyclomatic Complexity

by Phil Koopman

June 2, 2014

http://betterembsw.blogspot.com/2014/06/avoid-high-cyclomatic-complexity.htm

http://www.slideshare.net/quantum-leaps/beyond-the-rtos-a-better-way-to-design-realtime-embedded-software-61417916 (slide 14, Reduce "Spaghetti Code" with State Machines)

from

http://embeddedgurus.com/state-space/2016/04/beyond-the-rtos-a-better-way-to-design-real-time-embedded-software/

 

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

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

I count bytes and use that byte number as the driver for the switch statement. Thus, each case deals with one of the sequential characters in the command.

 

Clearly, there may have to be a if statements within each case if the interpretation of that character depends greatly on an earlier one.

 

This is equivalent to considering the interpreter as a state machine. 

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

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

Well after the better part of the night, Gogole, and K&R I have come to the conclusion there is no real 'elegant' way to accomplish what I am looking to do other than SWITCH/CASE.  I used the code:

char commands[COMMAND_NUM][COMMAND_LEN] = {
  "read",
  "dump",
  "open",
  "list",
  "drop",
  "load"
};

.....

  // Match
    for (match=0; match<COMMAND_NUM; match++) {
      if (strncmp(input, commands[match], COMMAND_LEN) == 0) {
        break;
      }
    }

to get the command from the buffer, then feed the <MATCH> into a SWITCH/CASE to perform the task based on the CASE.  THe downside to this though is that The reis a lot more than 30 commands as I also need to get status from the remote sensor interface at times so that adds more things to decipher, and then the LED lamps will be reporting temperature and other settings so the total command set is closer to 50.  That's a lot of CASE-s to muddle through.

 

I started thinking of the CISCO CLI where you entered the command, and then some read or write parameter.  Very crude/simple, but effective.  That's what I am looking to do.  I have a hard time believing that CISCO has a huge SWITCH/CASE to perform their CLI.

 

Jim

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

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

Jim,

 

Think about it for a human's convenience.    Do not worry about the Compiler.

If you have simple code,  it is easier to understand and maintain.

 

If you are parsing "command strings",   you will be thinking about strcmp() or even lex

If you are parsing multi-level cases,   use the case construct.

 

Multi-level case or nested if makes little difference to the Compiler.

But a normal human might find one more readable.    Indentation is the key.

 

David.

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

david.prentice wrote:
If you are parsing "command strings", you will be thinking about strcmp() or even lex

I am using STRNCMP(), and Lee mentioned LEX, which I am going to do more reading on.

david.prentice wrote:
If you are parsing multi-level cases, use the case construct.

I will read up on this as well.

david.prentice wrote:
Multi-level case or nested if makes little difference to the Compiler.

OK

david.prentice wrote:
But a normal human might find one more readable.

I am not what is considered normal by any stretch but thanks.

david.prentice wrote:
Indentation is the key.

Yes, and I have a lot to learn on that.

 

 

Thanks for the feedback.  So you are essentially saying that the simplest solution is one big SWITCH/CASE with a lot of comments to what's going on then?

 

JIm

 

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

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

No, I am just saying: do what you feel most comfortable with.
.
Automated formatting / indentation makes your life easier. You will see the Case logic.
.
Personally, I like reasonably compact code. i.e. attractive to read but still fit on a page.

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

For speed and small and readable I would get the needed bits from the 3 bytes into a byte (or perhaps an int if needed), and then make a "flat" case, where all the check values have a enum name. 
 

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

Well yes,   most hardware people would think like this.   Much like decoding opcodes on microprocessor.

 

If your mind works differently,  you should choose what suits you.

 

Whichever way you do things,  you have to be happy.

 

David.

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

david.prentice wrote:
Multi-level case or nested if makes little difference to the Compiler.

Embedded Gurus - Experts on Embedded Software

Efficient C/C++ « Stack Overflow

Efficient C Tip #12 – Be wary of switch statements

by Nigel Jones

Saturday, April 10th, 2010

http://embeddedgurus.com/stack-overflow/2010/04/efficient-c-tip-12-be-wary-of-switch-statements/

...

The first thing I discovered is that compilers typically have a number of ways of implementing a switch statement.

They seem to be loosely divided into the following trichotomy:

...

 

Maintenance

...

I’d also be remiss in not noting the dreaded missing break statement maintenance problem.

However as a religious user of Lint, I’m not normally too concerned about this.

 

Switch statement alternatives

...

This means either explicitly use an if-else-if chain or use function pointers.

...

 

Recommendations

...

 

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

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

I don't know the real background for the programmer of that link, but he is missing the most common structure "the binary search", that don't have that bad worst case, and I think that today most compilers use that (if it cant find a system or less than something like 8 or 16 elements).

 

I remember that the LCC compiler (the one imagecraft use (or used)), there you can tell the compiler what to use and things like how full the row should before it use a table.

 

One time I was in a hurry so I cut and pasted  30 if else parts, that had kind of random numbers from 1 to 5. I checked how bad the code was, and was surprised to see that it had made a case structure (winAvr2010).

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

Example of a Warnier-Orr diagram for a three-byte MIDI message parser
use Lucida Console or Terminal font to align text correctly
=============================================

 MIDIBuff:   0     1     2     3     4     5     6     7     8     9     a     b     c     d     e     f  
            ff    ff    ff    ff    ff    ff    ff    ff    ff    ff    ff    ff    ff    ff    ff    ff   

 ** variables
 	BYTES			BOOLEAN FLAGS
        -----                   ------------
 bytes: RunStat <-- 0xff    	FirstByteofMsg <-- T
	MsgSize <-- 0xff    	LastByteOfMsg  <-- F
	NumData <-- 0xff        SysExMsg       <-- F
	CmdType <-- 0xff
        MIDIbuffPtr <-- 0
  
  
	                 /----------------------------                                  
	                |                                    /--------------------------------------                      
  	                | LastByteOfMsg << F                | MsgSize << 3      
                        | RunStat << NewByte                | NumData << 2     SysExMsg << F         
                        | FirstByteofMsg << T               \--------------------------------------    
                        | CmdType << NewByte AND 0xf0       |     
          /--------     |                                   |     /-------------------------------------  
          |             | CmdType == x80,x90,xA0,xB0,xE0----/     |MsgSize << 2    
          |       	| [xor]                                   |NumData << 1   SysExMsg << F       
          |   cmd ------| CmdType == xC0,xD0 ---------------------\-------------------------------------       
          |   (*1)  	| [xor]                                               
          |        	| CmdType == xF0 --------------------------/-----------------------------------------------               
          |       	|  [xor]                                   | 
          |             | CmdType == xF1,xF2,xF3,xF4               | SysExMsg << T           /-------------------- 
          |             |            xF5,xF6,xF8,xF9               |                         |  MsgSize  << 1    LastByteOfMsg << T  
          |		|            xFA,xFB,xFC,xFD               | NewByte = xFF ~ xF8-----\-------------------- 
          |             |            xFE,xFF                       |  [xor]                                        
          |             |    [xor]                                 | NewByte = xF1 ~ xF6                           
          |             |  CmdType == xF7                          |  [xor]                                        
          |             | MIDIBuff[0] << NewByte                   | NewByte = xF0                                 
          |             | MIDIbuffPtr << MIDIbuffPtr + 1           |  [xor]                                        
          |             \------------------------------            | NewByte = xF7 ---------------------------- /--------------------        
          |                                                        |                                            | LastByteOfMsg << T  
          |                                                        |                                            | FirstByteofMsg << F
          |                                                        \-------------------------------------       \-------------------- 
new       |  [xor]
AuxMIDI   |  
byte in---|  
          |                                     /--------------------------------------------------------- 
          |                                     | MIDIBuff[0] << RunStat             LastByteOfMsg << F    
          |             /----------------       | MIDIBuff[1] << NewByte             SysExMsg << F         
          |             |  FirstByteofMsg = T---| MIDIbuffPtr << MIDIbuffPtr + 2                           
          |             |                       \----------------------------------------------------------
          |             | [xor]                                                                           
          |             |                                                                                 
          |  \cmd-------| \FirstByteofMsg = T                                                              
          |  (*1)       |                                                                                 
          |             \--------------                                                                                 
          |                                                                                              
          |                                                                                               
          |                           /--------------------                                                                          
          |  \LastByteOfMsg=T --------|  nothing                                                                                                      
          |                           \--------------------                                                                         
          |    [xor]
          |                                         
          |   LastByteOfMsg =  T  
          |                                         
          \----------------------------
          
         
              (*1)  cmd=newbyte bit 7 set 
              
              
               0    1   2   3   4   5   6   7   8   9   a   b   c   d     13*320=4160 uSec   1/240th second
  Test data:   90  3c  50  40  50  43  50  91  30  00  34  00  37  00     120 Beats/minute:  240 clocks/sec: 2 beats/second
                C3-major triad runstat-on   C2-major triad runstat-on	  180 Beats/minute: 
                  notes on  Channel 1		 notes-off  Channel 2   
                  
                                                                                         

   For extensive decision-making programs like you are describing, I use a Warnier-Orr type flowchart.   I do it rough by hand on newsprint first and then transfer it to a text file as in the example above.  

   In the example above there is no buffer and each byte is parsed as it arrives.  I use lots of variables and flags, static and volatile if there are interrupts. 

  After the Warnier-Orr is finished, I "walk through" it with many different byte values.  Especially test for beginning and end values,  also "impossible" conditions.

  Add lots of notes and comments in case the program is set aside for a period of time, or shared with noobies.

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

sparrow2 wrote:
I don't know the real background for the programmer of that link, but he is missing the most common structure "the binary search", that don't have that bad worst case, and I think that today most compilers use that (if it cant find a system or less than something like 8 or 16 elements).
There's a comment about that :

http://embeddedgurus.com/stack-overflow/2010/04/efficient-c-tip-12-be-wary-of-switch-statements/#comment-140496

 

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

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

I suspect a linear search using memcmp would be fast enough.

const struct {
    char opcode[3];
    void (*func)(int);
} selections[] = {
    { "no", no_fun },
    { "ye", yes },
    { "ma", maybe },
    { "pa", father }
} ;

Edit: moved const

Iluvatar is the better part of Valar.

Last Edited: Tue. May 31, 2016 - 01:41 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Surely this is straightforward Computer Science.

 

Different algorithms have different performance depending on complexity.

e.g. a linear Search through 1000000 strings.

or hash-Search,  binary-Search, ...

 

But a linear Search through 5 items will probably be simpler and quicker than the "clever" algorithms.

 

Embedded programs generally have very small numbers.   Simple normally wins.

The modern Compiler will probably select the most appropriate method by itself.

 

David.

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

Embedded programs generally have very small numbers. 

NO or at least define small.

For a complete state machine it can easy be 50+ states if it's made flat.

 

Often speed don't matter for the switch, but like the data in this thread, perhaps some kind of speed matter, and I can easy see a change in 300+ clk added to worst case from one model to the other.

Some times it matter sometimes it don't. (but if someone can point out how to control it in GNU C I will like a link, I do know that LCC has that control, but I have not found it in GNU )

 

 

 

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

I think many of these suggestions were made without enough knowledge of the problem. We are told there are about 30 different commands but only see one example of the ZONE command.

 

Are perhaps these 30 are actually different permutations of the same ZONE command ?

I see a case 'Z': which would suggest there other commands like "A,1,1,0,0,0" for example; is this true ?.

What is the command format ? Your code only checks for single character digits at fixed string positions. Would "Z255,255,255,255,255" (with the commas) be a valid command ?

Is the command format fixed or is it your own design and can it be modified ?

 

From the short snippet of code you've written above, I agree, this initial design will get seriously out of hand very quickly. A bit more background would help here:

 

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

I do a similar command parsing in my code.

Since all my commands are 2-ascii characters such as "GO","SC","DP" etc etc

What I do is cast a pointer to the 2-bytes to a uint16_t*

 

It works something like this:

void parse_cmd(char *pSZ)
{
    uint16_t cmd;
    cmd  = *(uint16_t*)pSZ;
    
    switch(cmd) {
    case 0x4f47: // "GO"
    ...
    
    }
}
    
    

A similar approach could be used for 16-Bit (4 byte) commands.

The constants could be DEFINED for readability.

SpiderKenny
@spiderelectron
www.spider-e.com

 

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

Why not go the extra step and use a enum? (or at least a define)

So it will be :

case GO:

 

Add:

Perhaps I should add that the compiler will generate the same code.  

Last Edited: Mon. May 30, 2016 - 11:09 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

David Prentice wrote:

 

Surely this is straightforward Computer Science.

 

This ignores the fact that many, maybe most, of us do not have a formal background in Computer Science. Me, I'm an EE who stumbled into Embedded Systems. Have a 1-quarter community college course in C (maybe 20-25 classroom hours, never referenced K&R). Read lots, but that is it. Bet that a large fraction of Freaks have similar "backgrounds". Many have never even taken a formal class. Like right-coast Jim, we muddle through, trying to figure out, intuitively and by trial-and-error, the "best way". Simply saying that "Computer Science will tell you" is not very helpful. I am not suggesting that we ignore or discard Computer Science but rather, we not take for granted that all of us have been exposed to these concepts and that we "ought to know".

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

Last Edited: Mon. May 30, 2016 - 05:26 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I am an ex-farmer. I don't think that you need a formal education to tell the difference between searching from a million and searching from 5.
My main point was that the Compiler chooses the appropriate method.
You choose the high level logic. i.e. concentrate on clear program design.
Jim's original subject has nothing to do with speed or efficiency. However, I am sure that he would like his Pool to operate correctly at all times.
.
Of course there are occasions when every nanosecond matters. And your priorities will alter accordingly.
.
If you have ever thought about searching, lookup, sorting, ... you will have heard of O(n) i.e. Big-O notation. How time increaes with record numbers.
When your records are in millions, you need to find something in 5 or 6 steps.
Wnen your records are in 5 or 10, a linear search is 5 or 10 steps. But the steps are very short.
.
As I said, the modern compiler chooses the appropriate method for case anyway.

Last Edited: Mon. May 30, 2016 - 05:39 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As I said, the modern compiler chooses the appropriate method for case anyway.

That is the problem with a compiler like GCC it make the appropriate code for a PC or ARM, but that is no given that is good code for an AVR.

 

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

Sheesh--all this back-and-forth.  And indeed, much of this covered [at least in my curriculum 40 some years ago] in basic and intermediate computer science classes.  I'll opine that a state machine is fastest, and amenable to automatic generation if you choose to define the language and go that route, but for a few commands -- someone above mentioned "whatever is clearest".  Be sure to cover all possible paths, as well as handle exceptions including timeout.

 

Now on to specifics:

david.prentice wrote:
Jim's original subject has nothing to do with speed or efficiency.

 

That is NOT the way I read the original post.  In particular:

jgmdesign wrote:
This to me seems wasteful, ...

 

In "real" apps with a more sizable language, it isn't unusual to evaluate several approaches for efficiency of one kind or another.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

sparrow2 wrote:

As I said, the modern compiler chooses the appropriate method for case anyway.

That is the problem with a compiler like GCC it make the appropriate code for a PC or ARM, but that is no given that is good code for an AVR.

 

 

I just let the Compiler decide.    No,  I have never studied the exact behaviour.

 

Most look at the case values and select the most suitable method.

e.g. consecutive values might use indexed list.

e.g. massive number of cases might use hash tablles

e.g. small number of cases probably just linear search.

 

I would hope that the cost-benefit analysis is specific to AVR and not a PC.

 

I am quite prepared to be proved wrong.   However,   68000 compilers from the 1980s used to tailor switch code successfully.

I am sure that modern compilers are even better.

 

Yes,   I know that Jim was talking about "wasteful".   I think he was more worried about code size than efficiency.

Regarding size.   If it fits in your Flash memory,  saving a few words is seldom important.

Regarding speed.    His Pool will not care.   Obviously other applications might.

 

David.

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

The way I read it is that this only is the beginning , with 3 bytes but will end with 5-7 bytes that is why I'm thinking about a "BIG" switch.

 

But I guess we need more info from Jim.
 

I would hope that the cost-benefit analysis is specific to AVR and not a PC.

That is the problem with the newer versions (Atmel releases) that they aren't, and that is why the old winAVR often make better code.

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

sparrow2 wrote:
That is the problem with the newer versions (Atmel releases) that they aren't, and that is why the old winAVR often make better code.

I'm sure that you are more versed in winAVR code generation than I am.

 

But I must ask:  >>Why<< would Atmel versions of GCC cater >>against<< AVR?!? 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

WOW!!  Quite the healthy commentary.  Let me see if I can satisfy some of the questions that have been presented.

 

I used the term 'wasteful' in my OP as I could not come up with a better term to describe what I originally wrote.  And to some extent what I have now.  Based on some of the code posted on this site by others far more brilliant than I will ever be, I figured my SWITCH/CASE scheme was rather clunk-ish.  Even more so hard to read or maintain - Something I have been beat up over numerous times by several of the well respected.  So I looked at my SWITCH/CASE and grabbed my K&R along with Google and started looking for a far more 'elegant' and more human readable way of deciphering and/or picking apart the command packets that I am going to be feeding this controller.  But based on what I have read here either the SWITCH/CASE is the most practical, there is a more elegant way to decipher the command scheme I am using(or suggest a better scheme), or everyone is just enjoying the back and forth....possibly all of the above? wink

 

One thing that seems to have come up is the sped of deciphering the command.  This is NOT a timing critical application.  It controls lawn sprinklers, pool hardware, and lighting.  So I could live with a big fat SWITCH/CASE and move on to the next phase, but I also wanted to see if I could possibly come up with a cleaner, more easily readable code.  50 CASE statements is tough on the brain.

 

N.Winterbottom wrote:
I think many of these suggestions were made without enough knowledge of the problem. We are told there are about 30 different commands but only see one example of the ZONE command. Are perhaps these 30 are actually different permutations of the same ZONE command ? I see a case 'Z': which would suggest there other commands like "A,1,1,0,0,0" for example; is this true ?.

100% true.  There are other commands using letters in the first byte.  Here are just the ones I have for a start(pardon the lack of comments)

char commands[COMMAND_NUM][COMMAND_LEN] = {
	"Z11",	//zone 1 on
	"Z10",	//zone 1 off
	"Z21",
	"Z20",
	"Z31",
	"Z30",
	"Z41",
	"Z40",
	"Z51",
	"Z50",
	"Z61",
	"Z60",
	"Z71",
	"Z70",
	"Z81",
	"Z80",
	"Z00",
	"ZEM",
	"PP1",
	"PP0",
	"PH1",
	"PH0",
	"AP0"
};

THese are simple ON/OFF commands.  "Z11" for example turns Zone 1 ON, while "Z10" turns zone 1 OFF.  "PP1" = Pool Pump ON, "PH1" = Pool Heater ON.  The idea here was SIMPLE on/off commands.

 

N.Winterbottom wrote:
What is the command format ? Your code only checks for single character digits at fixed string positions. Would "Z255,255,255,255,255" (with the commas) be a valid command ? Is the command format fixed or is it your own design and can it be modified ?

"Z255,255,255,255,255"  would not be valid currently.

I can change the format at the moment as this is still in it's infancy.

 

N.Winterbottom wrote:
From the short snippet of code you've written above, I agree, this initial design will get seriously out of hand very quickly.

Agreed, hence why I wanted to see if there is a better way to do this as I think the way I have things now is rather 'wasteful'

 

ka7ehk wrote:
Read lots, but that is it. Bet that a large fraction of Freaks have similar "backgrounds". Many have never even taken a formal class. Like right-coast Jim, we muddle through, trying to figure out, intuitively and by trial-and-error, the "best way".

Thank you for putting to words what I have been feeling for the last few years!  Every time I write something I am constantly wondering if I am doing 'this' properly - Meaning writing the code in the cleanest fashion.  and ever since the thread on why Globals are bad and as one said "un-professional" I have been doubting some things even more.  So as Jim W put it I am 'Muddling' through it, but I elected to show what I have done so far as homework before asking the professors here.

 

theusch wrote:
Now on to specifics: david.prentice wrote: Jim's original subject has nothing to do with speed or efficiency. That is NOT the way I read the original post. In particular: jgmdesign wrote: This to me seems wasteful, ...

My apologies Lee if you interpreted my OP as looking for the fastest way to process commands as the basis for the thread.  SURE, if there is a way to process a 1 in 50 command tree I would like to see it, but then the need for speed also goes with my desire for code 'efficiency'

Let me put it this way...When I meant 'wastefu'l I meant that I felt that I was/am using too may redundant statements(CASE) to decipher what the command intends.  When I asked about 'efficiency' I meant what C statement would be able to process the 1 in 50 better.

 

david.prentice wrote:
Yes, I know that Jim was talking about "wasteful". I think he was more worried about code size than efficiency.
Wouldn't code size and efficiency go hand in hand to some degree?  If let's say using CASE statements I will need 50  of them and the associated one or two lines of code for the action then that results in 150 lines of code and say 400 bytes...not bad.  But if there was a way of doing the same job with half the lines of code, then IMO that would be better as it is more efficient, and possibly easier to read, yes?

 

david.prentice wrote:
Regarding size. If it fits in your Flash memory, saving a few words is seldom important.

agreed.  As I noted earlier one of my quests was to make the deciphering easier to read for a human.  I will be using up more FLASH space for the messages that are going to be on the 4 x 20 LCD  In reality the basic I/O is trivial in size.  The bridge between the RS232 from the host to RS485 to the REMOTE I/O is also trivial in size as well.

 

david.prentice wrote:
Regarding speed. His Pool will not care. Obviously other applications might.

Agreed.  I am only running at 11.059MHZ and commands are only coming in when the Host sends one at specific times during the day.  The time between commands can be hours.

 

Thanks for all the great feedback.

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

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

Though OP does not say so explicitly,

he seems to want to code for maintenance.

A search of names for a function pointer would seem to fill the bill.

The functions would all have to have the same parameters,

but they would not have to use the same parameters.

What to make a parameter and what to make global

is an exercise left for the reader.

 

The following function might be useful:

// returns zero if op is an initial string of buf.
// Long op's should be tested forst.
int8_t cmp2op(const char *op, const char *buf)
{
    uint8_t len=strnlen(buf, 2);
    return memcmp(op, buf len);
}

 

Iluvatar is the better part of Valar.

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

When confronted with a unwieldy programming problem like yours one should employ a technique called "Separation of Concerns". The internet is rife with articles on the subject; Microsoft have an example https://msdn.microsoft.com/en-gb... as does Wikipedia of course http://en.wikipedia.org/wiki/Sep....

 

This means that your command processor should not get involved with storing your lamp settings or switching relays but limit itself to decomposing the command string and dispatching the zone work to a zone module (zone.c) which knows how to store the settings and which relays to switch. The same applies again for a lamp module (lamp.c).

 

Such is the simplification obtained I found time to actually write your command processor for Zones & Lamps as a PC console application. (This is a great way to quickly get non I/O specific code built, running and tested plus if you use GCC it needs only little work to compile for AVR). Paste the code into a CodeBlocks Console Project or similar IDE to run it. We don't know how you've modelled lamps and zones so couldn't implement that.

 

main.c

 

///
/// main.c simply collects keyboard input of test commands
/// and feeds those strings to the command processor cmd();
///

#include <stdio.h>
#include <stdlib.h>

void cmd (const char *buff);

char CmdBuff[32];

int main (void)
{
	printf("Enter some Home Controller commands and press return. q to quit.\nTypical Entries are:\n");
	printf("Z00\n");
	printf("Z91\n");
	printf("L01,100,100,100\n");

	while(1) {
		printf("\n> ");
		scanf("%31s", CmdBuff);
		if(CmdBuff[0] == 'q') break;
		cmd(CmdBuff);
	}
	return 0;
}

cmd.c

#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>

//
// I didn't write zone.c or lamp.c so their setter functions are defined here
//
int8_t SetZoneControl (int8_t zone, bool enable);
int8_t SetLampControl (int8_t lamp, bool enable, int8_t r, int8_t g, int8_t b);

static int8_t zoneCmd (const char *buff);
static int8_t lampCmd (const char *buff);

void cmd (const char *buff)
{
	int8_t error = 0;

	// The first character decides the basic command
	switch(buff[0]) {

	case 'L': error = lampCmd(buff); break;

	case 'Z': error = zoneCmd(buff); break;

	default: printf("Command Unrecognised: %s", buff);
	}

	if (error)
		printf("You entered an error in your command: %s", buff);
}

///Handle a LAMP command of format Lnb,r,g,b
///Where n is Zone Number 0 - 9
///Where b is 0 or 1 representing OFF or ON
///Where r,g,b are 0 to 100 representing intensity
static int8_t lampCmd (const char *buff)
{
	int8_t r, g, b;
	int8_t lamp = (buff[1] - '0');
	bool lampOn = (buff[2] != '0');
	if (sscanf(buff + 4, "%hhi,%hhi,%hhi", &r, &g, &b) != 3)
		return 1;
	return SetLampControl(lamp, lampOn, r, g, b);
}

///Handle a ZONE command of format Znb
///Where n is Zone Number 0 - 9
///Where b is 0 or 1 representing OFF or ON
static int8_t zoneCmd (const char *buff)
{
	int8_t zone = (buff[1] - '0');
	bool zoneOn = (buff[2] != '0');
	return SetZoneControl(zone, zoneOn);
}
///
/// This Function should be implemented in zone.c alongside the zone data model
///
int8_t SetZoneControl (int8_t zone, bool enable)
{
	if ((zone < 0) || (zone > 9))
		return 1;

	printf("You wish to set Zone %hhi %s.\n", zone, enable ? "On" : "Off");
	return 0;
}

///
/// This Function should be implemented in lamp.c alongside the lamp data model
///
int8_t SetLampControl (int8_t lamp, bool enable, int8_t r, int8_t g, int8_t b)
{
	if ((lamp < 0) || (lamp > 9))
		return 1;

	if ((r < 0) || (r > 100)
	 || (g < 0) || (g > 100)
	 || (b < 0) || (b > 100))
		return 1;

	printf("You wish to set Lamp %hhi %s & RGB intensity to %hhi:%hhi:%hhi.\n", lamp, enable ? "On" : "Off", r, g, b);
	return 0;
}

 

 

Last Edited: Tue. May 31, 2016 - 03:09 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Wow, that is a very interesting way to perform the tasks without a large SWITCH/CASE.  And as you note it certainly makes things easier to read and manage.

 

Currently I am using my big SWITCH/CASE as I had to get this thing running some basic parameters, but I can set this up in another project and try it out as well.

 

Thank you!

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

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

Nice +1!

 

(Possum Lodge oath) Quando omni flunkus, moritati.

"I thought growing old would take longer"