Jump to content
terjeio

Compact command parser/dispatcher example

Recommended Posts

The problem I see with the approach above is the overhead of having pointer to functions instead of direct functions. Using C++ templates you can get compile time polymorphism with zero overhead.  I noticed you are exposing the delay_us() function as a HAL method. I'm guessing you aren't expecting to actually get a 1 microsecond delay.

 

I think the overhead is pretty minimal, after all calling a function is just modifing the program counter (after the stack i set up which need to be done anyway) - the value used to set the new program counter value has to come from somewhere in both cases. IIRC there is a case if the BL instruction contains an offset - but it is a long time since I last coded in ARM assembly (ARM 2 & 3) so my memory is a bit fuzzy regarding this. I am more concerned about the overhead in the ISRs as I have to call a function via a pointer in them... Anyway - Grbl is writen for a far slower 8-bit processor so for this implementation it should not be a significant issue.

Share this post


Link to post
Share on other sites

@@terjeio

 

Thank you for the example! I think I'm getting it.

 

Yes, the case:break switch block becomes a function. That makes sense.

 

The (char* params) thing is confusing me though. Are all possible command variables of type char? Not type int?

 

Hmmm... Could you show me how parseInt(params) works? Maybe that will clear up my confusion?

Share this post


Link to post
Share on other sites

By the way, I googled this topic like crazy last night. The internet doesn't talk about this topic very much. And when they do give examples, they aren't very practical or straight forward.

 

I found that the best cheerleader for them is Nigel Jones from embeddedgurus.com. He's the author of the article @@terjeio linked in the first post.

 

Thanks @terjio for walking me through this. I am grateful!

Share this post


Link to post
Share on other sites

@@zeke : I am happy that you found my post useful.

 

My example is a relative primitive command line parser, a command line is always a string (char *) so may contain all kinds (types) of parameters. When calling the function associated with the command I pass the command tail, a pointer to the rest of the command line. It is the up to the called function to parse this and do any required sanity checking/conversions. Some command line parsers may break up the line into an array of strings, usually using space as a delimiter - argc/argv as arguments to main() is an example of that.

 

The parseInt() function is a poor mans implementation of atoi(), it is inherited from the MSP430G2553 version of my code - atoi() uses quite a bit of RAM so is a no-no when only 512 bytes are available. Here it is:

int32_t parseInt (char *s) {

	int32_t c, res = 0, negative = 0;

	if(*s == '-') {
		negative = 1;
		s++;
	} else if(*s == '+')
		s++;

	while((c = *s++) != '\0') {
		if(c >= 48 && c <= 57)
			res = (res * 10) + c - 48;
	}

	return negative ? -res : res;
}

Pretty primitive and does not do much sanity checking - but works...

 

Some of my commands take two numerical parameters - comma separated, to parse them I use:

bool start (char* params) {

	char *p2;
	uint32_t xpixels = 0, ypixels = 0;
	bool ok = false;

	if((p2 = strchr(params, ',')) != NULL) {
		*p2++ = '\0';
		ypixels = parseInt(p2);
	}

	xpixels = parseInt(params);

	if(xpixels == 0 || ypixels == 0)
		serialWriteLn("Bad/missing parameters");

	else {
...

Hope this helps.

 

Terje

Share this post


Link to post
Share on other sites

There are downsides to using function pointers as such. One such downside would be complexity, would / could lead to bugs that may be hard to track down. If you're like me though, and write a small bit of code for one specific bit of program functionality, then test. You might be ok. But later down the road, say a couple years, months, or whatever it takes for you to forget what you were doing . . . such code would be very terse reading. Especially if you bounce around between languages like I do from one project to the next. But I'm more of a generalist than specialist, so I tend to have to refresh my memory all the time when writing applications. Sometimes . . . even remembering std libc stuff, etc like sprintf() / printf() . . . which is sometimes embarrassing.

 

EDIT:

 

Another thing to consider is that your control logic of your code is always going to need if /else statements. So you *could* look at using if / else block statements as the "smart thing" to do. If you believe in the K.I.S.S. mantra.

 

So when I say this I'm not putting down this idea, or the OP for the idea. But I've personally been finding the simpler you keep your code. The easier time you have of troubleshooting, and maintaining your code in the future. But this also goes to reason if you're very used to certain code strategies( i.e. you use function pointers a lot ), perhaps this then becomes "simple" because you're used to it.

Share this post


Link to post
Share on other sites

@@yyrkoon : I am a great believer in the KISS mantra, this was the main reason I changed my code. Surely when confronted with new concepts they may be hard to grasp at first but becomes a easy to use as they become familiar. IMO this site is all about sharing/learning, this is why I posted the example.

 

Consider an example from code (not written by me) I rewrote because I found it hard to read and understand, original:

BOOL SetHeadroom(char headroom)
{
	DWORD dwBytes;
	unsigned char output[]={HEAD,0x01,0x20,0x01,0x00,0x01,0x00,END};
	unsigned char input[8]={0};

	if ((headroom < 0) || (headroom > 12))
		return false;		// Headroom out of range

	output[6]=headroom;

	if(!WriteSerialBytes(hSerial, output, 8, &dwBytes, NULL))
		return false;

	if (dwBytes != 8)
		return false;

	// need to check the return
	if(!ReadSerialBytes(hSerial, input, 8, &dwBytes, true))
		return false;
	else
	{
		if(GoodHeader(input, dwBytes))
		{
			if (input[2]==1)	
				return true;
			else
				return false;
		}
		else
			return false;
	}


	return true;

my version:

bool STREAM_SetHeadroom (uint8_t headroom) {

	bool success = false;

	if(headroom <= 12) { //  check valid input

		Command *response, *cmd = createCommand(0x01, 0x20, 1);

		if(cmd) {

			cmd->data[0] = headroom;

			if((response = exeCommand(cmd))) {

				success = response->type == 0x00 && response->id == 0x01 && response->length == 0;

				free(response);

			}

			free(cmd);

		}

	}

	return success;
}

I introduced a struct and dynamic memory allocation (not shown) - unfamiliar concepts for many and surely more technically complex, but the my version is IMHO a lot easier to read and understand... Which is KISS?

Share this post


Link to post
Share on other sites

@@yyrkoon : I am a great believer in the KISS mantra, this was the main reason I changed my code. Surely when confronted with new concepts they may be hard to grasp at first but becomes a easy to use as they become familiar. IMO this site is all about sharing/learning, this is why I posted the example.

 

Consider an example from code (not written by me) I rewrote because I found it hard to read and understand, original:

BOOL SetHeadroom(char headroom)
{
	DWORD dwBytes;
	unsigned char output[]={HEAD,0x01,0x20,0x01,0x00,0x01,0x00,END};
	unsigned char input[8]={0};

	if ((headroom < 0) || (headroom > 12))
		return false;		// Headroom out of range

	output[6]=headroom;

	if(!WriteSerialBytes(hSerial, output, 8, &dwBytes, NULL))
		return false;

	if (dwBytes != 8)
		return false;

	// need to check the return
	if(!ReadSerialBytes(hSerial, input, 8, &dwBytes, true))
		return false;
	else
	{
		if(GoodHeader(input, dwBytes))
		{
			if (input[2]==1)	
				return true;
			else
				return false;
		}
		else
			return false;
	}


	return true;

my version:

bool STREAM_SetHeadroom (uint8_t headroom) {

	bool success = false;

	if(headroom <= 12) { //  check valid input

		Command *response, *cmd = createCommand(0x01, 0x20, 1);

		if(cmd) {

			cmd->data[0] = headroom;

			if((response = exeCommand(cmd))) {

				success = response->type == 0x00 && response->id == 0x01 && response->length == 0;

				free(response);

			}

			free(cmd);

		}

	}

	return success;
}

I introduced a struct and dynamic memory allocation (not shown) - unfamiliar concepts for many and surely more technically complex, but the my version is IMHO a lot easier to read and understand... Which is KISS?

 

It seems as though as you took what I said as a personal attack from me to you. That's not the case.

 

if/else blocks are basic C constructs that anyone can read and immediately understand if they're familiar with C. pointers on the other hand particularly function pointers are not basic C methodologies that everyone can understand. Even if both concepts are understood by a single individual. if/else are much easier to read though, and understand program flow at a glance.

 

Simple fact of the matter is that function pointers are complex, and complexity is not necessarily a good thing for maintainability.

 

Don't get me wrong, I've done the same thing myself. Writing complex code. Then I've ad the time afterwards to reflect on my code I realized my code was far more complex than it had to be. SO thinking about the code you've presented here. Right away I can see that function pointers are not necessary. Would the code be better without function pointers ? I do not know. I'd have to study it more closely. For which, right now, I do not have the time.

Share this post


Link to post
Share on other sites

For instance, this snippet out of your code is very unclear . . .

bool ok = false;
    uint32_t i = 0, numcmds = sizeof(commands) / sizeof(command), cmdlen;

    while(!ok && i < numcmds) {

        cmdlen = strlen(commands[i].command);

        if(!(ok = !strncmp(commands[i].command, cmdline, cmdlen)))
            i++;

OK so semantics . . . when is "ok" ever false ? I do not mean in your code here, I mean semantically. So right away the code is not self commenting. But this conditional check:

if(!(ok = !strncmp(commands[i].command, cmdline, cmdlen)))

Whats going on here ? double negative makes for very terse reading. So right my point proven. But becasue I was glancing at your code, instead of reading it completely I missed the fact that you're assigning the value of (not strncmp) to a boolean variable . . . which is also a no no . . . Also, I'm not sure comparing the return value of strncmp() to bool is actually a good thing either. bool is true or false, and return type of this function is most definitely an integer.

some_type some_variable_name = strncmp(commands[i].command, cmdline, cmdlen);
if(conditional check versus some_variable_name){
        /* Act accordingly */
}

Would be a lot clearer.

 

So responding to the specific question as to whether I think your code is K.I.S.S. . . . No I do not think your code is simple. However, I want to point out that I was also not originally talking about your code specifically. I was talking directly about the concept of function pointers, and how I think they can sometimes very easily be misused, or used in situations where they're not required or needed. So, in effect I was sharing too. Sharing my opinion.

Share this post


Link to post
Share on other sites

I fought with this code for about six hours yesterday and I gave up when I couldn't get this code to run properly for me

if(!(ok = !strncmp(commands[i].command, cmdline, cmdlen)))

It would test only the first character in the command field. The first command in the list was getting executed when things failed.  I couldn't figure out how to get it to compare the input string to the command name string. I got frustrated.

 

So, I started checking out the Tivaware cmdline.c implementation and I discovered that I understood it after spending so much time on this implementation. It breaks up the input string into different command line arguements and then passes them off for processing. At this time, it seems like a more logical methodology that this one. <shrug>

 

After a nights rest, I am rethinking the coolness factor of function tables. My giant switch statement is still working properly but the function jump table isn't.

 

I amy stick with the switch statement at this point. It's definitely more intuitive than the jump table.

Share this post


Link to post
Share on other sites

I fought with this code for about six hours yesterday and I gave up when I couldn't get this code to run properly for me

if(!(ok = !strncmp(commands[i].command, cmdline, cmdlen)))

It would test only the first character in the command field. The first command in the list was getting executed when things failed.  I couldn't figure out how to get it to compare the input string to the command name string. I got frustrated.

 

So, I started checking out the Tivaware cmdline.c implementation and I discovered that I understood it after spending so much time on this implementation. It breaks up the input string into different command line arguements and then passes them off for processing. At this time, it seems like a more logical methodology that this one. <shrug>

 

After a nights rest, I am rethinking the coolness factor of function tables. My giant switch statement is still working properly but the function jump table isn't.

 

I amy stick with the switch statement at this point. It's definitely more intuitive than the jump table.

And this is the main reason why I mentioned what I did yesterday. Not to berate anyone or their code. But to state for others who may not have experience with function pointers that the code could be very hard to implement at first go.

 

@@zeke also yes, once I refreshed my memory as to what strncmp() actually does I started to see what @@terjeio was doing. The reason why I was having difficulty at first glance seeing what was going on this is because I do not write code like this - ever. Meaning, I would approach the problem from a different angle. I did not mention that however because thats a personal preference, and  we're all likely going to think differently on that. Still a lot of confusion could have been alleviated by . .

int myVar = !(strncmp( . . .));
if(condition){
         . . .
}

Another thing to consider zeke. Is that you can do very close to what he's doing here using plain functions, with no function pointers . . .

Share this post


Link to post
Share on other sites

Additionally, and under the right circumstances. If you use if/else or switch blocks. You do not necessarily have to do command error checking. You just test for your various commands, and whenever something does not match. It just get's ignored. This method, is likely to be slower because it has to test command validity. *OR* you could implement an if/else or switch block test code . . . which leads us back to square 1 ;)

Share this post


Link to post
Share on other sites

Anyone have any thoughts or comments as to what the upside to using function pointers for a table driven state machine are ?

 

Just quickly thinking about it myself . . . The ability to run less on the stack at once ? Such as you're only 2 functions deep into the stack when using function pointers this way. main(), and then the active function. Where main() is always on the stack in any C application anyway. Something like this however . . .

        function stage4(){
                if(condition){
                        // take some action
                } else{
                        // take an alternative action
                }
        }
        function stage3(){
                if(condition){
                        // goto stage4
                }
        }
        function stage2(){
                if(condition){
                        // goto stage3
                } else{
                        // take action or goto an alternative stage3
                }
        }
        function stage1(){
                if(condition){
                        // go to stage2
                }
        }

        function timer(){
                seconds++;
                stage1();
        }
        setInterval(timer, 1000);

Yes, I realize this is javascript. But I actually have a project right now that I'm prototyping in javascript first. Before I port it to C. Anyway . . . as we can see here, taking this approach( using functions without pointers ) we're able to achieve something similar. *BUT* we're also potentially putting more onto the stack at once. So while this may work for situations where you have only a few different possible "stages". Having many potential "stages" or states, using the command dispatcher could be the better option.

 

The main reason why I like this approach better. At least for my current situation is that I have many nested conditions that must be met, or tested for. While there is also a priority hierarchy. Structuring my code in this way, allows me to very clearly follow the logic of the code, multiple nested conditions and all. Without actually have to look at the "spaghetti-ness" of nested conditional branches.

 

So this, is not exactly a command dispatcher. No . . . but it is a way to achieve the same end goal under certain situations.

Share this post


Link to post
Share on other sites

@@yyrkoon

You're getting mighty close to needing a state machine with that example ;-)

 

After all of this learning about jump tables, I think the importance of readibility is becoming a higher priority to me than the whizzy cool factor.

 

I haven't given up on trying to make this work.  This time, I want to make one more attempt at jump tables (function pointers) but I want to start with the Tivaware utils/cmdline.c methodology.

Share this post


Link to post
Share on other sites

@@yyrkoon

You're getting mighty close to needing a state machine with that example ;-)

 

After all of this learning about jump tables, I think the importance of readibility is becoming a higher priority to me than the whizzy cool factor.

 

I haven't given up on trying to make this work.  This time, I want to make one more attempt at jump tables (function pointers) but I want to start with the Tivaware utils/cmdline.c methodology.

heh a state machine for my state machine eh ?

 

Well there are positives to using a command dispatcher. I named one above. But you could technically emulate that using functions normally. However with a command dispatcher you have the ability to take commands in any order - Dynamically. Which means a command dispatcher could be fed commands remotely. Over UART( serial ) for example. So in this way a remote machine could do all the heavy computations, and just pass the commands off to the board. It's actually a really cool "thing" to have. But it's not a one shoe fits all sort of deal. It's only meant really for one situation, the situation I just explained . . .

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


×
×
  • Create New...