0

With the following function in Visual Studio that are calling other functions from a class called CSerial, why is it a good idea to use dynamic memory allocation as in this example? The functionality of the program is to send data from a computer to an Arduino through the USB port. It is coded in Visual Studio.

bool sendExample(int port, int baudRate)
{
    char data[4];

    CSerial* s = new CSerial();

    if(!s->Open(port, baudRate))
    {
        std_out << _T("Could not open COM") << port << endl;
        return false;
    }


    // Sending a string of 4 characters
    data[0] = 0x31; 
    data[1] = 0x32;
    data[2] = 0x33;
    data[3] = 0x0D; // ASCII CR
    s->SendData(data, 4);

    s->Close();

    delete s;

}
walnut
  • 21,629
  • 4
  • 23
  • 59
user164324
  • 5
  • 3
  • 2
    It isn't a good idea, especially since there is a leak which wouldn't exist with static storage duration. – tkausl Dec 29 '19 at 20:00
  • @tkausl Are you sure? I have heard it is not a good idea for the Arduino itself, but this is on a pc. – user164324 Dec 29 '19 at 20:06
  • 1
    @user164324 Where did you hear that? Even if dynamic allocation is required for some reason, `std::unique_ptr` should be used instead of raw `new`/`delete`. – walnut Dec 29 '19 at 20:07
  • In general, don't `new` what you don't have to, and these days you almost never have to. See [Why should C++ programmers minimize use of 'new'?](https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new) for more information and alternatives. – user4581301 Dec 29 '19 at 20:10
  • 1
    Where does `CSerial` come from? The naming looks MFC, but a quick google turns up nothing. – user4581301 Dec 29 '19 at 20:13
  • 1
    @user4581301 Code for `CSerial` seems to be reproduced in OP's [other question](https://stackoverflow.com/questions/59139479/serial-communication-between-pc-and-arduino-through-usb) and googling for parts of it leads to many board posts sharing and asking about this class, but there does not seem to be a definitive origin for it. – walnut Dec 29 '19 at 20:22
  • Thanks, @walnut . Looking CSerial over, I can see one (bad) reason to always use `CSerial` through a pointer: It appears to violate the [Rule of Three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three), so making copies of a `CSerial`instance are fatal. Always `new`ing the instance probably stems from this simple design mistake. – user4581301 Dec 29 '19 at 20:28
  • @user4581301 The oldest source I can come up with is [this site](https://www.codeguru.com/cpp/i-n/network/serialcommunications/article.php/c2503/CSerial--A-C-Class-for-Serial-Communications.htm) from 1999, *slightly* out-dated... And yes, that would explain why the class is being used this way. Of course the correct course of action would be to *fix* the class or use a modern alternative, rather than working around the undefined behavior that is waiting to happen sooner or later. – walnut Dec 29 '19 at 20:29
  • @user164324 Your question is still listed as unanswered. If there's anything unclear in my answer, just ask and I'll try to improve it. – Ted Lyngmo Jan 02 '20 at 09:23

1 Answers1

3

In this example, an automatic variable should be the obvious choice:

CSerial s;

Local automatic variables are usually allocated on the stack and if you for some reason want to avoid that (because of a small stack or similar) in favour of dynamic allocation, which is usually done on the heap, use a smart pointer - or else it'll leak if the port fails to open or if any function throws an exception:

std::unique_ptr<CSerial> s = std::make_unique<CSerial>();

Why would it be good to use dynamic memory allocation with serial communication?

I've never seen a connection between automatic memory allocation and serial communication that's looked troublesome. I'd say, forget about it. Use automatic variables until you find a problem. Don't make programs a lot more complicated than they need to be.

Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108