Merge pull request firmata#41 from mmurdoch/dev

Fixed issue firmata#36 (setFirmwareVersion leaks memory)

Author: soundanalogous

Firmata.cpp

@@ -49,6 +49,7 @@ void FirmataClass::endSysex(void)
 FirmataClass::FirmataClass()
 {
   firmwareVersionCount = 0;
+  firmwareVersionVector = 0;
   systemReset();
 }
 
@@ -126,6 +127,9 @@ void FirmataClass::setFirmwareNameAndVersion(const char *name, byte major, byte
     firmwareVersionCount = strlen(name) + 2;
     filename = name;
   }
+
+  free(firmwareVersionVector);
+
   firmwareVersionVector = (byte *) malloc(firmwareVersionCount);
   firmwareVersionVector[firmwareVersionCount] = 0;
   firmwareVersionVector[0] = major;
@@ -136,6 +140,13 @@ void FirmataClass::setFirmwareNameAndVersion(const char *name, byte major, byte
   //             (char)major, (char)minor, firmwareVersionVector);
 }
 
+void FirmataClass::unsetFirmwareVersion()
+{
+  firmwareVersionCount = 0;
+  free(firmwareVersionVector); 
+  firmwareVersionVector = 0;
+}
+ 
 //------------------------------------------------------------------------------
 // Serial Receive Handling
 
@@ -179,7 +190,7 @@ void FirmataClass::processInput(void)
   int command;
 
   // TODO make sure it handles -1 properly
-
+  
   if (parsingSysex) {
     if(inputData == END_SYSEX) {
       //stop sysex byte      

Firmata.h

@@ -98,6 +98,7 @@ class FirmataClass
     void printFirmwareVersion(void);
   //void setFirmwareVersion(byte major, byte minor);  // see macro below
     void setFirmwareNameAndVersion(const char *name, byte major, byte minor);
+    void unsetFirmwareVersion();
 /* serial receive handling */
     int available(void);
     void processInput(void);

test/unit/firmata_test/firmata_test.ino

@@ -13,83 +13,47 @@ void loop()
   suite.run();
 }
 
-class InMemoryStream : public Stream
-{
-public:
-  virtual ~InMemoryStream() 
-  {
-  }
-
-  size_t write(uint8_t val)
-  {
-    _bytesWritten += (char) val;
-
-    return size_t(1);
-  }
-
-  void flush() 
-  {
-  }
-
-  const String& bytesWritten() 
-  {
-    return _bytesWritten;
-  }
-
-  void nextByte(byte b)
-  {
-    _nextByte = b;
-  }
-
-  int available() 
-  {
-    return 1;
-  }
-
-  int read() 
-  {
-    return _nextByte;
-  }
-
-  int peek()
-  {
-    return _nextByte;
-  }
-
-private:
-  String _bytesWritten;
-  byte _nextByte;
-};
-
 void assertStringsEqual(Test& __test__, const char* expected, const String& actual)
 {
   size_t expectedLength = strlen(expected);
   assertEquals(expectedLength, actual.length());
-  for (size_t i = 0; i < strlen(expected); i++)
+  for (size_t i = 0; i < expectedLength; i++)
   {
     assertEquals(expected[i], actual[i]);
   }
 }
 
+test(setFirmwareVersionDoesNotLeakMemory)
+{
+  Firmata.setFirmwareVersion(1, 0);
+  int initialMemory = freeMemory();
+
+  Firmata.setFirmwareVersion(1, 0);
+
+  assertEquals(0, initialMemory - freeMemory());
+  
+  Firmata.unsetFirmwareVersion();
+}
+
 test(beginPrintsVersion)
 {
-  InMemoryStream stream;
+  FakeStream stream;
 
   Firmata.begin(stream);
 
   char expected[] = 
   {
-    0xF9, // Version reporting identifier 
-    2,    // Major version number
-    3,    // Minor version number
+    REPORT_VERSION,
+    FIRMATA_MAJOR_VERSION,
+    FIRMATA_MINOR_VERSION,
     0
   };
   assertStringsEqual(__test__, expected, stream.bytesWritten());
 }
 
 void processMessage(const byte* message, size_t length)
 {
-  InMemoryStream stream;
+  FakeStream stream;
   Firmata.begin(stream);
 
   for (size_t i = 0; i < length; i++)
@@ -107,8 +71,14 @@ void writeToDigitalPort(byte port, int value)
   _digitalPortValue = value;
 }
 
+void setupDigitalPort() {
+  _digitalPort = 0;
+  _digitalPortValue = 0;
+}
+
 test(processWriteDigital_0)
 {
+  setupDigitalPort();
   Firmata.attach(DIGITAL_MESSAGE, writeToDigitalPort);
 
   byte message[] = { DIGITAL_MESSAGE, 0, 0 };
@@ -119,6 +89,7 @@ test(processWriteDigital_0)
 
 test(processWriteDigital_127)
 {
+  setupDigitalPort();
   Firmata.attach(DIGITAL_MESSAGE, writeToDigitalPort);
 
   byte message[] = { DIGITAL_MESSAGE, 127, 0 };
@@ -127,18 +98,9 @@ test(processWriteDigital_127)
   assertEquals(127, _digitalPortValue);
 }
 
-test(processWriteDigitalStripsTopBit)
-{
-  Firmata.attach(DIGITAL_MESSAGE, writeToDigitalPort);
-  
-  byte message[] = { DIGITAL_MESSAGE, B11111111, 0 };
-  processMessage(message, 3);
-
-  assertEquals(B01111111, _digitalPortValue);
-}
-
 test(processWriteDigital_128)
 {
+  setupDigitalPort();
   Firmata.attach(DIGITAL_MESSAGE, writeToDigitalPort);
 
   byte message[] = { DIGITAL_MESSAGE, 0, 1 };
@@ -149,6 +111,7 @@ test(processWriteDigital_128)
 
 test(processWriteLargestDigitalValue)
 {
+  setupDigitalPort();
   Firmata.attach(DIGITAL_MESSAGE, writeToDigitalPort);
 
   byte message[] = { DIGITAL_MESSAGE, 0x7F, 0x7F };
@@ -160,6 +123,7 @@ test(processWriteLargestDigitalValue)
 
 test(defaultDigitalWritePortIsZero)
 {
+  setupDigitalPort();
   Firmata.attach(DIGITAL_MESSAGE, writeToDigitalPort);
 
   byte message[] = { DIGITAL_MESSAGE, 0, 0 };
@@ -170,6 +134,7 @@ test(defaultDigitalWritePortIsZero)
 
 test(specifiedDigitalWritePort)
 {
+  setupDigitalPort();
   Firmata.attach(DIGITAL_MESSAGE, writeToDigitalPort);
 
   byte message[] = { DIGITAL_MESSAGE + 1, 0, 0 };