4
\$\begingroup\$

I've been developing this application as my first C++ project and am at a point where it is functional and would like to take a step back and critique/improve my code from a standpoint of manageability and better coding practices. Additionally, it has some stability issues which I need to address, so I thought instead of dumping a ton of code out there and asking for feedback we'd start at the beginning.

This application has multiple threads with shared resources, and sharing these resources across threads was particularly tricky for a novice programmer. So please see the below 'main.cpp' of my application that creates and waits for the destruction of all threads. Additionally these notes might be helpful:

  • The threads are meant to be asynchronous and operate at different rates. For example, the thread that updates the display operates a lower frame-rate than the thread that actually captures the image. I particularly wanted to avoid the scenario where the threads were locked together and the slowest thread set the pace for all others.
  • All threads are self exiting based on the 'exitsignal' variable. Essentially this code runs on raspberry pi in an automobile and when 12V signal is lost (car shut-off), a 3.3V input goes low and the GPIO thread writes 'exitsignal' true and all the other threads exit on their next iteration of their for-loop.
  • The GPIO thread is finally exited by the 'shutdownsignal' written at the end.

Thanks in advance, and all (constructive) input is appreciated.

int main()
{
    std::cout << "Program launched, starting log file..." << '\n';
    //Quick and dirty log file
    std::ofstream out("/log.txt", std::ios_base::app | std::ios_base::out);
    std::streambuf *coutbuf = std::cout.rdbuf();
    std::cout.rdbuf(out.rdbuf());

    //Create log header
    PrintHeader();

    //Check XML Properties
    if (settings::kreadsuccess < 0) {
        std::cout << "XML reading failed, using defaults." << '\n';
    } else {
        std::cout << "XML reading successful!" << '\n';
    }

    //Create shared resources
    std::atomic<bool> exitsignal{ false };
    std::atomic<bool> shutdownsignal{ false };
    cv::Mat captureimage;
    std::mutex capturemutex;
    cv::Mat displayimage;
    std::mutex displaymutex;
    ProcessValues processvalues;

    //Start threads

    //Start image capture thread
    std::thread t_imagecapture( CaptureImageThread,
                                &captureimage,
                                &capturemutex,
                                &exitsignal );
    //Start image processor thread
    std::thread t_imageprocessor( ProcessImageThread,
                                  &captureimage,
                                  &capturemutex,
                                  &processvalues,
                                  &exitsignal );
    //Start display thread
    std::thread t_displayupdate( DisplayUpdateThread,
                                 &displayimage,
                                 &displaymutex,
                                 &exitsignal );
    //Start image editor thread
    std::thread t_imeageeditor( ImageEditorThread,
                                &captureimage,
                                &capturemutex,
                                &displayimage,
                                &displaymutex,
                                &processvalues,
                                &exitsignal );
    //Start video writer thread
    std::thread t_videowriter( VideoWriterThread,
                               &captureimage,
                               &capturemutex,
                               &displayimage,
                               &displaymutex,
                               &exitsignal );
    //Start GPS poling thread
    std::thread t_gpspolling( GpsPollingThread,
                              &processvalues,
                              &exitsignal );
    //Start LIDAR polling thread
    std::thread t_lidarpolling( LidarPolingThread,
                                &processvalues,
                                &exitsignal );
    //Start GPIO thread
    std::thread t_gpiohandler( GpioHandlerThread,
                               &processvalues,
                               &exitsignal,
                               &shutdownsignal );

    //Set pace setter class!
    PaceSetter mypacesetter( 2, "main" );

    while( !exitsignal ){
        //Should check all threads still running
        mypacesetter.SetPace();
    }

    //Handle all the threads
    t_videowriter.join();
    t_imageprocessor.join();
    t_lidarpolling.join();
    t_gpspolling.join();
    t_imeageeditor.join();
    t_imagecapture.join();
    t_displayupdate.join();
    shutdownsignal = true;
    t_gpiohandler.join();
    std::cout.rdbuf(coutbuf);
    std::cout << "Program exited gracefully!"  << '\n';

    return 0;
}

This is what the code looks like now with a common polling thread for the 'IO':

int main()
{
    std::cout << "Program launched, starting log file..." << '\n';
    //Quick and dirty log file
    std::ofstream out("/log.txt", std::ios_base::app | std::ios_base::out);
    std::streambuf *coutbuf = std::cout.rdbuf();
    std::cout.rdbuf(out.rdbuf());

    //Create log header
    PrintHeader();

    //Check XML Properties
    if (settings::kreadsuccess < 0) {
        std::cout << "XML reading failed, using defaults." << '\n';
    } else {
        std::cout << "XML reading successful!" << '\n';
    }

    //Create shared resources
    std::atomic<bool> exitsignal{ false };
    cv::Mat captureimage;
    std::mutex capturemutex;
    cv::Mat displayimage;
    std::mutex displaymutex;
    ProcessValues processvalues;

    //Start threads

    //Start image capture thread
    std::thread t_imagecapture( CaptureImageThread,
                                &captureimage,
                                &capturemutex,
                                &exitsignal );
    //Start image processor thread
    std::thread t_imageprocessor( ProcessImageThread,
                                  &captureimage,
                                  &capturemutex,
                                  &processvalues,
                                  &exitsignal );
    //Start display thread
    std::thread t_displayupdate( DisplayUpdateThread,
                                 &displayimage,
                                 &displaymutex,
                                 &exitsignal );
    //Start image editor thread
    std::thread t_imeageeditor( ImageEditorThread,
                                &captureimage,
                                &capturemutex,
                                &displayimage,
                                &displaymutex,
                                &processvalues,
                                &exitsignal );
    //Start video writer thread
    std::thread t_videowriter( VideoWriterThread,
                               &captureimage,
                               &capturemutex,
                               &displayimage,
                               &displaymutex,
                               &exitsignal );

    //Set pace setter class!
    int pollrate { std::max(std::max(settings::comm::kpollrategps,
                                     settings::comm::kpollratelidar),
                            settings::comm::kpollrategpio) };
    int gpspollinterval{ pollrate / settings::comm::kpollrategps };
    int gpiopollinterval{ pollrate / settings::comm::kpollrategpio };
    int fcwpollinterval{ pollrate / settings::comm::kpollratelidar };
    PaceSetter mypacesetter( pollrate, "Main" );

    //Setup polling

    //GPIO
    bool gpiopoll{ false };
    if ( settings::gpio::kenabled ) {
        try {
            gpiopoll = GpioHandlerSetup();
        } catch ( const std::exception& ex ) {
            std::cout << "GPIO handler setup threw exception: "<< ex.what() << '\n';
            gpiopoll = false;
        } catch ( const std::string& str ) {
            std::cout << "GPIO handler setup threw exception: "<< str << '\n';
            gpiopoll = false;
        } catch (...) {
            std::cout << "GPIO handler setup threw exception of unknown type!" << '\n';
            gpiopoll = false;
        }
    }
    //GPS
    gpsmm* gpsrecv{ NULL };
    bool gpspoll{ false };
    bool timeset{ false };
    if ( settings::gps::kenabled ) {
        try {
            gpsrecv = new gpsmm("localhost", DEFAULT_GPSD_PORT);
            gpspoll = GpsPollingSetup( gpsrecv );
        } catch ( const std::exception& ex ) {
            std::cout << "GPS polling setup threw exception: "<< ex.what() << '\n';
            gpspoll = false;
        } catch ( const std::string& str ) {
            std::cout << "GPS polling setup threw exception: "<< str << '\n';
            gpspoll = false;
        } catch (...) {
            std::cout << "GPS polling setup threw exception of unknown type!" << '\n';
            gpspoll = false;
        }
    } else {
        gpsrecv = NULL;
    }
    //FCW
    FcwTracker* fcwtracker{ NULL };
    bool fcwpoll{ false };
    int dacmodule{ -1 };
    if ( settings::fcw::kenabled ) {
        try {
            fcwtracker = new FcwTracker( settings::fcw::ksamplestoaverage );
            dacmodule = LidarPollingSetup();
            if ( dacmodule >= 0 ) fcwpoll = true;
        } catch ( const std::exception& ex ) {
            std::cout << "FCW setup threw exception: "<< ex.what() << '\n';
            fcwpoll = false;
        } catch ( const std::string& str ) {
            std::cout << "FCW setup threw exception: "<< str << '\n';
            fcwpoll = false;
        } catch (...) {
            std::cout << "FCW setup threw exception of unknown type!" << '\n';
            fcwpoll = false;
        }
    } else {
        fcwtracker = NULL;
    }

    //Loop
    int i{ 0 };
    do {
        //Increment
        i++;
        //Flush cout buffer every second
        if ( i % gpspollinterval == 0 ) std::cout << std::flush;
        if ( (gpspoll) &&
             (i % gpspollinterval == 0) ) GpsPolling( processvalues,
                                                      gpsrecv,
                                                      timeset );
        if ( (gpiopoll) &&
             (i % gpiopollinterval == 0) ) GpioHandler( processvalues,
                                                        exitsignal );
        if ( (fcwpoll) &&
             (i % fcwpollinterval == 0) ) LidarPolling( processvalues,
                                                        dacmodule,
                                                        fcwtracker );

        //Set Pace
        mypacesetter.SetPace();
    } while( !exitsignal );

    //Cleanup variables
    delete gpsrecv;
    gpsrecv = NULL;
    delete fcwtracker;
    fcwtracker = NULL;

    //Handle all the threads
    t_videowriter.join();
    t_imageprocessor.join();
    t_imeageeditor.join();
    t_imagecapture.join();
    t_displayupdate.join();
    std::cout.rdbuf(coutbuf);
    std::cout << "Program exited gracefully!"  << '\n';

    return 0;
}

Probably is the polling of the GPS takes much longer than the LIDAR and GPIO, so it's pretty limiting of the poll rate of those devices. However, there is now a noticeable reduction in CPU utilization.

\$\endgroup\$
  • 2
    \$\begingroup\$ I'm not so convinced organizing that code in all different threads is a good idea. Wouldn't it be better to run a main loop and execute the different actions at the rate they need based on time measurement? Most gaming engines work that way AFAIK. \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 14:09
  • \$\begingroup\$ So instead of each thread having a for-loop, there is one main loop that calls each function to once? I haven't considered that. I think the disadvantage there would be performance, if I call the thread to capture the image then immediately call the thread to process the image, the image processing thread is going to wait on the image capturing thread anyways because of the mutex. However, with the threads running in a loop asynchronously the timing may not always be optimal, but the image processor request won't be immediately after the image capture request every time. \$\endgroup\$ – DrTarr Dec 30 '16 at 14:22
  • 2
    \$\begingroup\$ Threads do not magically speed up performance, in fact they come with cost and overhead. You need to be very careful figuring out the part's that are independent from accessing shared resources and can be executed at different CPU cores in parallel. \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 14:25
  • \$\begingroup\$ For sure I don't know how expensive creating all these threads really are. I figured it'd be better for performance reasons to create each thread once and be done with it, but possibly that's not the best solution. When I went with this approach I thought some of the threads might not really bring any performance benefits but wouldn't be detrimental, so I was playing it on the safe side. I will do some bench-marking by combining some threads, such as the image capture and image processor. The 3 polling threads for the gps, IO, and lidar are easy to combine, but also clearly independent. \$\endgroup\$ – DrTarr Dec 30 '16 at 14:51
  • \$\begingroup\$ Actually I've been down this road before. I wanted the image capture and image processor to be independent so I could begin capturing the next image while I was still processing the old one.. And the 3 polling threads I didn't combine because I wanted different rates between each one (1hz for gps, 10 hz for lidar, etc). But for clarification, are you suggest I just ditch the multi-threading, or that I just create and join a thread each iteration of a for-loop in the main function? \$\endgroup\$ – DrTarr Dec 30 '16 at 14:56
2
\$\begingroup\$

Don't use threads to scatter your timing schedule of independent actions

In most cases timing schedules can be solved with a simple main loop and time measurement like such:

 bool exit = false; // In case you have concurrency here a `std::atomic<bool>`
                    // is fine of course
 auto least = std::chrono::high_resolution_clock::now();
 do {
      auto current = std::chrono::high_resolution_clock::now(); 
      auto timeDiff = current - least;
      if(timeDiff >= 20ms)  {
          // do the action every 20 milliseconds
      }
      if(timeDiff >= 1s)  {
          // do the action every second
      }
      // aso.
 } while(!exit);

Threads and synchronization mechanisms come with some cost by means of performance and memory overhead you may want to avoid.

Minimize thread interdependencies

It seems a number of processing steps in your example could be serialized trivially by calling these one after another (has finishied).

You're passing cv::Mat captureimage; as parameter all of the time, but the processing steps are separate threads that have to wait for each others processing step.

Such could be easier solved with a simple function calling sequence.

Reduce the number of threads to the parts that could be run independently

It seems your scenario can be reduced to 3 threads. One that takes the video capture input, one that does the image processing (depends if another CPU core is available), and the last one that feeds the video output.

You likely can synchronize all these using a (std::mutex protected) processing queue of std::queue<cv::Mat> frames.

In that mentioned scenario a std::condition_variable may come in handy to trigger the concurrently waiting std::threads execution, as soon a new captured frame is available, or it's readily processed for output.

\$\endgroup\$
  • \$\begingroup\$ @NateGreco You do not need to prematurely accept my answer. There may be other readers of your question that may apply additional input. Just wait a bit more. I've only put my critiques on your code regarding some major considerations. Seeing an accept might discourage them to view your question and add another answer. \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 15:28
  • \$\begingroup\$ It was certainly the type of guidance I was looking for, much appreciated. I think what I'll test is putting the 3 polling threads (lidar, gps, and gpio) into one loop in the main function like you've suggested. From there maybe the output threads (video writer and display update) can also be combined. One note that wasn't mentioned was there was also multi-threading in the VideoWriter thread to handle writing the queue of cv::Mat's similar to how you described. \$\endgroup\$ – DrTarr Dec 30 '16 at 15:34
  • \$\begingroup\$ @NateGreco Well, I'm glad to help :-) \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 15:36
  • \$\begingroup\$ @NateGreco BTW, there's a load of video processing functionality available with the ffmpeg library. You might be interested to have a look into it. \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 15:38

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.