Why provide void execute() with non-const char *cmd?

[ondras12345]

Currently, there are several overloaded execute() methods.

Commander-API/src/Commander-API.hpp

Lines 171 to 185 in 9d185dc

	/// Default execution function.
	///
	/// This function tries to execute a command.
	/// It uses the default response channel, so
	/// the messages from the command handler wont
	/// be visible.
	void execute( char *cmd );
	/// Default execution function.
	///
	/// This function tries to execute a command.
	/// It uses the default response channel, so
	/// the messages from the command handler wont
	/// be visible.
	void execute( const char *cmd );

Looking at the implementation, it seems like the const and non-const versions do essentially the same thing, except that the const version casts cmnd to char * before calling executeCommand. This seems rather unsafe, because if executeCommand was to modify its argument, which was actually a const char * (e.g. a string literal), it would result in undefined behavior.

Commander-API/src/Commander-API.cpp

Lines 524 to 542 in 9d185dc

	void Commander::execute( char *cmd ){
	// Default execute handler, so the default response will be chosen.
	response = &defaultResponse;
	// Execute the command.
	executeCommand( cmd );
	}
	void Commander::execute( const char *cmd ){
	// Default execute handler, so the default response will be chosen.
	response = &defaultResponse;
	// Execute the command.
	executeCommand( (char*)cmd );
	}

I might very well be missing something, but it seems to me like it would be cleaner to declare executeCommand with const char * cmd, remove the cast and all execute methods with non-const cmd.
executeCommand copies the contents of cmd to its internal buffer regardless of which version of execute was called.

Commander-API/src/Commander-API.cpp

Lines 295 to 318 in 9d185dc

	void Commander::executeCommand( char *cmd, void* parent ){
	// The beginning of the argument list will be stored in this pointer
	char *arg;
	// This variable tracks the command name length
	uint32_t cmd_name_cntr;
	// If this flag is set, than the description message will be printed,
	// and the commands function won't be called.
	uint8_t show_description = 0;
	// Pointer to the selected command data.
	API_t *commandData_ptr;
	int32_t pipePos;
	uint32_t i;
	// Copy the command data to the internal buffer.
	// It is necessary because we have to modify the content
	// of it. If it is points to a const char array we will
	// get a bus-fault error without a buffer.
	strncpy( tempBuff, cmd, COMMANDER_MAX_COMMAND_SIZE );

Was this a deliberate design decision? If so, what was the reasoning behind it? It is because you intend to have a version of executeCommand that does not strncpy in a future revision?

[dani007200964]

Huhh it is a complicated question.

1. The overloaded versions were there because I hate casting char* to const char* and back. I know I know, I will burn in hell... I can accept that creating a prototype for const and non-const versions is unnecessary.
2. The internal buffer and the strncpy part is necessary to work with const data. As you mentioned, the way it is working right now is to modify the data in the input string. It can not be done if it is constant. But this way the memory consumption is more-less computable at compile time. The disadvantage is that this way a buffer is required. From the algorithm point of view, it is simpler to handle the pipes and the arguments like this. This library is meant to work on low-power systems as well, so allocating large buffers runtime is not an option.

Thank you for your feedback, I think it is a good idea to do some housekeeping on overloaded versions of the execute function.
If I understand correctly, you suggest using these variants somehow:

// Required to use it without response. The idea was to make it possible to run this from some kind of
// pre-defined script. For example, a simple txt file from an SD card can contain some pre-defined commands.
// In this case, you don't have any output.
void execute( const char *cmd );

// This is the simplest version with a response channel. This way the user could print some data back,
// or even interact with the Stram object.
void execute( const char *cmd, Stream *resp );

// This is a tricky one. Shellminator and this library always developed together. The current release has a big flaw.
// It can not interact with the caller terminal. It is a problem when you have multiple terminal objects.
// You have to have a reference to the caller terminal somehow. This is why the parent argument is implemented.
// If you want to use this library without Shellminator, you can ignore the parent variable.
// I know it is not the safest thing to cast a pointer from a Shellminator object to void*, but I do not have a better idea
// right now without serious linker magic. It has to be Arduino-compatible.
void execute( const char *cmd, Stream *resp, void* parent );

From the user's point of view, if you want to use it with a non-constant type this would be the correct method:

char* cmd = (char*)"help -d";
commander.execute( (const char*)cmd );

Am I understanding right your idea?

[ondras12345]

Thank you for your answer.

Am I understanding right your idea?

Yes.
I would also change the prototype of executeCommand:

void executeCommand( const char *cmd, void* parent = NULL );

That would allow you to get rid of that (char *) cast in execute(). It also seems more correct as executeCommand will never modify the contents of cmd.

From the user's point of view, if you want to use it with a non-constant type

I believe the compiler can cast char * to const char * automatically:

char* cmd = (char*)"help -d";  // not really a good idea, just an example
commander.execute( cmd );  // no need to explicitly cast here