0
\$\begingroup\$

I have a 2D array of fixed size and I've implemented a FIFO function. So, the array will always be full before it reaches the FIFO function, then the FIFO will remove the oldest array, move all of the arrays up by one and then add the newest array to the other end. Basically a queue. Now, this code will actually be running on a microcontroller, an array will come from an ADC and then I store the ADC array in a 2D array. To design the FIFO algorithm in simple C first, I have written a program that generates random numbers and then stores them into a 2D buffer. The program then asks the user what new values should be stored, user puts in the latest array of values and then the program removes the oldest array, shuffles all the remaining arrays up by one and then the latest user numbers get added to the end. The program does this 10 times. This is designed to simulate what is happening in the microcontroller code. The 2D array is already full with values and then the ADC adds the latest set of values to the array, and the oldest set are forgotten.

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
int fixed_number[5] = { 0 };
uint16_t big_array[10][5] = { 0 };
uint16_t big_array_copy[10][5] = { 0 };
uint16_t small_array_1[5] = { 0 };
uint16_t k[1] = { 0 };
uint8_t count = 0;
int main()
{
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array[j][i] = rand() % 255;
        }
    }
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            big_array_copy[j][i] = big_array[j][i];
        }
    }
    printf("These are the original numbers in the big array:\n");
    for (int j = 0; j < 10; j++)
    {
        for (int i = 0; i < 5; i++)
        {
            k[0] = big_array[j][i];
            printf("%d,", k[0]);
                    }
        printf("\n");
            }
    while (count < 10)
    {
        printf("Please enter values for queue:\n");
        for (int i = 0; i < 5; i++)
        {
            scanf_s("%d", &fixed_number[i]);
        }
           printf("\n\n");
        printf("Array values have been swapped:\n ");
        for (int k = 0; k < 5; k++)
        {
            big_array_copy[10 - 1][k] = 0;
        }
                for (int j = 9; j > 0; j--)
        {

            for (int i = 0; i < 5; i++)
            {
                //small_array_1[i] = big_array[j + 1][i];
                big_array_copy[j][i] = big_array[j - 1][i];
            }
        }
        for (int i = 0; i < 5; i++)
        {
            big_array_copy[0][i] = fixed_number[i];
        }
        for (int j = 0; j < 10; j++)
        {
            for (int i = 0; i < 5; i++)
            {
                k[0] = big_array_copy[j][i];
                printf("%d,", k[0]);
            }
            printf("\n");
        }
        for (int j = 0; j < 10; j++)
        {
            for (int i = 0; i < 5; i++)
            {
                big_array[j][i] = big_array_copy[j][i];
            }
        }
        count++;
    }
}

Now this code works perfectly fine, but there is a lot of loops and it also requires twice the memory space as the buffer, big_array[][] must be copied in to big_array_copy[][]. In the real application, the buffer is uint16_t somebuffer[100][45] which is 4.5kb. I've got 512kb of SRAM to use, but I'd rather use as little as I can.

The above method can be implemented on my target platform (STM32F767 for those wondering) but is there a more simple method to implementing the above, that uses fairly portable c with no special functions, that could reduce the memory required and the processing time required? thanks

\$\endgroup\$
9
  • 1
    \$\begingroup\$ Welcome to Code Review, I added the c language tag to your question. I am a lot rusty but you are using iostream in a c program ? \$\endgroup\$ – dariosicily Feb 23 at 15:51
  • 1
    \$\begingroup\$ C doesn't have <iostream>, but it appears that include can be safely removed to make this a real C program. \$\endgroup\$ – Toby Speight Feb 23 at 17:09
  • \$\begingroup\$ As I wrote this code in Visual Studio, it added <iostream> by default and I didn't notice. I'll remove it from the code just for clarification. \$\endgroup\$ – ChrisD91 Feb 23 at 17:18
  • \$\begingroup\$ When you say "In my real code" like that, it suggests that this isn't what you're actually using. Here at Code Review, we expect to see finished production code, not prototypes! \$\endgroup\$ – Toby Speight Feb 23 at 17:19
  • 2
    \$\begingroup\$ Please do not change the code after an answer has been written, everyone needs to be able to see the code that the reviewed saw. Please read What should I do when someone answers my question?. \$\endgroup\$ – pacmaninbw Feb 23 at 17:26
1
\$\begingroup\$

We're missing an include of <stdint.h>. On the other hand, we include <time.h> which we're not using.

The code doesn't compile here because of the use of Annex K scanf_s() without checking for its availability. There's no reason not to use ordinary scanf() here (but do examine its return value - that's important!)

There are a lot of magic constants sprinkled throughout the code, with no indication whether they relate to each other. That makes this an unmaintainable program, because you can't change any of those individually without understanding the whole program.

Also, with all the functionality thrown together in main(), that makes it harder to understand the purpose of each part of the code. Please divide the code into well-named functions (probably with static linkage).

I recommend fixing those before posting another review request to look at this in any depth.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.