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).

EDIT: I made feedSpeed a float because you used atof() but does it have to be?  int will be much faster (remove all occurrences of ".0").

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];
}

[Nominal Animal]

Consider the following example:

Code: [Select]

#include <stdint.h>
#include <stddef.h>

// Look for 'F' followed by one to three decimal digits within the data buffer.
// If found, returns the decimal value (0 - 999); otherwise returns -1.
// If the buffer length never exceeds 255, use 'const uint8_t len' instead.
int_fast16_t  find_feed_rate(const char *data, const size_t len) {
    // We need at least two characters of data.
    if (len < 2)
        return -1;

    // Pointer to last character in the buffer.
    const char *ends = data + len - 1;
    while (data < ends) {
        if (*(data++) == 'F') {
            if (*data >= '0' && *data <= '9') {
                // We have a match, with result being the first decimal digit value.
                int_fast16_t  result = *(data++) - '0';
                // If there is a second digit, append it to result.
                if (data <= ends && *data >= '0' && *data <= '9') {
                    result = 10 * result + *(data++) - '0';
                    // If there is a third digit, append it to result.
                    if (data <= ends && *data >= '0' && *data <= '9') {
                        result = 10 * result + *(data++) - '0';
                    }
                }
                // Return the one to three decimal digit number discovered (0 .. 999).
                return result;
            }
        }
    }

    return -1;
}If you have a continuous buffer, say struct Siwastaja mdiData, then you'd use
    int_fast16_t  rate = find_feed_rate(mdiData.string, sizeof mdiData.string);
    if (rate >= 0) {
        // Use feedrate 'rate'
    }

If you absolutely need to have the string characters interspersed with other stuff, i.e.
    static struct {
        char    character;
        uint8_t space;
    } mdiData[40];
then the function body to find the data rate would be

Code: [Select]

    // This will be set to 0 .. 999, if Fddd found in mdiData.
    int_fast16_t  rate = -1;

    // Number of elements in mdiData array, less one.
    const uint8_t n = sizeof mdiData / sizeof mdiData[0] - 1;

    // Loop over all except the last character in mdiData.
    for (uint8_t i = 0; i < n; i++) {
        if (mdiData[i].character == 'F' &&
            mdiData[i+1].character >= '0' && mdiData[i+1].character <= '9') {
                rate = mdiData[i+1].character - '0';
                i += 2;
                if (i <= n && mdiData[i].character >= '0' && mdiData[i].character <= '9') {
                    rate = 10 * rate + mdiData[i].character - '0';
                    i++;
                    if (i <= n && mdiData[i].character >= '0' && mdiData[i].character <= '9') {
                        rate = 10 * rate + mdiData[i].character - '0';
                    }
                }
                break;
            }
    }

    if (rate > 0) {
        // Set feed rate to 'rate'.
    }
}

In both cases above, the logic is the same:

• Start by initializing rate to an impossible value, for example -1.
  (In the first case, this is implicit, because rate is only initialized when a match is found.  However, the function does return -1 as if rate was initialized to that value, when no match is found.)
   
• Loop over all possible positions for 'F', i.e. all except the final character in the buffer.
  If the current character is 'F' and the following character is a decimal digit, we have found a match:
   
  • Save the decimal digit as a decimal number in rate (which needs to be at least ten bits in size).
    Advance by two characters (the 'F' and the decimal digit).
     
  • If still within the array, and the character at hand is a decimal digit, multiply rate by ten, and add the value of the decimal digit to rate.
    Advance by one character.
     
  • If still within the array, and the character at hand is a decimal digit, multiply rate by ten, and add the value of the decimal digit to rate.
     
  Return, or break out of the loop, with rate now containing the feed rate as a decimal nonnegative value (0 .. 999, inclusive).