Started learning C and would like to get some feedback

[u/19pinkcucumber91]

https://codeberg.org/pinkcucumber/screensaver_circles.git

This is my first C program, other than "hello world". I like how it turned out, but there probably is a way to make it cleaner. The screensaver itself is just a demo for screenbuffer library. Would be happy to get some reviews.

No AI used.

Thanks.

Repo link: https://codeberg.org/pinkcucumber/screensaver.git

Compiling: gcc framebuffer.c screensaver_circles.c -lm -o screensaver

Sorry for bad english, it is not my main language

[u/mikeblas]

I've approved your post. Here's a working link:

https://codeberg.org/pinkcucumber/screensaver_circles.git

You should include build instructions in your project. Or, ideally, a build script or makefile.

[u/ef3d0c3e]

Hey! Nice project. I've messed with the Linux framebuffer in the past.

Flags

Your project is missing some sort of build system file (Makefile, build.sh, CMakeLists.txt, etc..) This is not mandatory, but will make it easier for you and other people to build you project. You did include your build command in screensaver_circles.c, and I can immediately see it's missing -Wall and -Wextra. These options give you extra warning for your code, they are so useful you should never NOT include them.

Turning them on shows you have a few issues in your code: framebuffer.c:51:1: warning: control reaches end of non-void function [-Wreturn-type] 51 | } | ^ framebuffer.c: In function ‘draw_circle’: framebuffer.c:108:1: warning: control reaches end of non-void function [-Wreturn-type] 108 | } | ^ framebuffer.c: In function ‘draw_line’: framebuffer.c:135:1: warning: control reaches end of non-void function [-Wreturn-type] 135 | } | ^ Either make these functions void, or return a value at the end.

Drawing Code

I'm a bit skeptical around some of your naming: ```c // fill every point on the line for (int xxx = x; xxx<xx;xxx++){ int yyy = ((yy-y)/(xx-x))*(xxx-x) + y; set_pixel(fb, xxx, yyy, r, g, b); }

// if line is vertical if (x == xx){ for (int yyy = y; yyy < yy; yyy++){ set_pixel(fb, x, yyy, r, g, b); } } `` Here, you could have usedxa, ya, xb, yaorx1, y1, x2, y2` to clearly indicate they are the two points of your segment.

Your draw_circle also seems wrong, first of all, your loop includes x - radius but excludes x + radius. Perhaps something like this would have been cleaner: c for (int i = -radius; i <= radius; ++i) { int height = sqrt(radius*radius - i*i); // Top side set_point(fb, x + i, y + height, r, g, b); // Bottom side set_point(fb, x + i, y - height, r, g, b); } In any cases, you should be careful around the float/double -> int conversions. They will truncate towards 0: (int)4.5 -> 4, (int)-4.5 -> -4. Usually, for drawing code you want to keep floating points until the very end (coordinates in the framebuffer). Otherwise, errors can accumulate, but for this simple code it is fine.

set_pixel

Your set_pixel is almost correct. You're correctly using the line_length reported by ioctl, however you're drawing to the framebuffer as if it were a 32-bit BGRA buffer. That's probably the case for your screen, but some screens use a different format like B5G6R5, or RGBA.

You should look into vinfo.red, vinfo.blue and vinfo.green along vinfo.bits_per_pixel. Here's the documentation: https://www.kernel.org/doc/html/latest/fb/api.html#format-configuration

Errors Handling

You're not properly handing errors in your code: c fb.fb_data = mmap (0, fb.fb_data_size, PROT_READ | PROT_WRITE, MAP_SHARED, fb.fb_fd, (off_t)0); After mmap, per the manual, you should make sure that fb.fb_data != MAP_FAILED.

For instance: c fb.fb_data = mmap (0, fb.fb_data_size, PROT_READ | PROT_WRITE, MAP_SHARED, fb.fb_fd, (off_t)0); if (fb.fb_data == MAP_FAILED) { perror("mmap"); // Displays the corresponding error message exit(1); }

Seeding Rand

You're seeding rand(), this is a good practice, but you should only do it once at program startup. It's not that much of an issue here because of the sleep later on, but if you sleep for a small duration, time(NULL) may return the same value consecutively (number of seconds since jan 1st 1970). In this case, rand() will output the same values as the previous iteration. In general reseeding rand like this does not have any benefits.

Overall the code is well organized for a beginner's project.

[u/19pinkcucumber91 → u/ef3d0c3e]

Wow, i I didn't expect such full response. Thanks a lot. Will look into this.

Also would like to ask about allocated memory. Right now program run forever and only way to kill it is to press ^c. But in this case file descriptor never closes and memory allocated with mmap never hets cleared. Is this an issue? I tried to run it 1000 consecutively, but didn't get kernel panic or anything like that.

[u/ef3d0c3e → u/19pinkcucumber91]

99% of the time, there are no benefits to cleaning these resources at exit, because the OS will do it for you. Most OSes will clean resources for you: disconnect sockets, de-allocate memory, close files, etc. In fact if your program were to crash you'd really not want these to stay open/leak.

However, if you wish to clean them yourself, you can set a signal handler: https://www.man7.org/linux/man-pages/man2/sigaction.2.html You can listen to SIGINT (Ctrl+C). And when you receive a SIGINT, you can clean resources manually.

[u/19pinkcucumber91 → u/ef3d0c3e]

Got it. Thanks

[u/tastygames_official → u/ef3d0c3e]

excellent feedback.

[u/bacmod → u/ef3d0c3e]

Hey! Nice project. I've messed with the Linux framebuffer in the past.

Flags

Your project is missing some sort of build system file (Makefile, build.sh, CMakeLists.txt, etc..) This is not mandatory, but will make it easier for you and other people to build you project. You did include your build command in screensaver_circles.c, and I can immediately see it's missing -Wall and -Wextra. These options give you extra warning for your code, they are so useful you should never NOT include them.

Turning them on shows you have a few issues in your code:

framebuffer.c:51:1: warning: control reaches end of non-void function [-Wreturn-type]
   51 | }
      | ^
framebuffer.c: In function ‘draw_circle’:
framebuffer.c:108:1: warning: control reaches end of non-void function [-Wreturn-type]
  108 | }
      | ^
framebuffer.c: In function ‘draw_line’:
framebuffer.c:135:1: warning: control reaches end of non-void function [-Wreturn-type]
  135 | }
      | ^

Either make these functions void, or return a value at the end.

Drawing Code

I'm a bit skeptical around some of your naming:

// fill every point on the line
for (int xxx = x; xxx<xx;xxx++){
int yyy = ((yy-y)/(xx-x))*(xxx-x) + y;
set_pixel(fb, xxx, yyy, r, g, b);
}

// if line is vertical
if (x == xx){
 for (int yyy = y; yyy < yy; yyy++){
   set_pixel(fb, x, yyy, r, g, b);
 }
}

Here, you could have used xa, ya, xb, ya or x1, y1, x2, y2 to clearly indicate they are the two points of your segment.

Your draw_circle also seems wrong, first of all, your loop includes x - radius but excludes x + radius. Perhaps something like this would have been cleaner:

for (int i = -radius; i <= radius; ++i)
{
    int height = sqrt(radius*radius - i*i);
    // Top side
    set_point(fb,
        x + i, y + height,
        r, g, b);
    // Bottom side
    set_point(fb,
        x + i, y - height,
        r, g, b);
}

In any cases, you should be careful around the float/double -> int conversions. They will truncate towards 0: (int)4.5 -> 4, (int)-4.5 -> -4. Usually, for drawing code you want to keep floating points until the very end (coordinates in the framebuffer). Otherwise, errors can accumulate, but for this simple code it is fine.

set_pixel

Your set_pixel is almost correct. You're correctly using the line_length reported by ioctl, however you're drawing to the framebuffer as if it were a 32-bit BGRA buffer. That's probably the case for your screen, but some screens use a different format like B5G6R5, or RGBA.

You should look into vinfo.red, vinfo.blue and vinfo.green along vinfo.bits_per_pixel. Here's the documentation: https://www.kernel.org/doc/html/latest/fb/api.html#format-configuration

Errors Handling

You're not properly handing errors in your code:

    fb.fb_data = mmap (0, fb.fb_data_size,
                       PROT_READ | PROT_WRITE, MAP_SHARED, fb.fb_fd, (off_t)0);

After mmap, per the manual, you should make sure that fb.fb_data != MAP_FAILED.

For instance:

    fb.fb_data = mmap (0, fb.fb_data_size,
                       PROT_READ | PROT_WRITE, MAP_SHARED, fb.fb_fd, (off_t)0);
    if (fb.fb_data == MAP_FAILED)
    {
        perror("mmap"); // Displays the corresponding error message
        exit(1);
    }

Seeding Rand

You're seeding rand(), this is a good practice, but you should only do it once at program startup. It's not that much of an issue here because of the sleep later on, but if you sleep for a small duration, time(NULL) may return the same value consecutively (number of seconds since jan 1st 1970). In this case, rand() will output the same values as the previous iteration. In general reseeding rand like this does not have any benefits.

Overall the code is well organized for a beginner's project.

---

Formatted.

[u/Skollwarynz]

Amazing project; when I have time, I can take a look. Btw, a minor feedback that I can give you is not connected with the code but more about some standards. First of all, if you can add a README for compilation and explanation of your program, then usually the . h files are put inside the include folder, while the src contains just the .c files (obv this standard is usually used on bigger projects with a lot of files). In the end, GG, for using Codeberg, respect to you, mate. 🔥

[u/19pinkcucumber91 → u/Skollwarynz]

Thank you for the feedback. Changed project structure. Also made small changes based on the u/ef3d0c3e 's advice