My code is ugly

[Jester]

Often I can get something to work, but I know a real programmer could simplify my solution to a fraction of the size and complexity.

Hardware:  embedded Arduino application running on CortexM0 (MKRZero)

I have a structure and within that structure there is a string and somewhere within that string there is a  'F' followed by either 1, 2 or 3 digits for example F44, I need to extract the 44 as either an int or float.

This works but it sure seems clumsy,  suggestions welcome.

Code: [Select]

     

    struct mData{
      char data;
      int space;
    };

    mData mdiData[40];

// ..... at this point mdiData.data is populated with data including the Fxx portion and is null terminated after the Fxx numeric value.


           fLoc = 0;
           char fs[10];
           for (int j = 0; j < 39; j++) {                                                     
                    if (mdiData[j].data == 'F')
                           fLoc = j;
                                           
                     if (j > fLoc+3) {
                                fs[0] = mdiData[fLoc+1].data;
                                fs[1] = mdiData[fLoc+2].data;
                                fs[2] = mdiData[fLoc+3].data;
                                fs[3] = '\0';
                      }
               }
               feedSpeed = atof(fs);


I tried using strchr() to find the location of 'F' but I don't know how to get that to work when using a structure, I can make it work if the string is in a simple char[] array.

Thanks to anyone that can offer a more elegant solution.

[T3sl4co1l]

Can you not rearrange it to {char[] data[40], int[] space[40]} and then iterate over the string?

strchr will not work because of the alternating chars and ints (which incidentally won't pack tightly either, you lose 3/8ths of the memory there).  A random int value could contain a 'F' and create a false positive.

Tim

[pqass]

If the last four mdiData array elements are F123, then it won't be processed. 
Your code requires a NULL or dummy last element (" if (j > fLoc+3) ...").
Also, are there any characters to skip between F123 and the next F456?
And which (first or last) to keep?

This is my (untested) take:

Code: [Select]

#define NELEMS(x)  (sizeof(x) / sizeof((x)[0]))

float feedSpeed = 0.0;
for (int j = 0; j < NELEMS(mdiData); j++) {
if (mdiData[j].data == 'F') {
            j++;
for (feedSpeed = 0.0; j < NELEMS(mdiData) && isdigit(mdiData[j].data); j++) {
feedSpeed = (feedSpeed * 10.0) + (mdiData[j].data - '0');
            }
            j--;
}
}

feedSpeed is set to the last F456 (0 to n digits; n is more than you need) found.

EDIT: replace "j--;" with "break;" to exit on first occurrence.

The real question is why do you need an array of structures with just a one character (.data) and an int (.space)?

[Jester]

There will only be one Fxxx in the string.

The string gets sent to a small display, in order to display the complete string in the limited space, I cheat by using smaller spaces between terms. The string can be manipulated by the user by selecting the desired character with cursor keys, backspace etc, so I need to keep track of the width used for each character/space in the structure.

Thanks for your elegant solution, I need to read about NELEMS.

[pqass]

My solution involves two floating-point ops per digit read which may be time-costly on a 8-bit MCU.  But at least there are fewer iteration, index, and temp vars to manage (and debug).

sizeof returns bytes allocated to the whole type/var x so dividing by the bytes used by the first element, x[0], returns number of elements.  No need to create a #define for every array; can just drop in a one-time number at the array definition.  It's portable, type independent, and no overhead as the value gets resolved at compile-time.

[magic]

I have a structure and within that structure there is a string and somewhere within that string there is a  'F' followed by either 1, 2 or 3 digits for example F44, I need to extract the 44 as either an int or float.

Either int or float?
This would suggest that the number is always integer, so why bother with float at all?

[shabaz]

I'm guessing the use-case is G-code.. if so, it could be better to write more general code to tokenize the lines and have the capability to parse all the codes, rather than try to handle one at a time. Also, it could be worth coding on Linux first, so you can more easily test against a load of files from different sources, to try to see if it's rock solid (e.g. some files may be using different carriage return/linefeed maybe? I've never looked at G-code apart from just vaguely.

[Siwastaja]

First naming, data is usually a lazy and poor name but here it's extra confusing because data is kinda plural, usually it would refer to some kind of buffer or larger chunk of something, or something abstract. What you have is a single character so why not: char character or symbol or whatever.

Then about performance; memory probably matters in an embedded application. On Cortex-M0 int is 32 bits and your structure as it is requires 3 bytes of padding, so it's 8 bytes per element. I would guess your space is 0-255 so use uint8_t for it, and you are down to 2 bytes per element and no padding.

Then more importantly, it all starts with getting the memory structures right. You have chosen interleaved form of character-space-character-space which prevents you from using every normal string handling library function, and even if you roll your own, they would be more complicated than the usual ones. Why not use planar form:

struct
{
    char string[40];
    uint8_t spaces[40];
}