8
\$\begingroup\$

Problem

I've written a timer module in C for an SDL game. I'd like to get some eyeballs on. I wrote this for 2 requirements: I needed a timer that would signal on an elapsed interval, AND that was pausable and restartable. The timers in SDL signal a callback on a set interval, but they aren't pausable. I've seen other pausable timer implementations for SDL, but they've all required a query for a tick count.

I realized that since I already had an infinite loop using the SDL engine, I could leverage that to drive a pausable timer. I've included a small test program that you can use to evaluate the timer module if you want.

BE ADVISED: If you are sensitive to flashing visual stimuli, you shouldn't run the test program.

Also, the test program is NOT the code that I need reviewed.

The timer module works well, and the caveats I'm aware of already are:

  • The more simultaneous timers you use, the greater the likelihood that you will run into timer lag.
  • The work done in timer callbacks should be short and sweet, and callbacks should return as fast as possible. On the upside, with this implementation, there are no threading issues with timer callbacks.

If anyone can spot any gotchas that I'm not aware of, especially as they relate to using the SDL library, I would really appreciate it.

Timer header file:

#ifndef TIMER_H
#define TIMER_H

typedef struct Timer Timer;
typedef void(*TimerCallback)(void *data);

/*
  Initializes the timer mechanism, and allocates resources for 'nTimers'
  number of simultaneous timers.

  Returns non-zero on failure.
*/
int timer_InitTimers(int nTimers);

/*
  Add this to the main game loop, either before or after the loop that
  polls events. If timing is very critical, add it both before and after.
*/
void timer_PollTimers(void);

/*
  Creates an idle timer that has to be started with a call to 'timer_Start()'.

  Returns NULL on failure. Will fail if 'timer_InitTimers()' has not already
  been called.
*/
Timer *timer_Create(Uint32 interval, TimerCallback fCallback, void *data);

/*
  Pauses a timer. If the timer is already paused, this is a no-op.

  Fails with non-zero if 'timer' is NULL or not a valid timer.
*/
int timer_Pause(Timer *timer);

/*
  Starts a timer. If the timer is already running, this function resets the
  delta time for the timer back to zero.

  Fails with non-zero if 'timer' is NULL or not a valid timer.
*/
int timer_Start(Timer *timer);

/*
  Cancels an existing timer. If 'timer' is NULL, this is a no-op.
*/
void timer_Cancel(Timer *timer);

/*
  Releases the resources allocated for the timer mechanism. Call at program
  shutdown, along with 'SDL_Quit()'.
*/
void timer_Quit(void);

/*
  Returns true if the timer is running, or false if the timer is paused or
  is NULL.
*/
int timer_IsRunning(Timer *timer);

#endif

Timer source file:

#include <SDL.h>
#include "timer.h"

static Timer *Chunk; /* BLOB of timers to use */
static int ChunkCount;
static Timer *Timers; /* Linked list of active timers */
static Uint64 TicksPerMillisecond;
static Uint64 Tolerance; /* Fire the timer if it's this close */

struct Timer {
  int active;
  int running;
  TimerCallback callback;
  void *user;
  Timer *next;
  Uint64 span;
  Uint64 last;
};

static void addTimer(Timer *t) {
  Timer *n = NULL;

  if (Timers == NULL) {
    Timers = t;
  }
  else {
    n = Timers;
    while (n->next != NULL) {
      n = n->next;
    }
    n->next = t;
  }
}

static void removeTimer(Timer *t) {
  Timer *n = NULL;
  Timer *p = NULL;

  if (t == Timers) {
    Timers = Timers->next;
  }
  else {
    p = Timers;
    n = Timers->next;
    while (n != NULL) {
      if (n == t) {
        p->next = t->next;
        SDL_memset(n, 0, sizeof(*n));
        break;
      }
      p = n;
      n = n->next;
    }
  }
}

int timer_InitTimers(int n) {
  TicksPerMillisecond = SDL_GetPerformanceFrequency() / 1000;
  Tolerance = TicksPerMillisecond / 2; /* 0.5 ms tolerance */
  Chunk = calloc(n, sizeof(Timer));
  if (Chunk == NULL) {
    //LOG_ERROR(Err_MallocFail);
    return 1;
  }
  ChunkCount = n;
  return 0;
}

Timer *timer_Create(Uint32 interval, TimerCallback fCallback, void *data) {
  Timer *t = Chunk;
  int i = 0;

  while (i < ChunkCount) {
    if (!t->active) {
      t->span = TicksPerMillisecond * interval - Tolerance;
      t->callback = fCallback;
      t->user = data;
      t->active = 1;
      addTimer(t);
      return t;
    }
    i++;
    t++;
  }
  return NULL;
}

void timer_PollTimers(void) {
  Timer *t = Timers;
  Uint64 ticks = SDL_GetPerformanceCounter();

  while (t) {
    /* if a timer is not 'active', it shouldn't be 'running' */
    SDL_assert(t->active);

    if (t->running && ticks - t->last >= t->span) {
      t->callback(t->user);
      t->last = ticks;
    }
    t = t->next;
  }
}

int timer_Pause(Timer* t) {
  if (t && t->active) {
    t->running = 0;
    t->last = 0;
    return 0;
  }
  return 1;
}

int timer_Start(Timer *t) {
  if (t && t->active) {
    t->running = 1;
    t->last = SDL_GetPerformanceCounter();
    return 0;
  }
  return 1;
}

void timer_Cancel(Timer *t) {
  if (t) removeTimer(t);
}

void timer_Quit(void) {
  Timers = NULL;
  free(Chunk);
}

int timer_IsRunning(Timer *t) {
  if (t) {
    return t->running;
  }
  return 0;
}

Test program:

#include <stdio.h>
#include <SDL.h>
#include "timer.h"

Uint32 EVENT_TYPE_TIMER_RED;
Uint32 EVENT_TYPE_TIMER_BLUE;
Uint32 EVENT_TYPE_TIMER_GREEN;
Uint32 EVENT_TYPE_TIMER_YELLOW;

Uint32 colorRed;
Uint32 colorBlue;
Uint32 colorGreen;
Uint32 colorYellow;

SDL_Rect rectRed;
SDL_Rect rectBlue;
SDL_Rect rectGreen;
SDL_Rect rectYellow;

Timer* timerRed;
Timer* timerBlue;
Timer *timerGreen;
Timer *timerYellow;

int isRed;
int isBlue;
int isGreen;
int isYellow;

static void handleTimerRed(void*);
static void handleTimerBlue(void*);
static void handleTimerGreen(void*);
static void handleTimerYellow(void*);

SDL_Event QuitEvent = { SDL_QUIT };
SDL_Renderer *render;
SDL_Window *window;
SDL_Surface *surface;

static void initGlobals(void) {
  rectRed = (SDL_Rect){ 0, 0, 128, 128 };
  rectBlue = (SDL_Rect){ 640 - 128, 0, 128, 128 };
  rectGreen = (SDL_Rect){ 0, 480 - 128, 128, 128 };
  rectYellow = (SDL_Rect){ 640 - 128, 480 - 128, 128, 128 };

  EVENT_TYPE_TIMER_RED = SDL_RegisterEvents(4);
  EVENT_TYPE_TIMER_BLUE = EVENT_TYPE_TIMER_RED + 1;
  EVENT_TYPE_TIMER_GREEN = EVENT_TYPE_TIMER_RED + 2;
  EVENT_TYPE_TIMER_YELLOW = EVENT_TYPE_TIMER_RED + 3;

  timerRed = timer_Create(250, handleTimerRed, NULL);
  timerBlue = timer_Create(500, handleTimerBlue, NULL);
  timerGreen = timer_Create(750, handleTimerGreen, NULL);
  timerYellow = timer_Create(1000, handleTimerYellow, NULL);

  colorRed = SDL_MapRGB(surface->format, 170, 0, 0);
  colorBlue = SDL_MapRGB(surface->format, 0, 0, 170);
  colorGreen = SDL_MapRGB(surface->format, 0, 170, 0);
  colorYellow = SDL_MapRGB(surface->format, 255, 255, 0);

  SDL_FillRect(surface, NULL, 0);
  SDL_FillRect(surface, &rectRed, colorRed);
  SDL_FillRect(surface, &rectBlue, colorBlue);
  SDL_FillRect(surface, &rectGreen, colorGreen);
  SDL_FillRect(surface, &rectYellow, colorYellow);

  isRed = isBlue = isGreen = isYellow = 1;
}

static void handleEvent(SDL_Event *evt) {
  SDL_Texture *tex;

  if (evt->type == SDL_KEYDOWN) {
    if (evt->key.keysym.sym == SDLK_ESCAPE) {
      SDL_PushEvent(&QuitEvent);
    }
    else if (evt->key.keysym.sym == SDLK_r) {
      if (timer_IsRunning(timerRed)) {
        timer_Pause(timerRed);
      }
      else {
        timer_Start(timerRed);
      }
    }
    else if (evt->key.keysym.sym == SDLK_b) {
      if (timer_IsRunning(timerBlue)) {
        timer_Pause(timerBlue);
      }
      else {
        timer_Start(timerBlue);
      }
    }
    else if (evt->key.keysym.sym == SDLK_g) {
      if (timer_IsRunning(timerGreen)) {
        timer_Pause(timerGreen);
      }
      else {
        timer_Start(timerGreen);
      }
    }
    else if (evt->key.keysym.sym == SDLK_y) {
      if (timer_IsRunning(timerYellow)) {
        timer_Pause(timerYellow);
      }
      else {
        timer_Start(timerYellow);
      }
    }
  }
  else if (evt->type == EVENT_TYPE_TIMER_RED) {
    if (isRed) {
      SDL_FillRect(surface, &rectRed, 0);
      isRed = 0;
    }
    else {
      SDL_FillRect(surface, &rectRed, colorRed);
      isRed = 1;
    }
    tex = SDL_CreateTextureFromSurface(render, surface);
    SDL_RenderCopy(render, tex, NULL, NULL);
    SDL_RenderPresent(render);
    SDL_DestroyTexture(tex);
  }
  else if (evt->type == EVENT_TYPE_TIMER_BLUE) {
    if (isBlue) {
      SDL_FillRect(surface, &rectBlue, 0);
      isBlue = 0;
    }
    else {
      SDL_FillRect(surface, &rectBlue, colorBlue);
      isBlue = 1;
    }
    tex = SDL_CreateTextureFromSurface(render, surface);
    SDL_RenderCopy(render, tex, NULL, NULL);
    SDL_RenderPresent(render);
    SDL_DestroyTexture(tex);
  }
  else if (evt->type == EVENT_TYPE_TIMER_GREEN) {
    if (isGreen) {
      SDL_FillRect(surface, &rectGreen, 0);
      isGreen = 0;
    }
    else {
      SDL_FillRect(surface, &rectGreen, colorGreen);
      isGreen = 1;
    }
    tex = SDL_CreateTextureFromSurface(render, surface);
    SDL_RenderCopy(render, tex, NULL, NULL);
    SDL_RenderPresent(render);
    SDL_DestroyTexture(tex);
  }
  else if (evt->type == EVENT_TYPE_TIMER_YELLOW) {
    if (isYellow) {
      SDL_FillRect(surface, &rectYellow, 0);
      isYellow = 0;
    }
    else {
      SDL_FillRect(surface, &rectYellow, colorYellow);
      isYellow = 1;
    }
    tex = SDL_CreateTextureFromSurface(render, surface);
    SDL_RenderCopy(render, tex, NULL, NULL);
    SDL_RenderPresent(render);
    SDL_DestroyTexture(tex);
  }
}

int main(int argc, char* args[])
{
  (void)(argc);
  (void)(args);
  SDL_Event event = { 0 };
  int run = 0;
  SDL_Texture *texture = NULL;

  if (SDL_Init(SDL_INIT_VIDEO | SDL_INIT_AUDIO) < 0) {
    printf("Failed to init SDL library.");
    return 1;
  }
  if (SDL_CreateWindowAndRenderer(640,
                                  480,
                                  SDL_WINDOW_RESIZABLE,
                                  &window,
                                  &render))
  {
    printf("Could not create main window and renderer.");
    return 1;
  }
  if (SDL_RenderSetLogicalSize(render, 640, 480)) {
    printf("Could not set logical window size.");
    return 1;
  }
  if (timer_InitTimers(4)) {
    printf("Could not init timers.");
    return 1;
  }
  surface = SDL_GetWindowSurface(window);
  initGlobals();
  texture = SDL_CreateTextureFromSurface(render, surface);
  SDL_RenderCopy(render, texture, NULL, NULL);
  SDL_RenderPresent(render);
  SDL_DestroyTexture(texture);
  run = 1;
  while (run) {
    timer_PollTimers();
    while (SDL_PollEvent(&event)) {
      if (event.type == SDL_QUIT) {
        run = 0;
        break;
      }
      handleEvent(&event);
    }
    /* or here timer_PollTimers(); */
  }
  SDL_Quit();
  timer_Quit();
  return 0;
}

static void handleTimerRed(void *ignored) {
  SDL_Event event;
  (void)(ignored);

  event.type = EVENT_TYPE_TIMER_RED;
  SDL_PushEvent(&event);
}

static void handleTimerBlue(void *ignored) {
  SDL_Event event;
  (void)(ignored);

  event.type = EVENT_TYPE_TIMER_BLUE;
  SDL_PushEvent(&event);
}

static void handleTimerGreen(void *ignored) {
  SDL_Event event;
  (void)(ignored);

  event.type = EVENT_TYPE_TIMER_GREEN;
  SDL_PushEvent(&event);
}

static void handleTimerYellow(void *ignored) {
  SDL_Event event;
  (void)(ignored);

  event.type = EVENT_TYPE_TIMER_YELLOW;
  SDL_PushEvent(&event);
}
\$\endgroup\$

2 Answers 2

Reset to default
6
\$\begingroup\$

I don't see too much that I feel like complaining about.

Standard Types

In timer.h, you use Uint32. This is not a standard type. It comes from SDL, which in turn comes from stdint.h:

https://github.com/spurious/SDL-mirror/blob/17af4584cb28cdb3c2feba17e7d989a806007d9f/include/SDL_stdinc.h#L203

typedef uint32_t Uint32;

Headers should be include-order-agnostic; that is, your header should work even if it's included first (which currently it won't). One solution is to #include <stdint.h> in your header, and use its types rather than the SDL types.

For is your friend

This:

int i = 0;
while (i < ChunkCount) {
    ...
    i++;

is more simply expressed as

for (int i = 0; i < ChunkCount; i++) {
\$\endgroup\$
2
  • \$\begingroup\$ I agree with your points in general, but SDL takes no dependency on the C runtime, and those types, along with SDL_memset and SDL_assert, lets me avoid that as well. Usually I prefer a simple for loop over a while loop also, but in this case I would have to use Chunk[i].member, instead of just t->member. And besides, I have to return a pointer anyway, so I might as well use a pointer as I go. \$\endgroup\$
    – Mark Benningfield
    Commented Mar 16, 2019 at 6:24
  • \$\begingroup\$ You can use a for and still use t. t is maintained separately from i. \$\endgroup\$
    – Reinderien
    Commented Mar 17, 2019 at 17:38
5
\$\begingroup\$

Update

After further usage, I did discover one issue with regard to usability. Once a Timer instance has been cancelled, it is no longer valid and should not be used. So, if the pointer variable is going to hang around, it should be set to NULL.

The game I'm working on has several reentrant state machines, some of which contain Timer variables. So, everywhere I cancelled a timer, I had to be sure to set the variable to NULL, so that if the state machine is entered again, I could check it. I decided to enforce this by refactoring the timer_Cancel() function to accept the address of a Timer instance, and set it to NULL before returning from that function.

Here is the revised code, along with @Reinderien 's suggestions for improvement. Instead of including stdint.h, I moved the #include <SDL.h> line to the timer header file to eliminate the header dependency. That way, I avoid including any headers from the standard libraries, which means I don't have to take a dependency on the C runtime. I also refactored the while loop, which looks a little cleaner, but still sorta sets off some bells, because the first thing I look at when reviewing a for loop is how the iterator variable is used. I tend to get suspicious if it isn't used at all. Probably a question of style more than anything, I guess. The test program code is unchanged, so I didn't include it here.

Timer.h

#ifndef TIMER_H
#define TIMER_H

#include <SDL.h>

typedef struct Timer Timer;
typedef void(*TimerCallback)(void *data);

/*
    Initializes the timer mechanism, and allocates resources for 'nTimers'
    number of simultaneous timers.

    Returns non-zero on failure.
*/
int timer_InitTimers(int nTimers);

/*
    Add this to the main game loop, either before or after the loop that
    polls events. If timing is very critical, add it both before and after.
*/
void timer_PollTimers(void);

/*
    Creates an idle timer that has to be started with a call to 'timer_Start()'.

    Returns NULL on failure. Will fail if 'timer_InitTimers()' has not already
    been called.
*/
Timer *timer_Create(Uint32 interval, TimerCallback fCallback, void *data);

/*
    Pauses a timer. If the timer is already paused, this is a no-op.

    Fails with non-zero if 'timer' is NULL or not a valid timer.
*/
int timer_Pause(Timer *timer);

/*
    Starts a timer. If the timer is already running, this function resets the
    delta time for the timer back to zero.

    Fails with non-zero if 'timer' is NULL or not a valid timer.
*/
int timer_Start(Timer *timer);

/*
    Cancels an existing timer. If the timer is NULL, this is a no-op.

    Accepts the address of a 'Timer' pointer, and sets that pointer to
    NULL before returning.
*/
void timer_Cancel(Timer **timer);

/*
    Releases the resources allocated for the timer mechanism. Call at program
    shutdown, along with 'SDL_Quit()'.
*/
void timer_Quit(void);

/*
    Returns true if the timer is running, or false if the timer is paused or
    is NULL.
*/
int timer_IsRunning(Timer *timer);

#endif

Timer.c

#include "timer.h"

static Timer *Chunk; /* BLOB of timers to use */
static int ChunkCount;
static Timer *Timers; /* Linked list of active timers */
static Uint64 TicksPerMillisecond;
static Uint64 Tolerance; /* Fire the timer if it's this close */

struct Timer {
  int active;
  int running;
  TimerCallback callback;
  void *user;
  Timer *next;
  Uint64 span;
  Uint64 last;
};

static void addTimer(Timer *t) {
  Timer *n = NULL;

  if (Timers == NULL) {
    Timers = t;
  }
  else {
    n = Timers;
    while (n->next != NULL) {
      n = n->next;
    }
    n->next = t;
  }
}

static void removeTimer(Timer *t) {
  Timer *n = NULL;
  Timer *p = NULL;

  if (t == Timers) {
    Timers = Timers->next;
  }
  else {
    p = Timers;
    n = Timers->next;
    while (n != NULL) {
      if (n == t) {
        p->next = t->next;
        SDL_memset(n, 0, sizeof(*n));
        break;
      }
      p = n;
      n = n->next;
    }
  }
}

int timer_InitTimers(int n) {
  TicksPerMillisecond = SDL_GetPerformanceFrequency() / 1000;
  Tolerance = TicksPerMillisecond / 2; /* 0.5 ms tolerance */
  Chunk = calloc(n, sizeof(Timer));
  if (Chunk == NULL) {
    //LOG_ERROR(Err_MallocFail);
    return 1;
  }
  ChunkCount = n;
  return 0;
}

Timer *timer_Create(Uint32 interval, TimerCallback fCallback, void *data) {
  Timer *t = Chunk;

  for (int i = 0; i < ChunkCount; i++) {
    if (!t->active) {
      t->span = TicksPerMillisecond * interval - Tolerance;
      t->callback = fCallback;
      t->user = data;
      t->active = 1;
      addTimer(t);
      return t;
    }
    t++;
  }
  return NULL;
}

void timer_PollTimers(void) {
  Timer *t = Timers;
  Uint64 ticks = SDL_GetPerformanceCounter();

  while (t) {
    /* if a timer is not 'active', it shouldn't be 'running' */
    SDL_assert(t->active);

    if (t->running && ticks - t->last >= t->span) {
      t->callback(t->user);
      t->last = ticks;
    }
    t = t->next;
  }
}

int timer_Pause(Timer* t) {
  if (t && t->active) {
    t->running = 0;
    t->last = 0;
    return 0;
  }
  return 1;
}

int timer_Start(Timer *t) {
  if (t && t->active) {
    t->running = 1;
    t->last = SDL_GetPerformanceCounter();
    return 0;
  }
  return 1;
}

void timer_Cancel(Timer **t) {
  if (*t) {
    removeTimer(*t);
    *t = NULL;
  }
}

void timer_Quit(void) {
  Timers = NULL;
  free(Chunk);
}

int timer_IsRunning(Timer *t) {
  if (t) {
    return t->running;
  }
  return 0;
}
\$\endgroup\$
3
  • \$\begingroup\$ If you want that reviewed and the change was major you should post it as a new question \$\endgroup\$
    – Harald Scheirich
    Commented Mar 19, 2019 at 17:24
  • \$\begingroup\$ @HaraldScheirich: Sorry, no I don't need that reviewed. I've asked on Meta whether I should just delete this answer or not. \$\endgroup\$
    – Mark Benningfield
    Commented Mar 20, 2019 at 0:08
  • 1
    \$\begingroup\$ @HaraldScheirich Here is a link to the meta post. \$\endgroup\$
    – Peilonrayz ♦
    Commented Mar 20, 2019 at 2:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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