Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCPI commands list in flash (PROGMEM) on AVR #60

Open
mvladic opened this issue Oct 15, 2015 · 10 comments
Open

SCPI commands list in flash (PROGMEM) on AVR #60

mvladic opened this issue Oct 15, 2015 · 10 comments

Comments

@mvladic
Copy link

mvladic commented Oct 15, 2015

How much of trouble is to support SCPI commands list in PROGMEM on AVR? Usually, there is not much SRAM on these microcontrollers (in my case 8K) and my SCPI commands list is taking a lots of it and soon it will became unsustainable :(

BTW We are using your SCPI parser on this hobby project:

http://www.eevblog.com/forum/projects/diy-programmable-dual-channel-bench-psu-0-50v3a/

Once more, thanks for your excellent parser!

@j123b567
Copy link
Owner

I know this limitation, but I don't know, how to handle this in pure c.

It is possible to have full table in PROGMEM and have RAM buffer for one item. Then while iterating over commad list, every command list item is coppied to RAM buffer and evaluated as usual. This can save some space, but I'm not able to do this.

If you can produce some working modification and way how to compile it and verify it (toolchain/ide/project), I will be happy.
I can then add some ifdefs and merge it with main line.

I can also add ifdef to int32_t tag; in struct _scpi_command_t in types.h, so it will save some more space, if you don't use it.

@j123b567
Copy link
Owner

Here are some more hints:

So findCommandHeader will not do just
cmd = &context->cmdlist[i];
but some "deep copy" of cmdlist item.
deep_copy_from_progmem(context->param_list.cmd, &context->cmdlist[i]);

context->param_list.cmd.pattern must be preallocated with RAM buffer

Type of context->param_list.cmd must be some new type to hold non constant pattern buffer.

Redefine some variables and fields to be in PROGMEM (struct _scpi_command_t.pattern, struct _scpi_t.cmdlist, and scpi_commands[] array itself)

I can't simulate it, so I will not do it blindfolded. If you give me some more hints, options for AVR-GCC, or so, I can do more.

@j123b567
Copy link
Owner

You can now change USE_COMMAND_TAGS to 0 to save some space, if you are not using tags in command list.

@mvladic
Copy link
Author

mvladic commented Oct 16, 2015

Thanks. I will try to do the rest by myself using your tips. I will post here about the progress.

@mvladic
Copy link
Author

mvladic commented Oct 16, 2015

I forked your project and created avr_progmem branch. By now, I created two commits:

  1. support for 64K progmem

mvladic@d6ef5b1

  1. arduino example for 64K progmem

mvladic@dfebea1

It is slightly different to support only first 64K of program memory compared to the support of all memory (for example, AVR Mega 2560 has 256K of program memory). This is because AVR is using 16-bit pointers. Also, there is some overhead when you work with the full memory. So, my idea is first to add support only for 64K progmem, and I did it with these commits.

To use first 64K of progmem for command list you must set USE_64K_PROGMEM_FOR_CMD_LIST to 1.

BTW I found and fixed small bug with SCPI_CMD_LIST_END definition that is introduced with USE_COMMAND_TAGS (check my first commit).

I tested the code on Arduino platform. You can download Arduino IDE and compile test-arduino example for yourself, but to be able to test it you will need one of the Arduino boards (I tested on Arduino Mega 2560).

To compile test-arduino under Arduino IDE you need to execute build-arduino-library.py first (more about this script below) than open test-arduino.ino sketch in Arduino IDE and press Verify button.

Unfortunately, there are some general quirks when you compile the code under Arduino IDE, especially when you use external libraries like scpi-parser. Because of that, I wrote the script called build-arduino-library.py to build Arduino compatible library from scpi-parser source code. It will build arduino library and copy it to the standard place for the arduino libraries (for example, on my Windows machine it is in C:\Users\Martin\Documents\Arduino\libraries) so Arduino sketches can use it. I'm running this script every time I change something in scpi-parser and in scpi_user_config.h located in test-arduino. There is no way you can set define variables in Arduino IDE so I need to place my define's in scpi_user_config.h and include it with the library. I also modify config.h on the fly when creating library for Arduino so that scpi_user_config.h is always included.

Here is screenshot of running the test-arduino with USE_64K_PROGMEM_FOR_CMD_LIST set to 1:

screenshot 2015-10-16 18 03 48

It is using 2164 bytes of dynamic memory.

And here is when USE_64K_PROGMEM_FOR_CMD_LIST is 0:

screenshot 2015-10-16 18 05 31

It is using 2706 bytes.

@j123b567
Copy link
Owner

Nice!
I will check it and play with it little bit.

@mvladic
Copy link
Author

mvladic commented Oct 19, 2015

I added support for the full program memory, here is the commit:

mvladic@533299d

To support, not only first 64K, but full program memory there are some (minor) complications.
Let me try to explain everything from the beginning.

TLDR; there is a question at the end :)

This is how SCPI commands are normally defined:

static const scpi_command_t scpi_commands[] = {
    {"MEASure:VOLTage:DC?", DMM_MeasureVoltageDcQ, 0},
    {"CONFigure:VOLTage:DC", DMM_ConfigureVoltageDc, 0},
    // ...
};

scpi_t scpi_context = {
    scpi_commands,
    // ...
};

If we want scpi_commands array to be inside program memory, we can just add PROGMEM in scpi_commands declaration:

static const scpi_command_t scpi_commands[] PROGMEM = {
    {"MEASure:VOLTage:DC?", DMM_MeasureVoltageDcQ, 0},
    {"CONFigure:VOLTage:DC", DMM_ConfigureVoltageDc, 0},
    // ...
};

scpi_t scpi_context = {
    scpi_commands,
    // ...
};

And this is how findCommandHeader() could be implemented:

static scpi_bool_t findCommandHeader(scpi_t * context, const char * header, int len) {
    int32_t i;
    const char * pattern;

    for (i = 0; (pattern = (const char *)pgm_read_word(&context->cmdlist[i].pattern)) != 0; ++i) {
        if (matchCommand(pattern, header, len, NULL, 0, 0)) {
            context->param_list.cmd_s.pattern = pattern; 
            context->param_list.cmd_s.callback = (scpi_command_callback_t)pgm_read_word(&context->cmdlist[i].callback);
            context->param_list.cmd_s.tag = (int32_t)pgm_read_dword(&context->cmdlist[i].tag);
            return TRUE;
        }
    }
    return FALSE;
}

(Field context->param_list.cmd_s is added as temporary storage in dynamic memory for the found command.)

But, command pattern strings ("MEASure:VOLTage:DC?" and "CONFigure:VOLTage:DC") will not be stored in PROGMEM. To store pattern strings in program memory we must add PROGMEM specification for those strings too:

static const char DMM_MeasureVoltageDcQ_pattern[] PROGMEM = "MEASure:VOLTage:DC?";
static const char DMM_ConfigureVoltageDc_pattern[] PROGMEM = "CONFigure:VOLTage:DC"; 

static const scpi_command_t scpi_commands[] PROGMEM = {
    {DMM_MeasureVoltageDcQ_pattern, DMM_MeasureVoltageDcQ, 0},
    {DMM_MeasureVoltageDcQ_pattern, DMM_ConfigureVoltageDc, 0},
    // ...
};

scpi_t scpi_context = {
    scpi_commands,
    // ...
};

And here is how findCommandHeader() is implemented for this case:

static scpi_bool_t findCommandHeader(scpi_t * context, const char * header, int len) {
    int32_t i;
    PGM_P pattern;

    for (i = 0; (pattern = (PGM_P)pgm_read_word(&context->cmdlist[i].pattern)) != 0; ++i) {
        strncpy_P(context->param_list.cmd_pattern_s, pattern, SCPI_MAX_CMD_PATTERN_SIZE);
        context->param_list.cmd_pattern_s[SCPI_MAX_CMD_PATTERN_SIZE] = '\0';

        if (matchCommand(context->param_list.cmd_pattern_s, header, len, NULL, 0, 0)) {
            context->param_list.cmd_s.callback = (scpi_command_callback_t)pgm_read_word(&context->cmdlist[i].callback);
            context->param_list.cmd_s.tag = (int32_t)pgm_read_dword(&context->cmdlist[i].tag);
            return TRUE;
        }
    }
    return FALSE;
}

(Field context->param_list.cmd_pattern_s is added as temporary storage in dynamic memory for the found command pattern.)

Now, both scpi_commands array and all the patterns strings are stored in progmem. But, pointers are 16-bit so you can only address first 64K.

There is a way to get 32-bit (actually 24-bit) address of variable stored in program memory by using function pgm_get_far_address(). But, this function can only be called in runtime, not in compile time, so it is not possible to build scpi_commands like above.

We can have a partial solution where scpi_commands array can be anywhere in the program memory, but pattern strings can only be inside first 64K (please read the code comments for the explanations):

// we need to change cmdlist field declaration in scpi_t
struct _scpi_t {
#if USE_FULL_PROGMEM_FOR_CMD_LIST         
    uint_farptr_t cmdlist;
#else
    const scpi_command_t * cmdlist;
#endif
    // ...
};

static const char DMM_MeasureVoltageDcQ_pattern[] PROGMEM = "MEASure:VOLTage:DC?";
static const char DMM_ConfigureVoltageDc_pattern[] PROGMEM = "CONFigure:VOLTage:DC"; 

static const scpi_command_t scpi_commands[] PROGMEM = {
    {DMM_MeasureVoltageDcQ_pattern, DMM_MeasureVoltageDcQ, 0},
    {DMM_MeasureVoltageDcQ_pattern, DMM_ConfigureVoltageDc, 0},
    // ...
};

scpi_t scpi_context = {
    0, // we can't get 24-bit address of scpi_commands during compilation
    // ...
};

int main() {
    // ...

    // we can call pgm_get_far_address only in the runtime inside same function,
    // here is called inside main function
    scpi_context.cmdlist = pgm_get_far_address(scpi_commands);

    // ...
}

And here is how findCommandHeader() could be implemented for this case:

static scpi_bool_t findCommandHeader(scpi_t * context, const char * header, int len) {
    int32_t i;
    uint_farptr_t p_cmd = context->cmdlist;
    PGM_P pattern;

    for (i = 0;
         (pattern = (PGM_P)pgm_read_word_far(p_cmd + offsetof(scpi_command_t, pattern))) != 0;
         ++i, p_cmd += sizeof(scpi_command_t))
    {
        strncpy_P(context->param_list.cmd_pattern_s, pattern, SCPI_MAX_CMD_PATTERN_SIZE);
        context->param_list.cmd_pattern_s[SCPI_MAX_CMD_PATTERN_SIZE] = '\0';

        if (matchCommand(context->param_list.cmd_pattern_s, header, len, NULL, 0, 0)) {
            context->param_list.cmd_s.callback = (scpi_command_callback_t)pgm_read_word_far(p_cmd + offsetof(scpi_command_t, callback));
            context->param_list.cmd_s.tag = (int32_t)pgm_read_dword_far(p_cmd + offsetof(scpi_command_t, tag));
            return TRUE;
        }
    }
    return FALSE;
}

To put everything anywhere in program memory space we need to do something like this (again, please read the code comments for the explanations):

// we need to add a new field in scpi_t
struct _scpi_t {
#if USE_FULL_PROGMEM_FOR_CMD_LIST         
    uint_farptr_t cmdlist;
    uint_farptr_t cmdpatterns;
#else
    const scpi_command_t * cmdlist;
#endif
    // ...
};

// define all command pattern strings as single string like this:  
static const char scpi_command_patterns[] PROGMEM =
    "MEASure:VOLTage:DC?"
    "CONFigure:VOLTage:DC";

// in scpi_commands array, pattern field of scpi_command_t is used to store
// not the pattern itself but the size of pattern string  
static const scpi_command_t scpi_commands[] PROGMEM = {
    {(const char *)(sizeof("MEASure:VOLTage:DC?") - 1), DMM_MeasureVoltageDcQ},
    {(const char *)(sizeof("CONFigure:VOLTage:DC") - 1), DMM_ConfigureVoltageDc},
    // ...
    SCPI_CMD_LIST_END
};

scpi_t scpi_context = {
    0, // we can't get 24-bit address of scpi_commands during compilation
    // ...
};

int main() {
    // ...

    // in runtime we can set cmdlist and cmdpatterns far addresses:  
    scpi_context.cmdlist = pgm_get_far_address(scpi_commands);
    scpi_context.cmdpatterns = pgm_get_far_address(scpi_command_patterns);

    // ...
}

And here is the implementation findCommandHeader() for this case:

static scpi_bool_t findCommandHeader(scpi_t * context, const char * header, int len) {
    int32_t i;

    uint_farptr_t p_cmd = context->cmdlist;
    uint_farptr_t p_pattern = context->cmdpatterns;
    uint16_t pattern_length;

    for (i = 0;
        (pattern_length = pgm_read_word_far(p_cmd + offsetof(scpi_command_t, pattern))) != 0;
        ++i, p_cmd += sizeof(scpi_command_t), p_pattern += pattern_length)
    {
        strncpy_PF(context->param_list.cmd_pattern_s, p_pattern, pattern_length);
        context->param_list.cmd_pattern_s[pattern_length] = '\0';

        if (matchCommand(context->param_list.cmd_pattern_s, header, len, NULL, 0, 0)) {
            context->param_list.cmd_s.callback = (scpi_command_callback_t)pgm_read_word_far(p_cmd + offsetof(scpi_command_t, callback));
            context->param_list.cmd_s.tag = (int32_t)pgm_read_dword_far(p_cmd + offsetof(scpi_command_t, tag));
            return TRUE;
        }
    }
    return FALSE;
}   

Now, the question is do you want to add all these changes in the scpi parser? I want blame you if you find it too specific (or even ugly) to add it.

Another possible solution, which will solve my problem, is to give a user of the library a way to implemenent his own version of findCommandHeader() function. This could be done like this:

struct _scpi_interface_t {
    // ...
    scpi_bool_t (*findCommandHeader)(scpi_t * context, const char * header, int len);
};

static scpi_bool_t findCommandHeader(scpi_t * context, const char * header, int len) {
    if (context->interface->findCommandHeader) {
        return context->interface->findCommandHeader(context, header, len);
    }       
    // ...
}

Of course, matchCommand() function must be part of documented API, so user implementation of findCommandHeader() can use it.

With this solution, you could still include test-arduino in the scpi parser examples with the custom implementation of findCommandHeader() which works with command list stored in program memory.

@j123b567
Copy link
Owner

Wow, it is very interesting, but I have currently not much time to incorporate so big change. I have just few things.

Are there any drawbacks to leave it under 64k? Or the linker is not intelligent much and can place progmem section over this boundary?

Is it possible to add PROGMEM keyword to type deffinition? So you can do somethink like this?

    struct _scpi_command_progmem_t {
        const PROGMEM char * pattern;
        scpi_command_callback_t callback;
#if USE_COMMAND_TAGS
        int32_t tag;
#endif /* USE_COMMAND_TAGS */
    };
    typedef struct _scpi_command_progmem_t PROGMEM  scpi_command_progmem_t;

So you can leave command list as it was in original source but with scpi_command_progmem_t type?

I don't like dividing command list to two more arrays, because this will cause big problems. If this is really needed, you can read somethink about x-macro https://en.wikipedia.org/wiki/X_Macro and automate generation of all lists so it is not possible to make a mistake.

I'm using X-macro in this file libscpi/inc/scpi/error.h, after that, enum is generated using one macro and SCPI_ErrorTranslate is generated using second macro in error.c, but there is only one LIST_OF_ERRORS.

@mvladic
Copy link
Author

mvladic commented Oct 19, 2015

Are there any drawbacks to leave it under 64k? Or the linker is not intelligent much and can place progmem section over this boundary?

Linker will use program memory under 64k until we full this region. If our static data is bigger than 64K then it will use the memory above 64K (and pointers will overflow without warning). In my case, I need to store lots of static data like fonts, bitmaps, etc... so it is possible that at the end I will need more than 64K. Maybe I could put SCPI commands definition under 64K and other data above (still, I need to learn how to do that).

Is it possible to add PROGMEM keyword to type deffinition?

No, this is not possible. You can only place global const variables in PROGMEM section and you need to initialize those variables at the compile time.

I don't like dividing command list to two more arrays, because this will cause big problems. If this is really needed, you can read somethink about x-macro https://en.wikipedia.org/wiki/X_Macro and automate generation of all lists so it is not possible to make a mistake.

Actually, in my example test-arduino I was using something like x-macro (although I didn't know it is called x-macro). If you take a look on this file:

https://github.com/mvladic/scpi-parser/blob/avr_progmem/examples/test-arduino/scpi-def.cpp.

you will find the following (for the sake of brevity I removed some parts):

#define SCPI_COMMANDS \
    SCPI_COMMAND("*CLS", SCPI_CoreCls) \
    SCPI_COMMAND("*ESE", SCPI_CoreEse) \
    SCPI_COMMAND("*ESE?", SCPI_CoreEseQ) \
    SCPI_COMMAND("*ESR?", SCPI_CoreEsrQ) \
    ...

#define SCPI_COMMAND(P, C) static const char C ## _pattern[] PROGMEM = P;
SCPI_COMMANDS
#define SCPI_COMMAND(P, C) {C ## _pattern, C},
static const scpi_command_t scpi_commands[] PROGMEM = {
    SCPI_COMMANDS
    SCPI_CMD_LIST_END
};

So, you only need to define one array. (I forgot to #undef after first SCPI_COMMAND definition so you will get a warning).

I just learned that you actually don't need this trick. You can declare commands array like before by using PSTR macro:

static const scpi_command_t scpi_commands[] PROGMEM = {
    {PSTR("MEASure:VOLTage:DC?"), DMM_MeasureVoltageDcQ, 0},
    {PSTR("CONFigure:VOLTage:DC"), DMM_ConfigureVoltageDc, 0},
    // ...
};

Now, both scpi_commands array and command patterns will be stored in program memory.

Still, for the full memory access we will need x-macro.

I'm using X-macro in this file libscpi/inc/scpi/error.h, after that, enum is generated using one macro and SCPI_ErrorTranslate is generated using second macro in error.c, but there is only one LIST_OF_ERRORS.

BTW I would like to store error messages in program memory also ;) What do you think of idea that user could provide implementation of SCPI_ErrorTranslate() using scpi_interface_t like I proposed for findCommandHeader() in previous post?

@j123b567
Copy link
Owner

Sorry, I didn't look into the source. I just read the comment. So it is much nicer with this.

I have played little bit with Arduino IDE and it seems, that your previous solution is the only one saving some RAM.

SCPI_ErrorTranslate should be much easier to convert but still needs intermediate RAM buffer for one string.

So I will think about some API for ErrorTranslate and findCommandHeader so we can put them into separate file specific for AVR and keep rest of sources clean.

@j123b567 j123b567 added ready and removed ready labels Feb 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants